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

Fix Travis builds #27002

Closed
wants to merge 2 commits into from
Closed

Fix Travis builds #27002

wants to merge 2 commits into from

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Mar 30, 2019

Supersedes #26992 and incorporates #26968.

First commit:

build: add a `Prepare ccache` job in Travis

Combined compile and test of Node.js where lots of files need to be
compiled (e.g. after a V8 update) is exceeding the time limit for
Travis jobs (50 minutes).

Add a job to Travis that compiles Node.js but doesnt run any tests to
populate the ccache. Introduce staging and move the `Test Suite` job
into a later stage so that it can use the populated ccache.

Second commit:

build: fix skipping of flaky tests on Travis

`PARALLEL_ARGS` is overwritten in the Makefile if `JOBS` is set. Use
`CI_JS_SUITES` instead.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Combined compile and test of Node.js where lots of files need to be
compiled (e.g. after a V8 update) is exceeding the time limit for
Travis jobs (50 minutes).

Add a job to Travis that compiles Node.js but doesnt run any tests to
populate the ccache. Introduce staging and move the `Test Suite` job
into a later stage so that it can use the populated ccache.
`PARALLEL_ARGS` is overwritten in the Makefile if `JOBS` is set. Use
`CI_JS_SUITES` instead.
@richardlau richardlau added the build Issues and PRs related to build files or the CI. label Mar 30, 2019
@richardlau richardlau requested a review from refack March 30, 2019 02:09
@refack
Copy link
Contributor

refack commented Mar 30, 2019

Please 👍 to fast-track

@richardlau richardlau added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 30, 2019
@Trott
Copy link
Member

Trott commented Mar 30, 2019

Landed in 30e884f...58bf615

@Trott Trott closed this Mar 30, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Mar 30, 2019
Combined compile and test of Node.js where lots of files need to be
compiled (e.g. after a V8 update) is exceeding the time limit for
Travis jobs (50 minutes).

Add a job to Travis that compiles Node.js but doesnt run any tests to
populate the ccache. Introduce staging and move the `Test Suite` job
into a later stage so that it can use the populated ccache.

PR-URL: nodejs#27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Mar 30, 2019
`PARALLEL_ARGS` is overwritten in the Makefile if `JOBS` is set. Use
`CI_JS_SUITES` instead.

PR-URL: nodejs#27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@richardlau
Copy link
Member Author

Builds are now green again 🎉.

image

It should be safe to restart any timed out Travis pull request builds as they should pick up the refreshed ccache from the master builds.

@richardlau richardlau deleted the travis-stage branch March 30, 2019 06:29
@addaleax
Copy link
Member

@richardlau Why are we skipping flaky tests on Travis in the first place? Wouldn’t dontcare be better, like we do elsewhere in CI? I’ve personally made bad experiences with skipping tests in CI only, because that means that test suite is even more likely to fail for people locally…

@richardlau
Copy link
Member Author

@richardlau Why are we skipping flaky tests on Travis in the first place? Wouldn’t dontcare be better, like we do elsewhere in CI? I’ve personally made bad experiences with skipping tests in CI only, because that means that test suite is even more likely to fail for people locally…

¯\_(ツ)_/¯ Skipping flaky tests was introduced (albeit it wasn't working until this PR) in #23778. I have no objections to dontcare. cc @refack

@refack
Copy link
Contributor

refack commented Mar 30, 2019

I have no objections to dontcare. cc @refack

Even with dontcare the errorlevel is set to 2. Jenkins knows to interpret that as "Unstable", AFAICT Travis doesn't. (Nope, that was removed in favor of tap/xunit parsing).

I'm leaning towards minimal sanity tests in Travis (which IMHO is not CI), I would actually be happy to cull even more test categories...

BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
Combined compile and test of Node.js where lots of files need to be
compiled (e.g. after a V8 update) is exceeding the time limit for
Travis jobs (50 minutes).

Add a job to Travis that compiles Node.js but doesnt run any tests to
populate the ccache. Introduce staging and move the `Test Suite` job
into a later stage so that it can use the populated ccache.

PR-URL: #27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
`PARALLEL_ARGS` is overwritten in the Makefile if `JOBS` is set. Use
`CI_JS_SUITES` instead.

PR-URL: #27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
Combined compile and test of Node.js where lots of files need to be
compiled (e.g. after a V8 update) is exceeding the time limit for
Travis jobs (50 minutes).

Add a job to Travis that compiles Node.js but doesnt run any tests to
populate the ccache. Introduce staging and move the `Test Suite` job
into a later stage so that it can use the populated ccache.

PR-URL: #27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
`PARALLEL_ARGS` is overwritten in the Makefile if `JOBS` is set. Use
`CI_JS_SUITES` instead.

PR-URL: #27002
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
@refack
Copy link
Contributor

refack commented May 23, 2019

@richardlau check out https://travis-ci.com/nodejs/node/builds/112836093 for #27375
image
tl;dr the stages approach pays off in robustness

@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2019
@refack refack added the landed label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants