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

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

@@ -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.

BUILDING.md Outdated Show resolved Hide resolved
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

@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

@nodejs-github-bot
Copy link
Collaborator

@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
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 27, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 13, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #40945
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@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
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
Development

Successfully merging this pull request may close these issues.

5 participants