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

cli: allow test coverage, module mocks, and snapshots in NODE_OPTIONS #56114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JakobJingleheimer
Copy link
Member

After asking the @nodejs/test_runner team, there does not appear to be a particular reason these are not already included.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 2, 2024
@JakobJingleheimer JakobJingleheimer added lts-watch-v22.x PRs that may need to be released in v22.x and removed c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 2, 2024
@JakobJingleheimer
Copy link
Member Author

image

No I didn't…

@JakobJingleheimer JakobJingleheimer added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 2, 2024
src/node_options.cc Outdated Show resolved Hide resolved
@JakobJingleheimer JakobJingleheimer changed the title cli: allow test-coverage, module mocks, and snapshots in NODE_OPTIONS cli: allow test coverage, module mocks, and snapshots in NODE_OPTIONS Dec 2, 2024
@ljharb
Copy link
Member

ljharb commented Dec 2, 2024

@JakobJingleheimer ffr the api request to change github labels is done as a "here's all the labels this issue has" request, not a delta, so there was a race condition between you and the bot. the bot said "it's just these 2!" and you said "it's just this 1!".

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (d5d1e80) to head (df9e99c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56114   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         656      656           
  Lines      189002   189002           
  Branches    36003    36000    -3     
=======================================
+ Hits       166335   166338    +3     
- Misses      15838    15839    +1     
+ Partials     6829     6825    -4     
Files with missing lines Coverage Δ
src/node_options.cc 87.55% <100.00%> (ø)

... and 28 files with indirect coverage changes

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the only flags that need adding to NODE_OPTIONS? Or at least the only test runner-related ones. We might as well add all the missing ones, or at least the missing test runner flags, while we’re at it.

@JakobJingleheimer
Copy link
Member Author

I have no idea why only some flags are allowed in NODE_OPTIONS, and I can't find anyone who knows.

@@ -3070,6 +3070,9 @@ one is included in the list below.
* `--experimental-shadow-realm`
* `--experimental-specifier-resolution`
* `--experimental-strip-types`
* `--experimental-test-coverage`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested, but I would expect passing --experimental-test-coverage in NODE_OPTIONS to cause problems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test runner explicitly tries to filter that argument out when launching worker processes, but NODE_OPTIONS sidesteps that. It may work, but I wouldn't count on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll test that locally against nodejs/userland-migrations#7, which should spawn workers via test runner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v22.x PRs that may need to be released in v22.x needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants