-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve Travis CI build times #13103
Conversation
3ae2a83
to
45eda12
Compare
Follow up tasks:
|
Awesome work here, I'm not sure about caching node_modules as we often do |
- Cache npm and nvm artifacts - Use jest caching - Run E2E tests in parallel
This feels "more correct" and doesn't significantly affect build performance.
db2ddee
to
94ffdce
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.
We should double check how many Travis jobs can be executed at once. It looks like 7 still run at the same time which is good. I'm wondering how many PRs can be verified at once though. The total time for execution is probably going to be higher than before because all the setup work required for e2e tests even though individual jobs finish much earlier.
.travis.yml
Outdated
- ./bin/setup-local-env.sh | ||
script: | ||
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests | ||
- npm run test-e2e -- --ci --runInBand --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 0' < ~/.jest-e2e-tests ) |
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.
There is no need to define --runInBand
in here. It's already provided behind the scenes:
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.
👍 ab1faa1d7
script: | ||
- npm install || exit 1 | ||
- npm run ci || exit 1 | ||
- npm run lint |
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.
Just noting that those 3 commands were executed concurrently before. I didn't benchmark it but given that we use all 2 cores with unit tests it might have no impact at all :)
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, this is intentional. I checked and it's faster to run these commands sequentially. I also like that it reports lint errors back to the developer really quickly 🙂
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.
Adding fast_finish: true
to the build matrix might also be worthwhile
https://docs.travis-ci.com/user/customizing-the-build/#fast-finishing
"If some rows in the build matrix are allowed to fail, the build won’t be marked as finished until they have completed. To mark the build as finished as soon as possible"
It used to be in WP, but was removed due to a Travis CI bug with the feature:
env: WP_VERSION=latest | ||
before_install: | ||
- nvm install --latest-npm |
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.
Out of curiosity, why is it needed here? Does it override nvm install
from line 26?
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 defining before_install
here overrides the previous before_install
. We need --latest-npm
for this job so that we're using the latest version of npm
. We don't need --latest-npm
in other jobs because ./bin/setup-local-env.sh
will upgrade npm
if necessary.
install: | ||
- ./bin/setup-local-env.sh | ||
script: | ||
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests |
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.
It looks like this logic to running only the subset of test files is something that could be included in wp-scripts test-e2e
by design with a new CLI flag:
https://github.com/WordPress/gutenberg/blob/master/packages/scripts/scripts/test-e2e.js#L45
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.
👍 ab1faa1d7 see below
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.
We aren't able to use npm run test-e2e
here because it uses concurrently
which writes a bunch of extra logging to STDOUT
. We want only the test path names to be written to STDOUT
so that ~/.jest-e2e-tests
contains only test path names. This lets us pipe ~/.jest-e2e-tests
into the next command as an argument.
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, that's correct. Also npm run test-e2e
would run pretest-e2e
which executes npm run build
. In effect, they would run twice.
I was thinking about changing the way the script test-e2e
works when a new flag would be provided. It's a nice task for the future though.
@gziolo Via https://make.wordpress.org/core/2017/05/18/increased-concurrent-jobs-on-travis-ci-builds/
An example, if the WordPress/wordpress-develop repo is running a Travis CI build that will allocate 13 CI jobs, leaving only 2 for Gutenberg here. |
@ntwb thanks for the clarification. It looks like 5 vs 7 doesn't make any difference in such case. I would think we should optimize for the time to finish a single run on Travis. That means, I think we should give this PR a try and see how it goes. @noisysocks do you want to apply any of the suggestions I listed before we proceed? |
Thanks @gziolo! I applied or responded to your suggestions 🙂 |
ab1faa1
to
af4100d
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.
Thanks for your responses. In my opinion, this is good to go. We should iterate further with follow-up PRs.
Some experimenting with optimising our Travis CI configuration to make builds run faster.
I've:
I also cleaned up the Travis CI configuration file and gave each job a name so that it's clearer in the UI what job does what: