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

[JacobLey]

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]

@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.

[JacobLey]

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]

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]

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

[JoshuaKGoldberg]

Thanks, confirmed that that reproduces locally for me. 🚀

[voxpelli]

Kind of reminds me of nodejs/node#48918 (comment), but probably two different issues

[JoshuaKGoldberg]

Released in [email protected]. Thanks again @JacobLey! 🚀