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

make -J behavior default in test.py #40945

Closed
wants to merge 5 commits into from
+16 −11
Closed

make -J behavior default in test.py #40945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2244c1
tools,test: make -J behavior default for test.py
Trott Nov 24, 2021
c85a45b
doc: remove legacy -J test.py option from BUILDING.md
Trott Nov 24, 2021
1fe77c7
build: remove legacy -J test.py option from Makefile/vcbuild
Trott Nov 24, 2021
01db947
fixup! tools,test: make -J behavior default for test.py
Trott Nov 25, 2021
54996df
fixup! doc: remove legacy -J test.py option from BUILDING.md
Trott Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion BUILDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ by providing the name of a subsystem:

```text
$ make coverage-clean
$ NODE_V8_COVERAGE=coverage/tmp tools/test.py -J --mode=release child-process
$ NODE_V8_COVERAGE=coverage/tmp tools/test.py --mode=release child-process
$ make coverage-report-js
```

Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ FIND ?= find
ifdef JOBS
PARALLEL_ARGS = -j $(JOBS)
else
PARALLEL_ARGS = -J
PARALLEL_ARGS =
endif

ifdef ENABLE_V8_TAP
Expand Down
13 changes: 9 additions & 4 deletions tools/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,9 +1362,9 @@ def BuildOptions():
default="")
result.add_option("--warn-unused", help="Report unused rules",
default=False, action="store_true")
result.add_option("-j", help="The number of parallel tasks to run",
default=1, type="int")
result.add_option("-J", help="Run tasks in parallel on all cores",
result.add_option("-j", help="The number of parallel tasks to run, 0=use number of cores",
default=0, type="int")
result.add_option("-J", help="For legacy compatibility, has no effect",
default=False, action="store_true")
result.add_option("--time", help="Print timing information after running",
default=False, action="store_true")
Expand Down Expand Up @@ -1423,11 +1423,16 @@ 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.

# inherit JOBS from environment if provided. some virtualised systems
# tends to exaggerate the number of available cpus/cores.
cores = os.environ.get('JOBS')
options.j = int(cores) if cores is not None else multiprocessing.cpu_count()
elif options.J:
# If someone uses -j and legacy -J, let them know that we will be respecting
# -j and ignoring -J, which is the opposite of what we used to do before -J
# became a legacy no-op.
print('Warning: Legacy -J option is ignored. Using the -j option.')
if options.flaky_tests not in [RUN, SKIP, DONTCARE]:
print("Unknown flaky-tests mode %s" % options.flaky_tests)
return False
Expand Down
10 changes: 5 additions & 5 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ if /i "%1"=="nosnapshot" set nosnapshot=1&goto arg-ok
if /i "%1"=="noetw" set noetw=1&goto arg-ok
if /i "%1"=="ltcg" set ltcg=1&goto arg-ok
if /i "%1"=="licensertf" set licensertf=1&goto arg-ok
if /i "%1"=="test" set test_args=%test_args% -J %common_test_suites%&set lint_cpp=1&set lint_js=1&set lint_md=1&goto arg-ok
if /i "%1"=="test-ci-native" set test_args=%test_args% %test_ci_args% -J -p tap --logfile test.tap %CI_NATIVE_SUITES% %CI_DOC%&set build_addons=1&set build_js_native_api_tests=1&set build_node_api_tests=1&set cctest_args=%cctest_args% --gtest_output=xml:cctest.junit.xml&goto arg-ok
if /i "%1"=="test-ci-js" set test_args=%test_args% %test_ci_args% -J -p tap --logfile test.tap %CI_JS_SUITES%&set no_cctest=1&goto arg-ok
if /i "%1"=="test" set test_args=%test_args% %common_test_suites%&set lint_cpp=1&set lint_js=1&set lint_md=1&goto arg-ok
if /i "%1"=="test-ci-native" set test_args=%test_args% %test_ci_args% -p tap --logfile test.tap %CI_NATIVE_SUITES% %CI_DOC%&set build_addons=1&set build_js_native_api_tests=1&set build_node_api_tests=1&set cctest_args=%cctest_args% --gtest_output=xml:cctest.junit.xml&goto arg-ok
if /i "%1"=="test-ci-js" set test_args=%test_args% %test_ci_args% -p tap --logfile test.tap %CI_JS_SUITES%&set no_cctest=1&goto arg-ok
if /i "%1"=="build-addons" set build_addons=1&goto arg-ok
if /i "%1"=="build-js-native-api-tests" set build_js_native_api_tests=1&goto arg-ok
if /i "%1"=="build-node-api-tests" set build_node_api_tests=1&goto arg-ok
Expand All @@ -103,7 +103,7 @@ if /i "%1"=="test-doc" set test_args=%test_args% %CI_DOC%&set doc=1&&set li
if /i "%1"=="test-js-native-api" set test_args=%test_args% js-native-api&set build_js_native_api_tests=1&goto arg-ok
if /i "%1"=="test-node-api" set test_args=%test_args% node-api&set build_node_api_tests=1&goto arg-ok
if /i "%1"=="test-benchmark" set test_args=%test_args% benchmark&goto arg-ok
if /i "%1"=="test-simple" set test_args=%test_args% sequential parallel -J&goto arg-ok
if /i "%1"=="test-simple" set test_args=%test_args% sequential parallel&goto arg-ok
if /i "%1"=="test-message" set test_args=%test_args% message&goto arg-ok
if /i "%1"=="test-tick-processor" set test_args=%test_args% tick-processor&goto arg-ok
if /i "%1"=="test-internet" set test_args=%test_args% internet&goto arg-ok
Expand Down Expand Up @@ -644,7 +644,7 @@ if defined test_node_inspect goto node-test-inspect
goto node-tests

:node-check-deopts
python tools\test.py --mode=release --check-deopts parallel sequential -J
python tools\test.py --mode=release --check-deopts parallel sequential
if defined test_node_inspect goto node-test-inspect
goto node-tests

Expand Down