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

nyc + esm is broken in latest NodeJS versions #1530

Open
1 task done
jeremymeng opened this issue Sep 14, 2023 · 17 comments
Open
1 task done

nyc + esm is broken in latest NodeJS versions #1530

jeremymeng opened this issue Sep 14, 2023 · 17 comments

Comments

@jeremymeng
Copy link

nyc is not reporting code coverage results after possibly related nodejs change nodejs/node@15bced0bde

Link to bug demonstration repository

https://github.com/jeremymeng/nyc-repro

install package then run npm run cc

Expected Behavior

(same as in nodejs 18.17.0)

  basic test
    ✓ add correctly


  1 passing (4ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |
 index.js |     100 |      100 |     100 |     100 |
----------|---------|----------|---------|---------|-------------------

Observed Behavior

a warning followed by passing test but 0% coverage

Transformation error for /home/meng/git/nyc-repro/src/index.js ; return original code
The "mod" argument must be an instance of Module. Received an instance of Module


  basic test
    ✓ add correctly


  1 passing (3ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
----------|---------|----------|---------|---------|-------------------

Troubleshooting steps

  • still occurring when I put cache: false in my nyc config

I added a rimraf command to remve the cache directory

Environment Information

  System:
    OS: Linux 6.4 Arch Linux
    CPU: (8) x64 Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
    Memory: 23.40 GB / 31.11 GB
  Binaries:
    Node: 18.17.1 - ~/.nvm/versions/node/v18.17.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.17.1/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.1/bin/npm
  npmPackages:
    nyc: ^15.1.0 => 15.1.0
@jeremymeng
Copy link
Author

I wonder whether it's related to #1528

jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this issue Sep 14, 2023
Latest NodeJS versions broke nyc. This PR disables it temporarily.

istanbuljs/nyc#1530

An workitem is logged to track re-enabling it after the issue is resolved.

Azure#27128
jeremymeng added a commit to Azure/azure-sdk-for-js that referenced this issue Sep 15, 2023
Latest NodeJS versions broke nyc. This PR disables it temporarily.

istanbuljs/nyc#1530

An workitem is logged to track re-enabling it after the issue is
resolved.

#27128
@jeremymeng
Copy link
Author

/cc @RafaelGSS who signed off on nodejs/node@15bced0bde. Any insights?

@RafaelGSS
Copy link

This change should only have effect when --experimental-policy is enabled. Are you sure reverting this commit nyc works?

/cc @bmeck

@jeremymeng
Copy link
Author

@RafaelGSS nyc works fine in previous NodeJS build 18.17.0. The error is thrown by the new code added in the commit.

@jeremymeng
Copy link
Author

15bced0bde seems the only related commit in 18.17.1 https://nodejs.org/en/blog/release/v18.17.1#commits

Commits
[fe3abdf82e] - deps: update archs files for openssl-3.0.10+quic1 (Node.js GitHub Bot) #49036
[2c5a522d9c] - deps: upgrade openssl sources to quictls/openssl-3.0.10+quic1 (Node.js GitHub Bot) #49036
[15bced0bde] - policy: handle Module.constructor and main.extensions bypass (RafaelGSS) nodejs-private/node-private#417
[d4570fae35] - policy: disable process.binding() when enabled (Tobias Nießen) nodejs-private/node-private#460

@RafaelGSS
Copy link

This was a security release. I'm travelling at the moment but looks like nyc is creating Module in a non-conventional way. I will need to check.

@RafaelGSS
Copy link

Can you create a reproducible example? I can provide a fix very quickly.

@jeremymeng
Copy link
Author

@RafaelGSS yeah I have it in the issue description https://github.com/jeremymeng/nyc-repro

@jeremymeng
Copy link
Author

@RafaelGSS Any updates?

@RafaelGSS
Copy link

@RafaelGSS yeah I have it in the issue description https://github.com/jeremymeng/nyc-repro

Private repository

@jeremymeng
Copy link
Author

Private repository

Oops sorry. never meant to make it private. Fixed now.

@RafaelGSS
Copy link

I looked at that now and it's not a nyc bug, but a esm one. Diff https://gist.github.com/RafaelGSS/e08193f1328bdbd18ee4d340b6fb9041

nyc-repro git:(main) ✗ git remote -v
origin  [email protected]:jeremymeng/nyc-repro.git (fetch)
origin  [email protected]:jeremymeng/nyc-repro.git (push)nyc-repro git:(main) ✗ git rev-parse HEAD
48d25f8b353e483833fbf37489dcc4fae14651efnyc-repro git:(main) ✗ curl https://gist.githubusercontent.com/RafaelGSS/e08193f1328bdbd18ee4d340b6fb9041/raw/0f00994a06d3204e229797ecb8d565a7215ab4b5/gistfile0.txt | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1009  100  1009    0     0   5347      0 --:--:-- --:--:-- --:--:--  5513nyc-repro git:(main) ✗ node -v
v20.8.1nyc-repro git:(main) ✗ npm run cc

> [email protected] cc
> rimraf ./node_modules/.cache && nyc mocha test/**/*.spec.js



  basic test
    ✓ add correctly


  1 passing (2ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |
 index.js |     100 |      100 |     100 |     100 |
----------|---------|----------|---------|---------|-------------------nyc-repro git:(main) ✗ git reset --hard
HEAD is now at 48d25f8 add readmenyc-repro git:(main) ✗ npm run cc

> [email protected] cc
> rimraf ./node_modules/.cache && nyc mocha -r esm test/**/*.spec.js

Transformation error for /Users/rafaelgss/repos/os/nyc-repro/src/index.js ; return original code
The "mod" argument must be an instance of Module. Received an instance of Module


  basic test
    ✓ add correctly


  1 passing (1ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
----------|---------|----------|---------|---------|-------------------

@jeremymeng
Copy link
Author

@RafaelGSS It must have something to do with nyc and esm together, because running the test directly there's no error

I added the following command to the repro

    "test": "rimraf ./node_modules/.cache && mocha -r esm test/**/*.spec.js",

jeremymeng added a commit to Azure/azure-sdk-for-js that referenced this issue Oct 23, 2023
***NO_CI***

The combination of `nyc` + `esm` is broken in latest versions of
NodeJS (istanbuljs/nyc#1530 (comment)).
Since `esm` package is not actively maintained and its repo is archived, it's
less likely that the issue will be fixed.

This PR switches to use another code coverage tool `c8` which is not affected.
`c8` also respects `.nycrc` config files so those are not renamed in this PR.
jeremymeng added a commit to Azure/azure-sdk-for-js that referenced this issue Oct 23, 2023
***NO_CI***

The combination of `nyc` + `esm` is broken in latest versions of
NodeJS (istanbuljs/nyc#1530 (comment)).
Since `esm` package is no longer actively maintained and its repo is archived,
it's less likely that the issue will be fixed soon.

This change switches to use another code coverage tool `c8` which is not
affected. `c8` respects `.nycrc` config files so those are not renamed.
jeremymeng added a commit to Azure/azure-sdk-for-js that referenced this issue Oct 23, 2023
***NO_CI***

The combination of `nyc` + `esm` is broken in latest versions of
NodeJS (istanbuljs/nyc#1530 (comment)).
Since `esm` package is no longer actively maintained and its repo is archived,
it's less likely that the issue will be fixed soon.

This change switches to use another code coverage tool `c8` which is not
affected. `c8` respects `.nycrc` config files so those are not renamed.
benbp pushed a commit to Azure/azure-sdk-for-js that referenced this issue Oct 23, 2023
***NO_CI***

The combination of `nyc` + `esm` is broken in latest versions of
NodeJS (istanbuljs/nyc#1530 (comment)).
Since `esm` package is no longer actively maintained and its repo is archived,
it's less likely that the issue will be fixed soon.

This change switches to use another code coverage tool `c8` which is not
affected. `c8` respects `.nycrc` config files so those are not renamed.
@jeremymeng jeremymeng changed the title nyc is broken in latest NodeJS versions nyc + esm is broken in latest NodeJS versions Oct 24, 2023
@Sandy8i
Copy link

Sandy8i commented Feb 13, 2024

I am still facing the issue with node 20. Do you have any updates, please? Or any workarounds?

@jeremymeng
Copy link
Author

@Sandy8i we switched to c8 for now. It still supports .nycrc. Ideally we'd like to continue using nyc + esm

@Sandy8i
Copy link

Sandy8i commented Feb 13, 2024

@Sandy8i we switched to c8 for now. It still supports .nycrc. Ideally we'd like to continue using nyc + esm

Thank you for the quick response. I am actually struggling a little bit with c8. I am on node 20, the latest esm, and the latest c8. When I run my unit tests, I get the below error. Can you please help me identify the correct workspace to discuss the issue?

/webui/interface.js:55
response?.status === 401 &&
^

SyntaxError: Invalid or unexpected token
at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)

Thanks again.

@jeremymeng
Copy link
Author

@Sandy8i that's a known esm issue. Some people switched to use a fork that supports newer EcmaScript syntax standard-things/esm#866 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants