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

Make unittests pass with "npm run coverage" on Node 14 #1593

Closed
smcclure15 opened this issue Apr 15, 2021 · 1 comment · Fixed by #1594
Closed

Make unittests pass with "npm run coverage" on Node 14 #1593

smcclure15 opened this issue Apr 15, 2021 · 1 comment · Fixed by #1594
Assignees
Labels
Component: CLI Type: Bug Something isn't working right. Type: Meta Seek input from maintainers and contributors.

Comments

@smcclure15
Copy link
Member

I'm on Node 14.15.0 (on Mac), running against QUnit 2.15.0 source.

npm test runs cleanly, but npm run coverage reveals two failures in the CLI tests:

not ok 19 CLI Main > run ESM test suite with import statement
  ---
  message: "Promise rejected during "run ESM test suite with import statement": Command failed: /bin/sh -c ../../../bin/qunit.js ../../es2018/esm.mjs

TAP version 13
not ok 1 ../../es2018/esm.mjs > Failed to load the test file with error:
/Users/stevemcclure/Documents/GitHub/qunit/test/es2018/esm.mjs:1
import sum from "./sum.mjs";
^^^^^^

SyntaxError: Cannot use import statement outside a module

and

not ok 21 CLI Main > mapped trace with native source map
  ---
  message: "failed"
  severity: failed
  actual  : "TAP version 13\nok 1 Example > good\nnot ok 2 Example > bad\n  ---\n  message: \"failed\"\n  severity: failed\n  actual  : false\n  expected: true\n  stack:     at Assert.pushResult (/qunit/qunit/qunit.js)\n  ...\n1..2\n# pass 1\n# skip 0\n# todo 0\n# fail 1"
  expected: "TAP version 13\nok 1 Example > good\nnot ok 2 Example > bad\n  ---\n  message: \"failed\"\n  severity: failed\n  actual  : false\n  expected: true\n  stack:     at Object.<anonymous> (/qunit/test/cli/fixtures/sourcemap/sourcemap/source.js:7:10)\n  ...\n1..2\n# pass 1\n# skip 0\n# todo 0\n# fail 1"
  stack:     at Object.<anonymous> (/Users/stevemcclure/Documents/GitHub/qunit/test/cli/main.js:175:12)

Still not sure if it's just a test fix (stdout formatting could be different), or there's any latent bugs here.

@smcclure15 smcclure15 added the Type: Meta Seek input from maintainers and contributors. label Apr 15, 2021
@smcclure15 smcclure15 self-assigned this Apr 15, 2021
Krinkle pushed a commit that referenced this issue Apr 15, 2021
Also skip the sourcemap test under coverage mode.

Fixes #1593.
Closes #1594.
@Krinkle
Copy link
Member

Krinkle commented Apr 15, 2021

Disabling the sourcemap test under coverage mode makes sense indeed. LGTM. I hadn't faced it yet since we use Node 10 LTS for the version-agnostic coverage run in CI, and me locally as well.

The syntax error is an interesting one. It seems that may've changed in upstream Node.js 14 within a minor version, due to changes in V8? In any event, I can reproduce it and confirm that Node's own upstream tests check for this the same way, so LGTM on both accounts.

@Krinkle Krinkle added Type: Bug Something isn't working right. Component: CLI labels Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CLI Type: Bug Something isn't working right. Type: Meta Seek input from maintainers and contributors.
Development

Successfully merging a pull request may close this issue.

2 participants