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

Windows CI job update changes #30724

Closed

Conversation

@joaocgreis
Copy link
Member

joaocgreis commented Nov 30, 2019

The Windows CI job currently includes direct calls to cctest and test.py. Moving most of the logic back to vcbuild has been in our backlog since the change was made.

This became necessary now because I'm working on an update to the CI job that makes it work for all Node.js versions, if the vcbuild targets are there. I will open backport PRs to active branches.

The second commit fixes an issue where failures cctest make the job become yellow rather than red. Since this works with the new target, this will only apply to the new job.

Refs: nodejs/build#1996

cc @nodejs/build @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
joaocgreis added 2 commits Nov 29, 2019
Don't exit vcbuild with error code 0 when cctest fails.
@nodejs-github-bot

This comment has been minimized.

@rvagg
rvagg approved these changes Dec 2, 2019
Copy link
Member

rvagg left a comment

I'm surprised that test-ci was deprecated and now removed, is this because we run fanned and don't need them together?

@joaocgreis

This comment has been minimized.

Copy link
Member Author

joaocgreis commented Dec 2, 2019

I believe test-ci was deprecated for nodejs/build#1761, the logic moved to the Jenkins job. This brings it back to vcbuild.bat. There are two test-ci-* targets because we will have two test jobs (preview in https://ci.nodejs.org/job/reis-test-commit-windows-fanned/).

@nodejs/build @nodejs/platform-windows please take a look, we are not running CI at full capacity because some resources have already been re-organized to test, I need this to land to complete the work. Thanks!

@Trott Trott added the author ready label Dec 2, 2019
@Trott
Trott approved these changes Dec 2, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 2, 2019

Landed in 8c101de...3b484ed

Trott added a commit that referenced this pull request Dec 2, 2019
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott added a commit that referenced this pull request Dec 2, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott closed this Dec 2, 2019
targos added a commit that referenced this pull request Dec 3, 2019
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Dec 3, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
BethGriggs added a commit that referenced this pull request Dec 3, 2019
Backport-PR-URL: #30726
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this pull request Dec 3, 2019
Don't exit vcbuild with error code 0 when cctest fails.

Backport-PR-URL: #30726
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this pull request Dec 3, 2019
Backport-PR-URL: #30727
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs added a commit that referenced this pull request Dec 3, 2019
Don't exit vcbuild with error code 0 when cctest fails.

Backport-PR-URL: #30727
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2019
targos added a commit that referenced this pull request Dec 5, 2019
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos added a commit that referenced this pull request Dec 5, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
PR-URL: nodejs#30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sebastien-Ahkrin added a commit to Sebastien-Ahkrin/node that referenced this pull request Dec 11, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: nodejs#30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 16, 2019
Backport-PR-URL: #30727
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 16, 2019
Backport-PR-URL: #30726
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 16, 2019
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Don't exit vcbuild with error code 0 when cctest fails.

Backport-PR-URL: #30726
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
Don't exit vcbuild with error code 0 when cctest fails.

Backport-PR-URL: #30727
PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
BethGriggs added a commit that referenced this pull request Dec 31, 2019
Don't exit vcbuild with error code 0 when cctest fails.

PR-URL: #30724
Refs: nodejs/build#1996
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@@ -86,8 +92,8 @@ 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
:: test-ci is deprecated
if /i "%1"=="test-ci" goto arg-ok

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 2, 2020

Member

The removal of the test-ci target breaks some of the CI jobs, see https://ci.nodejs.org/job/node-test-commit-windows/976/nodes=win10/console:

C:\workspace\node-test-commit-windows\nodes\win10>vcbuild.bat release nosign x64 test-ci ignore-flaky NODE_TEST_DIR=C:\node-tmp  
Note: vcbuild no longer signs by default. "nosign" is redundant.
Error: invalid command line option `test-ci`.

(Apologies if it's already been reported. I searched nodejs/node and nodejs/build but didn't find an exact match.)

This comment has been minimized.

Copy link
@joaocgreis

joaocgreis Jan 2, 2020

Author Member

@bnoordhuis any reason not to use node-test-commit-windows-fanned instead? node-test-commit-windows has been forgotten for a long time, I'll archive it if there's no objection.

Note that test-ci was a no-op as removed here, running with it only compiled, did not run tests.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Jan 3, 2020

Member

Ah, if it's unmaintained then yes, please remove or archive it to avoid confusion. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.