Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Bug: Modules in errors cause unhandledRejection, tests quit early #4887

Open
4 tasks done
JacobLey opened this issue May 22, 2022 · 4 comments · May be fixed by #5040
Open
4 tasks done

🐛 Bug: Modules in errors cause unhandledRejection, tests quit early #4887

JacobLey opened this issue May 22, 2022 · 4 comments · May be fixed by #5040
Labels
status: in triage a maintainer should (re-)triage (review) this issue type: bug a defect, confirmed by a maintainer

Comments

@JacobLey
Copy link
Contributor

Prerequisites

  • Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
  • Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
  • 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
  • Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

When a mocha test throws an error that contains a Module Mocha will quit the process unexpectedly.

Internally this is caused by Mocha being unable to serialize ("canonicalize") the module object, and throwing an error.

Steps to Reproduce

Simple test:

// my-module.spec.js
import { expect } from 'chai';

describe('Compare imports', () => {
    it('different imports', async () => {
        const modA = await import('mod-a');
        const modB = await import('mod-b);
        // throws
        expect(modA).to.eq(modB);
    });
    it('always true', () => {
        console.log('This will never run');
    });
});

Reproduces how often: 100%

Versions

mocha --version => 10.0.0
node --version => v16.15.0

Additional Information

Will open PR to fix.

  1. Issue comes from canonicalType returning 'module' for modules (I agree with this behavior)
  2. canonicalize does not handle the 'module' case explicitly, so it falls back on implicit stringification
  3. Modules cannot be implicitly stringified (try import('fs').then(fs => fs + '') => Uncaught TypeError: Cannot convert object to primitive value)
  4. Uncaught error is caught + logged + rethrown here
  5. NodeJS quits on uncaught error
@JoshuaKGoldberg
Copy link
Member

@JacobLey I can't figure out how to reproduce this locally. I believe you that it is a real bug 😄 but need an isolated reproduction to test it with. What's the simplest way to get this to happen in a clean/new repository?

For reference, trying this with "type": "module":

import { expect } from 'chai';

describe('Compare imports', () => {
  it('different imports', async () => {
    const modA = await import('dedent');
    const modB = await import('once');
    // throws
    expect(modA).to.eq(modB);
  });
  it('always true', () => {
    console.log('This will never run');
  });
});

...I get fine looking failures:

$ npx mocha test.js

  Compare imports
    1) different imports
This will never run
    ✔ always true


  1 passing (5ms)
  1 failing

  1) Compare imports
       different imports:

      AssertionError: expected { default: [Function dedent], …(1) } to equal { default: [Function wrapper], …(1), …(1) }
      + expected - actual


      at Context.<anonymous> (file:///Users/josh/repos/mocha-context-only-bug/test.js:8:21)

Same with switching to "type": "commonjs" with a require and chai@4.

@JoshuaKGoldberg JoshuaKGoldberg changed the title Modules in errors cause unhandledRejection, tests quit early 🐛 Bug: Modules in errors cause unhandledRejection, tests quit early Jan 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg added type: bug a defect, confirmed by a maintainer status: waiting for author waiting on response from OP - more information needed and removed unconfirmed-bug labels Jan 15, 2024
@JacobLey
Copy link
Contributor Author

JacobLey commented Apr 18, 2024

I've finally gotten around to trying to reproduce!

Sorry there was a minor mistake in my example. Change the code to instead be

expect({ a: modA }).to.deep.equal({ b: modB });

Now the module is not the "top level" object and experiences the issue.

So when stringify-ing the error, https://github.com/mochajs/mocha/blob/master/lib/utils.js#L242 this will attempt the "canonicalize" the top-level object.

Which internally runs this loop over the keys and tries to "implicitly stringify" the module: https://github.com/mochajs/mocha/blob/master/lib/utils.js#L408


The error didn't show up in my original example, because Mocha correctly handles "Module" in that case. Not using the suggested approach above, but handles it nonetheless.

In that case it jumps straight to the json stringification https://github.com/mochajs/mocha/blob/master/lib/utils.js#L235

Which falls back on native JSON.stringify https://github.com/mochajs/mocha/blob/master/lib/utils.js#L315 which works


Looking back at my original suggested solution, it should resolve the issue: https://github.com/mochajs/mocha/pull/5040/files

@JacobLey
Copy link
Contributor Author

JacobLey commented Apr 18, 2024

Did a little bit more research and actually think proposed solution does not go far enough...

Basically the two curveballs that are happening are:

  • Module instances have a null prototype.
    • This causes the implicit stringification to fail
  • Module instances have a custom toString implementation
    • This is what avoids the object handling

So you could actually recreate this with any instance that fulfills those requirements.

import { expect } from 'chai';
import { describe, it } from 'mocha';

describe('Compare imports', () => {
  it('different imports', async () => {

    const foo = Object.create(null, { 
      [Symbol.toStringTag]: { value: 'Foo' }, 
      bing: { get: () => 'bong', enumerable: true }
    });
    const bar = Object.create(null, { 
      [Symbol.toStringTag]: { value: 'Bar' }, 
      bing: { get: () => 'boing', enumerable: true }
    });

    // throws
    expect({ a: foo }).to.deep.equal({ b: bar });
  });
  it('always true', () => {
    console.log('This will never run');
  });
});

Similar to above, JSON.stringify() of foo and bar works as expected, so need to wrap the data in an object to trigger the failure.

Updated PR with comments as well.

@JacobLey
Copy link
Contributor Author

Easily reproducible version for anyone that wants to test out: https://github.com/JacobLey/issue-recreator/tree/mocha-esm

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue and removed status: waiting for author waiting on response from OP - more information needed labels Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in triage a maintainer should (re-)triage (review) this issue type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants