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.

Sign up for GitHub

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

Jump to bottom

test_runner: do not read from process.argv and process.cwd() in run() #53867

Open
mcollina opened this issue Jul 16, 2024 · 11 comments · Fixed by #53937
Open

test_runner: do not read from process.argv and process.cwd() in run() #53867

mcollina opened this issue Jul 16, 2024 · 11 comments · Fixed by #53937
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@mcollina
Copy link
Member

Currently the codebase for

node/lib/internal/test_runner/runner.js

Line 472 in 362afa5

function run(options = kEmptyObject) {
accesses some poperties of process, capturing some options there. However, we also expose run() to the end users, therefore we should capture all this information in

node/lib/internal/main/test_runner.js

Line 68 in 362afa5

const options = {
and pass it down to run() as options.
@mcollina mcollina added good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem. labels Jul 16, 2024
@mcollina mcollina changed the title test_runner: do not read from process.argv in run() test_runner: do not read from process.argv and process.cwd() in run() Jul 16, 2024
@mcollina mcollina mentioned this issue Jul 16, 2024
Merged
@eliphazbouye
Copy link
Contributor

@mcollina this issue still relevant or are you already working on it ? I ask it because I see you mentioned it on your last PR !
If it's still relevant, I'll start working on it :)

@mcollina
Copy link
Member Author

It's likely best to wait until my PR lands first!

@eliphazbouye
Copy link
Contributor

Okay, I'll wait :)

This was referenced Jul 18, 2024
Closed
Merged
nodejs-github-bot pushed a commit that referenced this issue Jul 22, 2024
@cjihrig
test_runner: refactor coverage to pass in config options
ea837a0 
This commit updates the test runner's code coverage so that
coverage options are explicitly passed in instead of pulled
from command line options.

PR-URL: #53931
Refs: #53924
Refs: #53867
Refs: #53866
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member Author

@eliphazb go ahead and make the PR!

@eliphazbouye
Copy link
Contributor

Okay 🚀 I'm going

targos pushed a commit that referenced this issue Jul 28, 2024
@cjihrig @targos
test_runner: refactor coverage to pass in config options
280ca56 
This commit updates the test runner's code coverage so that
coverage options are explicitly passed in instead of pulled
from command line options.

PR-URL: #53931
Refs: #53924
Refs: #53867
Refs: #53866
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
@atlowChemi atlowChemi mentioned this issue Jul 30, 2024
Merged
@eliphazbouye
Copy link
Contributor

@mcollina someone already take it :(

RafaelGSS pushed a commit that referenced this issue Aug 5, 2024
@cjihrig @RafaelGSS
test_runner: refactor coverage to pass in config options
876e7b3 
This commit updates the test runner's code coverage so that
coverage options are explicitly passed in instead of pulled
from command line options.

PR-URL: #53931
Refs: #53924
Refs: #53867
Refs: #53866
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina mcollina mentioned this issue Aug 8, 2024
Merged
@mcollina
Copy link
Member Author

@SophonieBouye doesn't look like it to me. There is a lot more to be done here.

@cjihrig cjihrig mentioned this issue Aug 13, 2024
Closed
cjihrig added a commit to cjihrig/node that referenced this issue Sep 8, 2024
@cjihrig
test_runner: add 'test:summary' event
b5254f9 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- The test runner internals no longer change the process exit
  code. This may be important to run() users.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
@cjihrig cjihrig mentioned this issue Sep 8, 2024
Merged
cjihrig added a commit to cjihrig/node that referenced this issue Sep 14, 2024
@cjihrig
test_runner: add 'test:summary' event
dfcbb86 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
cjihrig added a commit to cjihrig/node that referenced this issue Sep 14, 2024
@cjihrig
test_runner: add 'test:summary' event
808d7ad 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
@pmarchini
Copy link
Member

@mcollina, Is this issue really closed? The PR I'm working on, #54705, seems related

@cjihrig cjihrig reopened this Sep 20, 2024
cjihrig added a commit to cjihrig/node that referenced this issue Sep 20, 2024
@cjihrig
test_runner: add 'test:summary' event
468d4e3 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
nodejs-github-bot pushed a commit that referenced this issue Sep 21, 2024
@cjihrig
test_runner: add 'test:summary' event
059e08b 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: #53867
Refs: #54812
PR-URL: #54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Oct 4, 2024
@atlowChemi @targos
test_runner: add support for coverage via run()
d7c708a 
PR-URL: #53937
Fixes: #53867
Refs: #53924
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Oct 4, 2024
@cjihrig @targos
test_runner: add 'test:summary' event
b62f2f8 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: #53867
Refs: #54812
PR-URL: #54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@mcollina
Copy link
Member Author

I think this is fixed in #54705

@cjihrig
Copy link
Contributor

cjihrig commented Oct 12, 2024

It's not fixed 😞. There are still a number of uses of process state.

@cjihrig cjihrig reopened this Oct 12, 2024
@karankraina
Copy link

@cjihrig Can you please elaborate a little on what is pending in this? I will pick it up.

louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@atlowChemi @louwers
test_runner: add support for coverage via run()
b30f1f3 
PR-URL: nodejs#53937
Fixes: nodejs#53867
Refs: nodejs#53924
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@cjihrig @louwers
test_runner: add 'test:summary' event
0c2ab76 
This commit adds a new 'test:summary' event to the test runner's
reporting interface. This new event serves two purposes:

- In the future, the test runner internals will no longer need to
  change the process exit code. This may be important to run()
  users. Unfortunately, this is a breaking change, so it needs to
  be changed in a major version.
- The reporting interface now has a single event that can identify
  passing or failing test runs.

Refs: nodejs#53867
Refs: nodejs#54812
PR-URL: nodejs#54851
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test_runner: add coverage support to run function atlowChemi/node
5 participants
@mcollina @cjihrig @karankraina @pmarchini @eliphazbouye