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

make -J behavior default in test.py #40945

Closed
wants to merge 5 commits into from
Closed

make -J behavior default in test.py #40945

wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 24, 2021

No description provided.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2021
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Nov 24, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41067/

richardlau
richardlau reviewed Nov 24, 2021
View reviewed changes
tools/test.py
@@ -1423,7 +1423,7 @@ def ProcessOptions(options):
if options.run[0] >= options.run[1]:
print("The test group to run (n) must be smaller than number of groups (m).")
return False
if options.J:
if options.j == 0:
Copy link
Member

Choose a reason for hiding this comment

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

This changes precedence of the -j and -J flags, e.g. if test.py -j 3 -J was run and JOBS=2 then the old code would have overwritten the j value from JOBS but the new code would leave j unchanged.

It's probably a rare edge case that both flags would be specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we keep it so -J effectively negates -j, we'll have to update the help text to indicate that -J doesn't quite have no effect at all.

If we want to preserve that behavior, then perhaps the thing to do is print a warning when someone is using both -J and -j? Or maybe print a warning every time -J is used?

I suspect, like you, that this is a rare edge case. I also suspect that it basically does not matter. Like, if we run with 2 processes instead of 4 (or the other way around) in a rare edge case....oh well?

@richardlau Would you be comfortable if we kept the behavior as it is in this PR but add a warning that gets printed if both -j and -J are specified?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be okay with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau Would you be comfortable if we kept the behavior as it is in this PR but add a warning that gets printed if both -j and -J are specified?

This is the behavior I've implemented in a fixup commit. PTAL.

@github-actions github-actions bot mentioned this pull request Nov 25, 2021
Open
38 tasks
richardlau
richardlau reviewed Nov 25, 2021
View reviewed changes
BUILDING.md Outdated Show resolved Hide resolved
richardlau
richardlau approved these changes Nov 25, 2021
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

lgtm sans the conflict markers in BUILDING.md

@github-actions github-actions bot mentioned this pull request Nov 26, 2021
Open
33 tasks
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41121/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41122/

@github-actions github-actions bot mentioned this pull request Nov 27, 2021
Open
27 tasks
@Trott Trott added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 27, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in e64c66c...a257294

nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2021
@Trott @nodejs-github-bot
tools,test: make -J behavior default for test.py
f7341e3 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2021
@Trott @nodejs-github-bot
doc: remove legacy -J test.py option from BUILDING.md
5068168 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2021
@Trott @nodejs-github-bot
build: remove legacy -J test.py option from Makefile/vcbuild
a257294 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
This was referenced Nov 28, 2021
Open
Open
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
tools,test: make -J behavior default for test.py
17c6d1a 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
doc: remove legacy -J test.py option from BUILDING.md
3fd1cb7 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
@Trott @danielleadams
build: remove legacy -J test.py option from Makefile/vcbuild
6523b14 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
tools,test: make -J behavior default for test.py
82daaa9 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
doc: remove legacy -J test.py option from BUILDING.md
61b2e2e 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
@Trott @danielleadams
build: remove legacy -J test.py option from Makefile/vcbuild
1b8baf0 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danielleadams danielleadams mentioned this pull request Dec 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@Trott @danielleadams
tools,test: make -J behavior default for test.py
b6c7f8d 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@Trott @danielleadams
doc: remove legacy -J test.py option from BUILDING.md
eac6031 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
@Trott @danielleadams
build: remove legacy -J test.py option from Makefile/vcbuild
4947869 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
tools,test: make -J behavior default for test.py
53570a8 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
doc: remove legacy -J test.py option from BUILDING.md
268771c 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@Trott @danielleadams
build: remove legacy -J test.py option from Makefile/vcbuild
066e0d6 
PR-URL: #40945
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
@Trott Trott deleted the dash-j branch September 25, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@richardlau richardlau richardlau approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @nodejs-github-bot @lpinca @richardlau @aduh95