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

doc: update test concurrency description / default value #46457

Conversation

richiemccoll
Copy link
Contributor

@richiemccoll richiemccoll commented Feb 1, 2023

Fixes: #45643.

This PR aligns the documentation for the test runner concurrency options. The description for context.test and run are now consistent with the test section as they both allow number or boolean for concurrency.

Screenshot 2023-02-01 at 09 46 05

I've also updated:

  • the default value for context.test from 1 to false so that it's consistent with test concurrency default.
  • the default value for run from true to false so that it's consistent with the test/context.test defaults.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. dont-land-on-v14.x test_runner Issues and PRs related to the test runner subsystem. labels Feb 1, 2023
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch from 85910bb to 0210c43 Compare February 1, 2023 09:37
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch 4 times, most recently from 2937cfc to ac76ec2 Compare February 1, 2023 10:10
@aduh95
Copy link
Contributor

aduh95 commented Feb 1, 2023

Other occurrences of concurrency default being documented:

node/doc/api/test.md

Lines 679 to 685 in 1ecc03e

* `concurrency` {number|boolean} If a number is provided,
then that many files would run in parallel.
If truthy, it would run (number of cpu cores - 1)
files in parallel.
If falsy, it would only run one file at a time.
If unspecified, subtests inherit this value from their parent.
**Default:** `true`.

node/doc/api/test.md

Lines 1648 to 1650 in 1ecc03e

* `concurrency` {number} The number of tests that can be run at the same time.
If unspecified, subtests inherit this value from their parent.
**Default:** `1`.

It'd be nice to make all of them be consistent w.r.t to truthy/true etc.

@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch from ac76ec2 to f23581b Compare February 1, 2023 15:07
@richiemccoll
Copy link
Contributor Author

@aduh95 Thanks. The options.concurrency description and default values for test, context.text and run should be consistent now.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

The default is actually neither 1 nor false for t.test, the default is to inherit from the parent, let's fix that.

doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch 2 times, most recently from ed6de70 to 75f89a6 Compare February 1, 2023 16:15
@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch 2 times, most recently from d73bf28 to 5156131 Compare February 2, 2023 15:26
@aduh95 aduh95 requested a review from MoLow February 2, 2023 15:40
@GeoffreyBooth
Copy link
Member

cc @JakobJingleheimer

@JakobJingleheimer
Copy link
Contributor

JakobJingleheimer commented Feb 2, 2023

This updates only the documentation, but no implementation. Was the documentation merely incorrect? (I'm pretty sure at least 1 of the changes does correct the doc to the actual behaviour).

If so, happy to switch to approve.

@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch from 5156131 to 388b813 Compare February 3, 2023 10:17
@richiemccoll
Copy link
Contributor Author

This updates only the documentation, but no implementation. Was the documentation merely incorrect? (I'm pretty sure at > least 1 of the changes does correct the doc to the actual behaviour).

@JakobJingleheimer Yes, the documentation was incorrect regarding subtest options.concurrency. The other changes are small clarifications regarding os. availableParallelism - 1 and subtest default value.

doc/api/test.md Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch 3 times, most recently from 4d9eae5 to 1f61d56 Compare February 5, 2023 06:37
@MoLow
Copy link
Member

MoLow commented Feb 5, 2023

@richiemccoll linter is failing

@richiemccoll richiemccoll force-pushed the doc/test-runner-context-concurrency branch from 1f61d56 to 2109a3d Compare February 6, 2023 07:56
@richiemccoll
Copy link
Contributor Author

@richiemccoll linter is failing

@MoLow Thanks. Pushed a fix.

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 6, 2023
@nodejs-github-bot
Copy link
Collaborator

MoLow pushed a commit that referenced this pull request Feb 6, 2023
PR-URL: #46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@MoLow
Copy link
Member

MoLow commented Feb 6, 2023

Landed in 7d68b7b

@MoLow MoLow closed this Feb 6, 2023
@aduh95
Copy link
Contributor

aduh95 commented Feb 6, 2023

Thanks a lot for the contribution @richiemccoll and congrats on your first commit landed on nodejs/node 🎉

@richiemccoll
Copy link
Contributor Author

richiemccoll commented Feb 6, 2023

Thanks a lot for the contribution @richiemccoll and congrats on your first commit landed on nodejs/node

Thanks for the reviews and feedback! @aduh95 @MoLow

MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
PR-URL: nodejs/node#46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
(cherry picked from commit 7d68b7bbfc9ffbd2ad0913972ac0b1a315679b06)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 7, 2023
PR-URL: nodejs/node#46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
(cherry picked from commit 7d68b7bbfc9ffbd2ad0913972ac0b1a315679b06)
MoLow pushed a commit to nodejs/node-core-test that referenced this pull request Feb 8, 2023
PR-URL: nodejs/node#46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
(cherry picked from commit 7d68b7bbfc9ffbd2ad0913972ac0b1a315679b06)
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
PR-URL: #46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
PR-URL: #46457
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. doc Issues and PRs related to the documentations. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[node:test] concurrency type and default value
7 participants