fix(esm): allow import from mocha in parallel

[nicojs]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

Fixes the issue where import { describe, it } from 'mocha' doesn't work with --parallel when using es modules by proxying describe, it and friends to their actual contextual counterpart.

Alternate Designs

There is not much else we can do I think, other than going full blown esm (for example, using package exports).

Why should this be in core?

Its imported from core (bare 'mocha').

Benefits

Allows import {...} from 'mocha' in an es module setting with parallel

Possible Drawbacks

More code to maintain

Applicable issues

Closes #4559, should be a patch release.

[nicojs]

Of course, my new test is failing on node 10 😇 Will have a look at that soon (probably need to enable --experimental-modules)

[nicojs]

I ended up skipping the tests on node 10. Importing esm style from a cjs module is supported from node 12.

[juergba]

@nicojs Are describe, it, ... still on global scope with this solution?

import { describe, it } from 'mocha': we don't need an ESM entry point anymore to achieve this?

[giltayar]

@juergba Oh, I didn't notice. You're right!

This is rich! What happened is interesting. Node.js has this mechanism when you import a CJS file in that it parses it and tries to determine what the named exports are. It uses a very basic parser that looks for standard patterns like exports.abc = ... and others.

What happened was that previously, that parser chocked on the mocha entry point, and defaulted to only default exports. But @nicojs's changes are making that parser very happy, so suddenly it's OK, and mocha is doing named exports from CJS.

Note that my PR is still better because each of the entry points ('/tdd, '/qunit...) exports only the correct test functions per-interface, and does it without the CJS parsing thing (slightly faster), whereas this PR exports them ALL.

But it's definitely progress!

[nicojs]

Yeah, I think what was happening is that when you import {describe, it} from mocha the first time, the ESM - CJS interop layer retrieves the named exports, and caches them in ESM land. This is why, in parallel mode, the first job works and the second one does not, because the tests get added to the old suite (cached describe and it).

This PR fixes that, because it allows describe and it to be cached forever. They will never change since they just proxy to whatever suite is loading at that moment in time. I was actually surprised to see this wasn't done already.

[juergba]

@giltayar can you build your ESM entry points on top of this PR? Or does it prevent it in anyway?

[...] whereas this PR exports them ALL

Do we have any negative implications on that?

@nicojs could you have a short look on our watcher, please? I remember difficulties with the UI loading. Maybe we can simplify some coding?

[giltayar]

@juergba of course. Once this lands, my PR will return to what it was initially: making Mocha a dual-mode library with real ESM entry points that align with the stamdard interfaces. I will remove all the crud from there that pertains to fixing the problem that this PR fixed (much more elegantly).

But that PR now becomes less urgent, and is now a PR that just fine tunes the entry points to be more "correct" and not exposes all test functions of all the interfaces, but exposes entry points for each interface that expose only the functions for that entry point. The main entry point will still expose everything, for backward compatibility.

[juergba]

It would be awesome to get some feedback regarding the watcher and especially lib/browser-entry.js .
Our browser tests are weak and I want to make sure to not add any regressions with this PR. Thank you.

[nicojs]

Hi @juergba 👋, I've taken a look.

• browser-entry.js: wow I haven't taken a proper look at that file yet. It has some hacks (no offense whatsoever),
  • It overrides mocha.ui and forces a emit of pre-require.

    mocha.ui = function(ui) {
     Mocha.prototype.ui.call(this, ui);
     this.suite.emit('pre-require', global, null, this);
     return this;
    };

    This allowed the beforeEach, describe etc to be exported in the previous setup. With this new code change, it still works the same way, except there is the proxy function in between, i.e. these functions are now cacheable.
  • However, I think I did break something for both browser and non-browser environments:

  $ git checkout fix/import-parallel
  $ npm run start -- build
  $ node -p 'require("./mocha").setup("bdd").Mocha.describe'
   [Function]
  $ git checkout master
  $ npm run start -- build
  $ node -p 'require("./mocha").setup("bdd").Mocha.describe'
  [Function] { skip: [Function], only: [Function] }

  So apparently the old describe, it etc have functions that are no longer exposed with this PR. We will need to fix that! Is there a way to see an extensive list of these functions?
• lib/cli/watch-run.js: The way I understand the watch is that this feature is only supported for commonjs scenarios (there is currently no way to clear from the ESM cache, right @giltayar ?). They should still work with the proxying mechanism because test files are cleared from the require cache and then reloaded. If they use const {describe} = require('mocha') syntax, they should still be allowed to do so, at least, that's what I can deduct from this comment:

    // this `require` is needed because the require cache has been cleared.  the dynamic
    // exports set via the below call to `mocha.ui()` won't work properly if a
    // test depends on this module (see `required-tokens.spec.js`).
    const Mocha = require('../mocha');

[nicojs]

Don't merge this PR yet! edit: since then fixed

[nicojs]

Ok, I think I've added proxying of describe.only and all of its friends. I also added a test for it. I think I've got everything covered except for it.retries and describe.retries. I'm not familiar with that part of our API. @juergba should those also be covered? What .retries mean in this context?

Looking at https://mochajs.org/#retry-tests I can only find this.retries.

[juergba]

@nicojs thank you for your comments. I try to answer some of your questions.

I understand now that we are only talking about the require interface.
The keywords describe etc. will remain in global scope and therefore we don't put at risk neither our browser nor watcher nor parallel workers.

lib/cli/watch-run.js: yes, our watcher is limited to CJS, we have documented that somewhere. Nevertheless some users run ESM in parallel mode, which seems to work. The ESM cache is "cleared" by killing the worker processes on each re-run.

retries(n): has no effect on a passing test. A failing test is re-run max. n times, until it passes or n is reached. In lib/interfaces/bdd.js you see that it.retries exists, but describe.retries does not. I have never used it.retries() yet.

I will need more time to review/test your recent additions. The topic is still fuzzy to me. Where does this context object comes from? IMO we don't need to export each and every alias, eg. describe yes, but it's alias context no.

[nicojs]

Thanks for your explanation @juergba

I've tried to add it.retries to the test but it fails because the context seems to always be undefined. This already was the case before this change, so I don't think it ever worked with cjs require syntax,

Where does this context object comes from? IMO we don't need to export each and every alias, eg. describe yes, but it's alias context no.

What do you mean by context? This PR keeps a local variable called currentContext, which is the current mocha context object for the file that is being loaded, but it doesn't export that.

I agree that not every alias needs exporting. Users who are using the "bdd" UI probably don't want suite and test. However, I don't think we can do anything about it without bigger changes. The way I understand it is that, for the cjs-esm bridge to work, your exports should be cacheable. Since one process could run mocha once for a "bdd" project and once for a "tdd" project in the same process (using the programmatic API), I don't see any other way.

One thing we could do is export suite/test from 'mocha/tdd' and export describe/it from 'mocha/bdd', something like that. But that would mean a bigger (breaking?) change.

[juergba]

@nicojs when I test:

const ste = require('mocha').describe;

ste('test suite', () => {
    it('test', () => {
        console.log('running the test');
    });
});

In Mocha v.8.3.0 the test passes, with this branch the result is TypeError: ste is not a function.
The same with import {describe,it} from "mocha";. v8.3.0. is working and this branch fails.
(this synthax already works in v8.3.0 @giltayar 😵😈)

Am I doing something wrong? Or do your test use the global keywords, not the required/imported ones?

[juergba]

Some additional information:

• I installed this branch locally with npm install https://github.com/mochajs/mocha#2b608c31: above test runs succcessfully.
• this branch in my local git clone: I changed to const ste = require('./mocha/lib/mocha').describe; and test also passes.
• test/integration/fixtures/common-js-require.fixture.js: this test fails in local git clone

    1) Uncaught error outside test suite:
         Uncaught TypeError: Cannot read property 'apply' of undefined

[juergba]

I've tried to add it.retries [...]: I agree, doesn't seem to work, so we just ignore it as before.

I suggest adding xdescribe, since we are already exporting xit.

Exporting named functions eg. exports.describe = function describe(...args) { would make debugging easier.

[giltayar]

@juergba the error in test/integration/fixtures/common-js-require.fixture.js is per-spec. The test in that fixture has a delayed run and according to https://mochajs.org/#delayed-root-suite, you need to add a --delay. Adding that option when running the test removed the failure. Not sure,BTW, why that option is needed. It is used in mocha.js, in this line of the prerequire hook:

    context.run = mocha.options.delay && common.runWithSuite(suite);

If I remove the mocha.options.delay &&, everything works without the --delay flag.

This could be something that was introduced because there was no proxy? As a hack? Something that maybe isn't needed anymore?

[nicojs]

The reason why I've added --delay was so I could test that exporting run would also work. The test is now testing every single export. I've added a remark in a comment to make this more clear.

I suggest adding xdescribe, since we are already exporting xit.

I did that now. Note that this is technically a new feature since it wasn't exported before.