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

Possible Bug with mocha new configuration file #3975

Closed
JaseP88 opened this issue Jul 26, 2019 · 27 comments · Fixed by #3983
Closed

Possible Bug with mocha new configuration file #3975

JaseP88 opened this issue Jul 26, 2019 · 27 comments · Fixed by #3983
Labels
area: integrations related to working with 3rd party software (e.g., babel, typescript) area: node.js command-line-or-Node.js-specific type: bug a defect, confirmed by a maintainer

Comments

@JaseP88
Copy link

JaseP88 commented Jul 26, 2019

Hi, I need help with an issue with mocha new configuration file. I apologize if I am going through the wrong avenue for help.
Anyways my .mocharc.js file is in the root directory of my project and its content is

'use strict';

module.exports = {
  require: ['ts-node/register', 'esm'],
  file: ['./test/testSetup.ts'],
  reporter: 'nyan',
  ui: 'bdd',
};

when i run the cli mocha "./test/**/*.spec.tsx" I get an unexpected token export in one of my node_modules dependencies

when I specify explicitly through the cli mocha -r ts-node/register -r esm --file "./test/testSetup.ts" --ui bdd "./test/**/*.spec.tsx" it is able to run the test just fine

@JaseP88 JaseP88 added the type: question support question label Jul 26, 2019
@juergba
Copy link
Member

juergba commented Jul 27, 2019

The parsing of .mocharc.js works correctly, I just checked that. The parsing result should be the same, independently of wether options have been supplied by RC file or CLI (except the reporter).

So I have no explanation why CLI should work and RC file does not.

The default extension is only set to js, you could add the extensions you need like ts /... and give it another try. I don't think this helps though.

PS: Mocha v6.2.0

@JaseP88
Copy link
Author

JaseP88 commented Jul 29, 2019

Well... I know it is able to find my mocha config file. It doesn't seem like the require flag attribute is working correctly with more than one modules.

Screen Shot 2019-07-29 at 11 59 29 AM

Screen Shot 2019-07-29 at 11 57 25 AM

I would like for the configuration file to work so I don't have to manually set the options in my IDE mocha test plugin for each spec file.

PS: I am on Mocha 6.2

@juergba
Copy link
Member

juergba commented Jul 30, 2019

In your second example: where do you set the reporter to 'nyan'?
Do you use more than one config file?

@JaseP88
Copy link
Author

JaseP88 commented Jul 30, 2019

As I mention it is able to find the config file and set a lot of the other flags just fine. I only have 1 config file in the root.

From the first post.

Anyways my .mocharc.js file is in the root directory of my project and its content is


'use strict';

module.exports = {
 require: ['ts-node/register', 'esm'],
 file: ['./test/testSetup.ts'],
 reporter: 'nyan',
 ui: 'bdd',
};

@juergba
Copy link
Member

juergba commented Jul 30, 2019

If I bother you with my questions, please just tell me.

Otherwise in node_modules\mocha\lib\cli\cli.js add in L38:
console.log(args.require)

Then please tell me what you get for CLI and config file run.

@JaseP88
Copy link
Author

JaseP88 commented Jul 30, 2019

It doesn't bother me. Sorry if I sound rude. I would like to figure this out together because there's definitely something weird going on. And I would like to utilize the configuration file, otherwise when I go debug and run individual spec and individual pieces of mocha's describe and it block I have to copy and paste required modules into my IDE.

Here is what you have asked for.

This is console log with explicit cli

Screen Shot 2019-07-30 at 12 16 26 PM

This is console log with config file.

Screen Shot 2019-07-30 at 12 11 03 PM

@juergba
Copy link
Member

juergba commented Jul 30, 2019

Thank you.

The parsing result of --require is identical both ways.
The require modules are loaded here: lib/cli/run-helpers.js.
Anyway require(modpath); throws an error when the module is not found.

So I have no explanation for now. It can't be the parsing since there is no difference.
I know Mocha is not ready for esm, unfortunately I have no experience with esm.
But since it's working one way (CLI), it should work the other way (RC file) as well.

@JaseP88
Copy link
Author

JaseP88 commented Jul 30, 2019

Hrmm thank you for your help. I'll see if I can find any work around.

@mobalt
Copy link

mobalt commented Aug 4, 2019

Odd. Loading esm via args must be done exactly one way:

mocha --require esm works
mocha --require=esm does not work

The equal sign shouldn't be a problem because yargs is able to parse them both into exactly the same result, but mocha must be doing it's own process.argv checking and butchering/overriding yargs output args object.

@mobalt
Copy link

mobalt commented Aug 4, 2019

Here's another gem: If you specify require: esm somewhere in a config file (package.json, mocharc.yml, ...), having the standalone word "esm" present anywhere in the CLI args will be enough for it to work. For example:

mocha                           # doesn't work, but it should as reported above @JaseP88
mocha --require esm            # works, obviously. (even without setting config)
mocha --fgrep esm              # weird! this also works now?
mocha esm                      # it still works! (but with the warning below)

Warning: Cannot find any files matching pattern "esm"

... Mocha is definitely doing some funky with process.argv internally.

@JaseP88
Copy link
Author

JaseP88 commented Aug 4, 2019

That is some excellent detective work mobalt!

@flinders-corey
Copy link

Hi,

I had a similar issue to this happen to me earlier today.

Using version 6.1.4 I was able to use mocha with esm with the following sections in my package.json file:

"scripts": {
  "test": "mocha"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

Post upgrade to mocha 6.2.0 this did not work. The esm module wasn't being called. I was getting the dreaded SyntaxError: Unexpected token { error for my import statement.

I needed to change my package.json file so that esm was explicitly called via the command line. For example:

"scripts": {
  "test": "mocha --require esm"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

I'm not sure if these two issues are related or not. As I found this thread via a Google search to work out what was wrong, I thought it worth mentioning.

@juergba
Copy link
Member

juergba commented Aug 7, 2019

@flinders-corey could you run one test for me, please?

In your package.json file replace --require esm by a node flag like eg. --no-deprecation.
For this test it does not really matter which node flag you choose. Now mocha should run in a child process, not in the main process. I wonder wether this could make a difference.

"scripts": {
  "test": "mocha --no-deprecation"
},
[...]

@mobalt
Copy link

mobalt commented Aug 7, 2019

@juergba that fixed it for me. A possible hotfix would be to temporarily revert #3827 so that all processes launched from CLI load in a child process.

@mobalt
Copy link

mobalt commented Aug 7, 2019

My theory for a longterm fix has to do with at what point the mocha function handleRequires starts loading esm. esm overrides node's require function to extend functionality. Since mocha uses node's built-in requireto load test files, if esm is loaded early enough, mocha will use esm's require when loading the files. If however, esm is loaded too late, then mocha will use the built-in require, and which will throw the SyntaxError: Unexpected token { when trying to require test files with import/export syntax.

yargs must be running .check() sooner on process.argv's args vs those passed in via yargs.config(). Since handleRequires is called in the .check validation (below)

handleRequires(argv.require);

then this is impacting which require is being used by mocha, esm or the builtin version. So for example if there was a way to change the require used in L334

mocha/lib/mocha.js

Lines 315 to 338 in 9d3c584

/**
* Loads `files` prior to execution.
*
* @description
* The implementation relies on Node's `require` to execute
* the test interface functions and will be subject to its cache.
*
* @private
* @see {@link Mocha#addFile}
* @see {@link Mocha#run}
* @see {@link Mocha#unloadFiles}
* @param {Function} [fn] - Callback invoked upon completion.
*/
Mocha.prototype.loadFiles = function(fn) {
var self = this;
var suite = this.suite;
this.files.forEach(function(file) {
file = path.resolve(file);
suite.emit(EVENT_FILE_PRE_REQUIRE, global, file, self);
suite.emit(EVENT_FILE_REQUIRE, require(file), file, self);
suite.emit(EVENT_FILE_POST_REQUIRE, global, file, self);
});
fn && fn();
};

mocha.js:L334

require = require("esm")(module)
// now no error will be thrown when loading index.spec.js
suite.emit(EVENT_FILE_REQUIRE, require(file), file, self);

index.spec.js

// ESM syntax is supported.
export {}

But this seems hacky, replacing require after the fact. So once again, the easiest fix is to just require("esm") before mocha/lib/mocha.js is even loaded so that it won't hold on to a reference of the old builtin require rather it'll inherit the new one.

@flinders-corey
Copy link

@flinders-corey could you run one test for me, please?

In your package.json file replace --require esm by a node flag like eg. --no-deprecation.
For this test it does not really matter which node flag you choose. Now mocha should run in a child process, not in the main process. I wonder wether this could make a difference.

"scripts": {
  "test": "mocha --no-deprecation"
},
[...]

Hi @juergba,

Sure, happy to test.

Mocha runs if I have this in my package.json file:

"scripts": {
  "test": "mocha --require esm"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

Mocha fails to run if I have this in my package.json file:

"scripts": {
  "test": "mocha"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

Interestingly it runs successfully if I have this:

"scripts": {
  "test": "mocha --no-deprecation"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

Hope this helps.

@juergba
Copy link
Member

juergba commented Aug 8, 2019

Thank you @flinders-corey @mobalt

I need to understand first why it works via CLI and - with the identical parsing result - does not work via configuration file.

It's not yargs which does the CLI and config file parsing, this job is done earlier by yargs-parser in "lib/cli/options.js".

@boneskull
Copy link
Member

boneskull commented Aug 8, 2019

@juergba that fixed it for me. A possible hotfix would be to temporarily revert #3827 so that all processes launched from CLI load in a child process.

this might be the best course of action if it's fixing people. I'm unable to reproduce this in a way that makes much sense.

this is what I've tried:

  1. create package dir mkdir /tmp/foo && cd /tmp/foo

  2. create new package (npm init -y)

  3. npm i mocha esm

  4. create test file:

    // test.js 
    import {ok} from 'assert';
    describe('fixture', function() {
      it('should be true', function() {
        ok(true);
      });
    });
  5. run node_modules/.bin/mocha -r esm test.js; success

now, back in my mochajs/mocha working copy:

  1. $ git checkout v6.2.0
    $ npm i
    
  2. cd /tmp/foo
  3. /path/to/mochajs/mocha/bin/mocha -r esm test.js; fails w/ "Unexpected Token" SyntaxError

The above only works if I fix a bug; module.paths is being set in (inexplicably) two places (cli.js and lib/mocha.js) and not doing much of anything. moving the call in cli.js to within the handleRequires function means I can require a local esm while running mocha from an arbitrary location.

anyway--until we track this down, we should revert that change

@juergba
Copy link
Member

juergba commented Aug 9, 2019

I'm starting to doubt on this esm module.

When I change "bin/_mocha" as proposed by @jdalton in #3703 ==> it works.

When I change "bin/mocha" the same way on L13 / L156: ==> it fails

L13 : const esmRequire = require('esm')(module);
...
L155: } else {
      esmRequire('../lib/cli/cli').main(unparse(mochaArgs, {alias: aliases}));
}

@juergba
Copy link
Member

juergba commented Aug 9, 2019

@boneskull thank you for your input.

Can I propose an alternative, softer revert. We spawn a child-process if:

  • either Node flags
  • or --require esm option (via CLI or RC file)

In bin/mocha:
if (Object.keys(nodeArgs).length || mochaArgs.require && mochaArgs.require.includes('esm')) {

EDIT: --require is also a node flag, so we could put --require esm directly into nodeArgs.

@juergba
Copy link
Member

juergba commented Aug 9, 2019

When I change "bin/mocha" the same way on L13 / L156: ==> it fails

When I add on L13 following line: ==> it works without spawning a child-process
require = require('esm')(module);

My conclusion is: esm can't be loaded just somewhere in middle of mocha's code. If there already have been calls to the native node require, esm fails unless we spawn a child-process.

@juergba juergba added type: bug a defect, confirmed by a maintainer area: integrations related to working with 3rd party software (e.g., babel, typescript) area: node.js command-line-or-Node.js-specific and removed type: question support question labels Aug 10, 2019
@craigtaub
Copy link
Contributor

craigtaub commented Aug 10, 2019

My conclusion is: esm can't be loaded just somewhere in middle of mocha's code.

Hmm but surely child process executes the requires inside at top of cli.js + run.js before it finally requires esm via handleRequires (i.e. child or main would have same problem)? 🤷‍♂

@juergba
Copy link
Member

juergba commented Aug 11, 2019

@craigtaub
When I debug this weird case, I can see that esm is loaded in the main process, since there is an entry in require.cache. It's not loaded in the main module, so probably too late and the main process fails. When a child-process is spawned, the environment is inherited and esm is already loaded. The child runs successfully without depending on the require esm in handleRequires.

Meanwhile I think it's a bad hack to manipulate Node out of running Mocha.
We should treat --require esm as a Node flag and remove any esm loading out of Mocha.
node --require esm mocha
This way Node has been extended with esm before Mocha is launched (in a child process) and we are on the safe side.

@craigtaub
Copy link
Contributor

craigtaub commented Aug 11, 2019

@juergba

It's not loaded in the main module, so probably too late and the main process fails.

This confused me. In my debugging I can see an esm entry inside require.cache at beginning of loadFiles (i.e. before requiring in users test spec). Was unsure what was meant by "main module".
Chalking it down to quirk of esm so happy to accept letting node handle it despite not completely understanding why.

@juergba
Copy link
Member

juergba commented Aug 12, 2019

@JaseP88 @flinders-corey Do you have some time left?
If yes, please patch "bin/mocha" as per #3983 and tell me wether your case is working correctly?
Thank you.

@JaseP88
Copy link
Author

JaseP88 commented Aug 12, 2019

@juergba patching bin/mocha with that solution works. Thank you!

@flinders-corey
Copy link

flinders-corey commented Aug 13, 2019

@juergba I can confirm that with the patch applied as per #3983 my use case works. Specifically using the following in my package.json file.

"scripts": {
  "test": "mocha"
},
"mocha": {
  "require": [
    "esm"
  ],
  "recursive": true
}

My thanks to you and the team for working on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: integrations related to working with 3rd party software (e.g., babel, typescript) area: node.js command-line-or-Node.js-specific type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants