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

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Nov 30, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joaocgreis
Copy link
Member Author

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 2, 2019
@Trott
Copy link
Member

Trott commented Dec 2, 2019

Landed in 8c101de...3b484ed

Trott pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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
MylesBorins pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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 pushed 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants