-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
build: enable debugger tests in CI #10456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in principle, but the CI still seems broken.
Would there not be a corresponding change to make in |
Makefile
Outdated
@@ -123,8 +123,7 @@ v8: | |||
test: all | |||
$(MAKE) build-addons | |||
$(MAKE) cctest | |||
$(PYTHON) tools/test.py --mode=release -J \ | |||
addons doctool inspector known_issues message pseudo-tty parallel sequential | |||
$(PYTHON) tools/test.py --mode=release -J $(CI_JS_SUITES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs $(CI_NATIVE_SUITES)
to include addons
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardlau I included that now. Can you please check if the batch file changes are okay?
5ff72fe
to
263c036
Compare
263c036
to
f894bca
Compare
@Trott Oops, I totally forgot about that. I changed that as well now. |
Makefile
Outdated
@@ -200,11 +199,12 @@ test-all-valgrind: test-build | |||
$(PYTHON) tools/test.py --mode=debug,release --valgrind | |||
|
|||
CI_NATIVE_SUITES := addons | |||
CI_JS_SUITES := doctool inspector known_issues message parallel pseudo-tty sequential | |||
CI_JS_SUITES := addons debugger doctool inspector known_issues message \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think addons should be here (it's already in CI_NATIVE_SUITES
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack!
if /i "%1"=="test" set test_args=%test_args% addons doctool known_issues message parallel sequential -J&set jslint=1&set build_addons=1&goto arg-ok | ||
if /i "%1"=="test-ci" set test_args=%test_args% %test_ci_args% -p tap --logfile test.tap addons doctool inspector known_issues message sequential parallel&set cctest_args=%cctest_args% --gtest_output=tap:cctest.tap&set build_addons=1&goto arg-ok | ||
if /i "%1"=="test" set test_args=%test_args% %ci_test_targets% -J&set jslint=1&set build_addons=1&goto arg-ok | ||
if /i "%1"=="test-ci" set test_args=%test_args% %test_ci_args% -p tap --logfile test.tap %ci_test_targets%&set cctest_args=%cctest_args% --gtest_output=tap:cctest.tap&set build_addons=1&goto arg-ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the history here why test
and test-ci
were running different set of tests -- Anyone know why inspector
tests weren't run for test
before and if this change (which would include them) would cause any issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nodejs/platform-windows, any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f894bca
to
9595406
Compare
CI Run: https://ci.nodejs.org/job/node-test-pull-request/5662/ (after landing #10455) |
I think the only reason the debugger tests are in their own directory is because they are so fussy and unreliable. If we can't get the whole directory to be reliable on all platforms, maybe the thing to do instead of enable the debugger directory for tests is to move the ones that are now reliable to |
Let's be careful with this one in CI. I could be wrong but it sure looks like the test failures here linger and hold onto ports that prevent subsequent CI runs from passing on AIX and FreeBSD. Shouldn't stop you from running CI, but maybe @-mention @nodejs/build and/or hop on #node-build on IRC if tests fails so someone can hop on the machines and terminate any stalled processes. |
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for nodejs#10456
1. In Makefile, though CI and local test tests the same subsystems, CI target uses a variable and the local test enumerates the subsystems being tested. This patch makes both of them to use the same variable. 2. In vcbuild.bat, the inspector tests are not run when tested locally. This patch makes sure that they both use the same set of subsysterms set in a variable.
All the problems blocking nodejs#10361 have been resolved. This patch enables debugger in CI.
d0f51f9
to
e6b277c
Compare
New CI Run: https://ci.nodejs.org/job/node-test-pull-request/6106/ (after landing #10822) |
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for nodejs#10456 PR-URL: nodejs#10597 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for nodejs#10456 PR-URL: nodejs#10597 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
@@ -39,6 +39,7 @@ set enable_vtune_arg= | |||
set configure_flags= | |||
set build_addons= | |||
set dll= | |||
set ci_test_targets="addons debugger doctool inspector known_issues message parallel sequential" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will include the quotes in the variable. Open the quotes before the variable name instead:
set "ci_test_targets=addons debugger doctool inspector known_issues message parallel sequential"
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for nodejs#10456 PR-URL: nodejs#10597 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for #10456 PR-URL: #10597 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
`process.title` would work properly only in FreeBSD, OSX, and Linux as per test/parallel/test-setproctitle.js. This patch makes sure that the test expects an empty string in other platforms. This patch helps fix the SmartOS failures in https://ci.nodejs.org/job/node-test-commit/6962/ for #10456 PR-URL: #10597 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
Ping. updates on this one? |
I would suggest closing this. The tests in test/debugger are highly unreliable and we're phasing out the old debugger anyway. It doesn't make sense to sink a lot of time in getting them green. |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build
Description of change
Since #10361 is solved in master
branch, we can enable debugger tests in CI.
cc @nodejs/build