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

run tests es6 modules In CLI #1465

Closed
alexeyp0708 opened this issue Aug 2, 2020 · 1 comment
Closed

run tests es6 modules In CLI #1465

alexeyp0708 opened this issue Aug 2, 2020 · 1 comment
Assignees
Labels
Component: CLI help welcome Status: Ready A "Meta" type issue that has reached consensus. Type: Enhancement New idea or feature request.
Milestone

Comments

@alexeyp0708
Copy link

alexeyp0708 commented Aug 2, 2020

  • Qunit 2.10.1:
  • Node v14.5.0:

running es6 module tests via terminal (CLI)

// tests/qunit/test.js
import {Object} from "../../src/export.js";
// Tests
//...
//...

package.json

{

	"name": "Test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "devDependencies": {
	"qunit":"^2.10.1"
  }
}

run cli command
$ npx qunit tests/qunit/test*.*js

Result :

not ok 1 tests/qunit/test.js > Failed to load the test file with error:
.\tests\qunit\test.js:1
import {Argv} from "../../src/export.js";
^^^^^^

SyntaxError: Cannot use import statement outside a module

Оr

package.json

{

	"name": "Test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "type":"module",
  "devDependencies": {
	"qunit":"^2.10.1"
  }
}

run cli command
$ npx qunit tests/qunit/test*.*js `

Result

not ok 1 tests/qunit/test.js > Failed to load the test file with error:
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: 
./tests/qunit/test.js
require() of ES modules is not supported.
require() of ./tests/qunit/test.js from ./node_modules/qunit/src/cli/run.js is an ES module file as it is a .js file whose nearest parent package.json
contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename test.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from ./package.json.

Fix

To fix the situation of dynamic loading of ES6 modules in the CommonJS context, dynamic loading must is performed using the import () function.

In order to fix this problem, I wrote my own test runner qunit - js_qunit_run_module_es6
To fix the problem, evaluate the fixes from line 49 in the file src/cli/run.js , and make the appropriate edits to your code.

Since you are using Eslint, I will not be able to offer you a pull request with fixes as your tests will not be able to run due to innovations (conflict from Eslint). Make corrections yourself.

For migration to ES6, you can create a separate run script. For example qunit -m tests/test*.js or qunit --module tests/test*.js

@Krinkle Krinkle added Component: CLI Type: Meta Seek input from maintainers and contributors. labels Aug 4, 2020
@Krinkle
Copy link
Member

Krinkle commented Nov 7, 2020

I'd like for people to able to use .mjs and .js files together, and without any special options. It should just work when you run qunit test/, qunit test/*, qunit test/*.{mjs,js} or qunit test/foo.mjs.

I've spent some time reading the Node.js docs on ESM imports and CJS imports. If I understand correctly:

  • require() is only for CommonJS files, but the function is available in both ESM and CJS contexts.
  • import statements can handle both CJS and ESM files, but if used for a CJS file, it may look a little different due to the default export caveat, so blindly switching from require() to import can sometimes break, but might work for us here since test suites don't actually need to export anything. But.. import statements only work from an ESM file, which the QUnit CLI is not (yet) using.
  • The dynamic import() function is availble in both CJS and ESM contexts, and it can presumably handle both types of files as well, with the same default-export caveat that we don't care about.

Based on this, it seems like simply using the import() function instead of require() might be all we need to do?

But, looking at how other frameworks handle this side-by-side, it seems they do it differently. For example, Mocha (source) checks the file extension and calls either import() or require() accordingly. I probably missed something that explains why that would be needed. It's not an issue either way of course, it's simple enough. I suspect it might have to do with supporting older Node.js versions.

@Krinkle Krinkle added help welcome Status: Ready A "Meta" type issue that has reached consensus. Type: Enhancement New idea or feature request. and removed Type: Meta Seek input from maintainers and contributors. labels Nov 7, 2020
@Krinkle Krinkle modified the milestones: 3.0, 3.0 release, 2.x release Nov 7, 2020
@Krinkle Krinkle self-assigned this Nov 13, 2020
Krinkle added a commit that referenced this issue Nov 17, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 17, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 28, 2020
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Closes #1510.
@Krinkle Krinkle closed this as completed Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CLI help welcome Status: Ready A "Meta" type issue that has reached consensus. Type: Enhancement New idea or feature request.
Development

No branches or pull requests

2 participants