Skip to content

Conversation

@MoLow
Copy link
Member

@MoLow MoLow commented Jun 11, 2023

Fixes #46727

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jun 11, 2023
@MoLow
Copy link
Member Author

MoLow commented Jun 11, 2023

@sankalp1999 FWIW, if you want to complete the work on #47797 I will close this PR

@sankalp1999
Copy link
Contributor

sankalp1999 commented Jun 11, 2023

@MoLow sorry, I couldn't find the time. I think you can continue with this one. I will close my PR. Thank you for reviewing.

* `file` {string|undefined} The path of the test file,
undefined if test is not ran through a file.
* `name` {string} The test name.
* `nesting` {number} The nesting level of the test.
Copy link
Member

Choose a reason for hiding this comment

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

Nit

Suggested change
* `nesting` {number} The nesting level of the test.
* `level` {number} The nesting level of the test.
Copy link
Member Author

Choose a reason for hiding this comment

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

nesting is already used on all other events on this class

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2023
@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 12, 2023

* `data` {Object}
* `file` {string|undefined} The path of the test file,
undefined if test is not ran through a file.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
undefined if test is not ran through a file.
undefined if test was not run through a file.
Copy link
Member Author

Choose a reason for hiding this comment

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

this exact terminology is used in 6 other places in this file, so I will fix it in a follow-up PR


* `data` {Object}
* `file` {string|undefined} The path of the test file,
undefined if test is not ran through a file.
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to say here?

"will not run" in this (and above) perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think running within REPL is the main example of this. Il refine this in a follow-up

const filename = `custom.${ext}`;
const child = spawnSync(process.execPath,
['--test', '--test-reporter', fixtures.fileURL('test-runner/custom_reporters/', filename),
testFile]);
Copy link
Member

Choose a reason for hiding this comment

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

Would be useful to also test the arguments

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 8bc6e19 into nodejs:main Jun 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 8bc6e19

@MoLow MoLow deleted the enque-deque branch June 13, 2023 18:39
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
PR-URL: #48428 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
MoLow added a commit to MoLow/node that referenced this pull request Jul 9, 2023
PR-URL: nodejs#48428 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 12, 2023
PR-URL: #48428 Backport-PR-URL: #48684 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48428 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48428 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

8 participants