-
Notifications
You must be signed in to change notification settings - Fork 30k
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
test: run the addon tests last #12062
Conversation
I think that "See ..." line should be formatted as a proper "Refs:" or "Fixes:" metadata field. Also, there's probably a typo in the commit message (s/lasts/last ?). |
9eeb691
to
cb369d2
Compare
Thanks for the tips, I've changed the commit message. |
Does this affect Windows builds? Should the |
Some tests failed, trying another CI: https://ci.nodejs.org/job/node-test-pull-request/7055/ |
@seppevs regarding @vsemozhetbyt's comment, please see https://github.com/nodejs/node/blob/master/vcbuild.bat#L62-L63. |
@vsemozhetbyt those tests are unrelated to this PR and I don't think another CI run will help, tbh :) |
cb369d2
to
dc3a770
Compare
@aqrln Thanks for the tip. I have modified the vcbuild.bat now. Can you review it, I'm not completely sure I did it correctly (and I have no Windows machine to verify). |
@seppevs heh, neither do I, so I hope my suggestion was correct :) /cc @nodejs/build @nodejs/platform-windows EDIT: whoops, only members can mention teams. @vsemozhetbyt can you do that, please? |
/cc @nodejs/build @nodejs/platform-windows |
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.
Yes please.
cc @bnoordhuis probably?
Makefile
Outdated
@@ -195,7 +195,7 @@ test: all | |||
$(MAKE) build-addons | |||
$(MAKE) cctest | |||
$(PYTHON) tools/test.py --mode=release -J \ | |||
addons doctool inspector known_issues message pseudo-tty parallel sequential | |||
doctool inspector known_issues message pseudo-tty parallel sequential 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.
One more thing. CI runs make test-ci
, not make test
, so you need to change line 311 too.
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.
Thanks, I've changed the order on line 311 as well.
dc3a770
to
3196f56
Compare
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.
Note that failures in cctest
also suffer this problem.
So on windows, it's only a half solution since |
@refack hmm, not only only on Windows. The same goes for Unix. |
Makefile
Outdated
@@ -195,7 +195,7 @@ test: all | |||
$(MAKE) build-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.
maybe split this to "all other test" -> build -> "test addons"... 🤔
vcbuild.bat
Outdated
if /i "%1"=="test" set test_args=%test_args% addons doctool known_issues message parallel sequential -J&set cpplint=1&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% doctool known_issues message parallel sequential addons -J&set cpplint=1&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 doctool inspector known_issues message sequential parallel addons&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.
Is you decide to split the test, you'll need it here too...
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.
Too much hassle to refactor vcbuild.bat
Current LGTM (works on Windows)
3196f56
to
141a6d9
Compare
This needs a rebase before it can be landed. |
Running the addon tests before the parallel, sequential, etc. tests can be a problem if there is a bug in node that prevents the addon tests from running properly. When the addon tests fail for any reason, then none of the other tests (e.g. parallel, etc.) are executed. Running the addon tests last fixes this. Refs: nodejs#12031
141a6d9
to
9149610
Compare
@jasnell rebase done |
@seppevs can you please rebase it again? |
ha! posted then saw @aqrln's comment. guess I should read all the way through before posting ;-) |
Poor guy already rebased one. I'm taking care of this. |
Running the addon tests before the parallel, sequential, etc. tests can be a problem if there is a bug in node that prevents the addon tests from running properly. When the addon tests fail for any reason, then none of the other tests (e.g. parallel, etc.) are executed. Running the addon tests last fixes this. Refs: nodejs#12031 PR-URL: nodejs#12062 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Landed in 3d2181c |
Running the addon tests before the parallel, sequential, etc. tests can be a problem if there is a bug in node that prevents the addon tests from running properly. When the addon tests fail for any reason, then none of the other tests (e.g. parallel, etc.) are executed. Running the addon tests last fixes this. Refs: #12031 PR-URL: #12062 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Running the addon tests before the parallel, sequential, etc. tests can be a problem if there is a bug in node that prevents the addon tests from running properly. When the addon tests fail for any reason, then none of the other tests (e.g. parallel, etc.) are executed. Running the addon tests last fixes this. Refs: #12031 PR-URL: #12062 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Running the addon tests before the parallel, sequential, etc. tests can be a problem if there is a bug in node that prevents the addon tests from running properly. When the addon tests fail for any reason, then none of the other tests (e.g. parallel, etc.) are executed. Running the addon tests last fixes this. Refs: #12031 PR-URL: #12062 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Running the addon tests before the parallel, sequential, etc. tests
can be a problem if there is a bug in node that prevents the
addon tests from running properly. When the addon tests fail for any
reason, then none of the other tests (e.g. parallel, etc.)
are executed.
Running the addon tests last fixes this.
See #12031
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test