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

build: run flaky tests in Travis #27158

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 9, 2019

Continuation of #27015

Skipping flaky tests in CI is an anti-pattern that should be avoided,
as we do in our ownCI. Failing flaky tests don’t need to be blockers
for a green CI result, but they should be run and reported somehow.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Skipping flaky tests in CI is an anti-pattern that should be avoided,
as we do in our ownCI. Failing flaky tests don’t need to be blockers
for a green CI result, but they should be run and reported *somehow*.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 9, 2019
@addaleax addaleax mentioned this pull request Apr 9, 2019
2 tasks
@Trott Trott requested review from richardlau, lpinca, refack and BridgeAR and removed request for richardlau April 9, 2019 20:50
@refack
Copy link
Contributor

refack commented Apr 9, 2019

IMO the travis run should be a minimal sanity test that returns results ASAP. For me test coverage is a non-goal. I'd go as far as advocating for skipping more tests, not less.

ATM a typical Travis run takes a minimum of 12.5 minutes:
image
of those only 3.5 are bootstrapping and compilation, so 9 minutes just for testing. Seems unproductive.
https://pythonspeed.com/articles/high-cost-slow-tests/

@Trott
Copy link
Member Author

Trott commented Apr 9, 2019

IMO the travis run should be a minimal sanity test that returns results ASAP. For me test coverage is a non-goal. I'd go as far as advocating for skipping more tests, not less.

Interesting. For me, finishing quickly in Travis is a non-goal. (Finishing quickly for local runs, however, is.) Making Travis as useful as possible is a goal, and that means not skipping tests we don't need to skip.

@refack
Copy link
Contributor

refack commented Apr 9, 2019

For example this PR, 15 minutes after submission, our Lite-CI finished, and the Travis job hasn't even started
comments in screenshot were trimmed for emphasis of time and results
image

@Trott
Copy link
Member Author

Trott commented Apr 9, 2019

For example this PR, 15 minutes after submission, our Lite-CI finished, and the Travis job hasn't even started

I just don't see that as a problem, to be honest. If tests aren't working in a Travis environment, they're probably not going to work for some users in some environments. So I want to know.

@richardlau
Copy link
Member

Practically speaking this PR only affects a small number of tests. e.g. for #27164 our CI ran 2615 tests (https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel72-s390x/12353/) while the Travis run ran 2610 tests (https://travis-ci.com/nodejs/node/jobs/191674829#L1063). Five tests isn't going to make a huge difference to the overall running time.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 11, 2019
@Trott
Copy link
Member Author

Trott commented Apr 11, 2019

Landed in 56354d4

@Trott Trott closed this Apr 11, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Apr 11, 2019
Skipping flaky tests in CI is an anti-pattern that should be avoided,
as we do in our ownCI. Failing flaky tests don’t need to be blockers
for a green CI result, but they should be run and reported *somehow*.

PR-URL: nodejs#27158
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
refack added a commit to refack/node that referenced this pull request Apr 12, 2019
* skip compilation by using cache
* split testing into two jobs
* DRY with YAML anchors

PR-URL: nodejs#27182
Refs: nodejs#27158
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants