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

Tests are not running anymore with C8 on node 18.11 #45013

Closed
w3nl opened this issue Oct 15, 2022 · 20 comments · Fixed by #45055
Closed

Tests are not running anymore with C8 on node 18.11 #45013

w3nl opened this issue Oct 15, 2022 · 20 comments · Fixed by #45055
Labels
coverage Issues and PRs related to native coverage support. test_runner

Comments

@w3nl
Copy link

w3nl commented Oct 15, 2022

Version

18.11.0

Platform

Darwin CLC02DRB4AMD6R 20.6.0 Darwin Kernel Version 20.6.0: Mon Aug 29 04:31:06 PDT 2022; root:xnu-7195.141.39~2/RELEASE_X86_64 x86_64

Subsystem

Mac OS 11.7, zsh 5.8

What steps will reproduce the bug?

When i run node --test src/ my tests are running.
When i use C8 c8 node --test src/, no tests are running, and no coverage.

And I see an error:

ERROR: Coverage for lines (0%) does not meet global threshold (90%)
zsh: segmentation fault  c8 node --test src/

.nycrc

{
  "all": true,
  "include": [
    "src/**/*.js"
  ],
  "exclude": [
    "**/*.test.js",
    "**/__fixtures__/**"
  ],
  "reporter": [
    "text",
    "text-summary",
    "lcov",
    "clover"
  ],
  "check-coverage": true,
  "watermarks": {
    "lines": [
      80,
      95
    ],
    "functions": [
      80,
      95
    ],
    "branches": [
      80,
      95
    ],
    "statements": [
      80,
      95
    ]
  }
}

Tested with node 18.10.0, and it was working

Has node 18.11.0 a breaking change or bug, or is it a C8 issue?

How often does it reproduce? Is there a required condition?

Only when combined with C8.
Doesnt work anymore since node 18.11.0

What is the expected behavior?

Like node 18.10.0, run the tests

What do you see instead?

ERROR: Coverage for lines (0%) does not meet global threshold (90%)
zsh: segmentation fault c8 node --test src/

Additional information

It looks like a breaking change in node 18.11.0, or is it a bug in c8?

@Trott
Copy link
Member

Trott commented Oct 15, 2022

I'm on macOS 12.6 and not having a problem with c8@7.12.0 with Node.js 18.11.0. What version of c8 are you running?

@bcoe Any suggestions for how @w3nl might determine if this is a Node.js bug, a c8 bug, or something else?

@w3nl
Copy link
Author

w3nl commented Oct 15, 2022

Latest version of c8: 7.12

@w3nl
Copy link
Author

w3nl commented Oct 15, 2022

@Trott Here a test with a package in Github actions: https://github.com/hckrnews/objects/actions/runs/3255633842/jobs/5345156351

Segmentation fault (core dumped)
Error: Process completed with exit code 139.

Schermafbeelding 2022-10-15 om 14 12 19

@Trott
Copy link
Member

Trott commented Oct 15, 2022

@Trott Here a test with a package in Github actions:

That's running on Ubuntu Linux, but you reported macOS in the Platform information in the original post. Are you seeing the problem on macOS too or just on Linux?

@w3nl
Copy link
Author

w3nl commented Oct 15, 2022

On both platforms, on my local Mac and also in Linux in the GitHub action.
So it's not a local issue that only exists on my machine.
So it can be node, c8 or my config the project.

FYI Originally found in a repo that isnt public (of the company i work), but in this public package it's the same issue.
I add both version in the github action to show that the same code works on node 18.10 and not on node 18.11.

@w3nl
Copy link
Author

w3nl commented Oct 15, 2022

Here a test with and without c8: https://github.com/hckrnews/objects/actions/runs/3256091485/jobs/5346101809
node 18.10: works with and without c8
node 18.11: works without c8, but with c8 it failed, no tests are found
Schermafbeelding 2022-10-15 om 16 51 03

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2022

I can reproduce this on macOS:

const test = require('node:test');
test('test');

npx c8 node --test test.js segfaults on v18.11.0 and main. If you don't use --test, the segfault does not occur. On v18.10.0, it works fine with and without --test.

It looks like there hasn't been a new version of c8 in a few months so this is almost certainly a node bug.

@Trott
Copy link
Member

Trott commented Oct 15, 2022

@nodejs/test_runner

@MoLow
Copy link
Member

MoLow commented Oct 15, 2022

I will take a look soon, I am with a temp computer so might take more time.
It might very well be related to #44520

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2022

It looks like beb0520 introduced the problem, assuming I bisected correctly.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 15, 2022

More specifically, commenting out this if statement makes things work again.

@MoLow
Copy link
Member

MoLow commented Oct 15, 2022

this also reproduces when passing NODE_V8_COVERAGE

root@81d2d82220ad:/home/node# NODE_V8_COVERAGE=. node --test index.js
Segmentation fault (core dumped)

@cjihrig - the if you have mentioned is there to make SIGUSR1 only start an inspector server on the internal process (to avoid conflicting ports), so I will try tackling a bit to make NODE_V8_COVERAGE work without exposing a port, since what it seems to do is save files to the disk

@MoLow MoLow added the coverage Issues and PRs related to native coverage support. label Oct 15, 2022
@cjihrig
Copy link
Contributor

cjihrig commented Oct 16, 2022

@MoLow do you think you'll be able to fix it before the next release? Is there anything I can help with?

If we can't get it fixed, I think we should either revert the relevant changes, or reintroduce logic to make --test and --inspect incompatible. Regarding port conflicts - is there any reason not to use the approach the cluster module uses?

@MoLow
Copy link
Member

MoLow commented Oct 18, 2022

@cjihrig I'l try fixing before the next release, indeed.
if not - lets revert, sounds correct.

regarding the cluster module - I find the behavior there a little less friendly then #44520 - forcing a concurrency of 1 makes the inspector port be the port you specified, and in node:cluster you end up with open ports you did not specify

@MoLow
Copy link
Member

MoLow commented Oct 18, 2022

Is there anything I can help with?

@cjihrig If you want to take a look - My plan was to figure out how to start a v8 inspector agent that does not expose/run a websocket server and enable that when !options.allow_attaching_debugger && NODE_V8_COVERAGE,

nodejs-github-bot pushed a commit that referenced this issue Oct 20, 2022
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@w3nl
Copy link
Author

w3nl commented Oct 22, 2022

Thanks a lot @Trott @cjihrig @MoLow

RafaelGSS pushed a commit that referenced this issue Nov 1, 2022
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@w3nl
Copy link
Author

w3nl commented Nov 2, 2022

@RafaelGSS any idea when a new release of node will be published? 18.12 doesn't had the fix

@RafaelGSS
Copy link
Member

@RafaelGSS any idea when a new release of node will be published? 18.12 doesn't had the fix

Probably next week or the one after. We'll update nodejs/Release#737 according

RafaelGSS pushed a commit that referenced this issue Nov 10, 2022
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this issue Nov 23, 2022
PR-URL: nodejs#45055
Fixes: nodejs#45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this issue Dec 9, 2022
PR-URL: nodejs#45055
Fixes: nodejs#45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@w3nl
Copy link
Author

w3nl commented Dec 19, 2022

Still not available in node 18.x, only in 19.x
Hope a new release of 18 will be soon, there was a release scheduled, but still no release deployed in december.

danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45055
Fixes: #45013
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@anton-107
Copy link

I have fixed my issue by moving to node:20 image in my github actions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage Issues and PRs related to native coverage support. test_runner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@anton-107 @Trott @cjihrig @w3nl @MoLow @RafaelGSS and others