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.

Sign up for GitHub

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

Jump to bottom

Improve Travis CI build times #13103

Merged
merged 7 commits into from
Jan 4, 2019
Merged

Improve Travis CI build times #13103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0b2f880
Optimize Travis CI integration
noisysocks Dec 27, 2018
976e8c2
Use `npm ci` instead of `npm install`
noisysocks Dec 28, 2018
c37ba28
Remove unnecessary npm scripts
noisysocks Dec 28, 2018
95f9981
Configure E2E tests to use caching and to not update snapshots
noisysocks Dec 28, 2018
1bb8a1c
Tell jest to not bother with regular expressions
noisysocks Dec 28, 2018
94ffdce
Oops, how'd this get here?
noisysocks Dec 28, 2018
af4100d
Don't provide unnecessary --runInBand argument
noisysocks Jan 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 46 additions & 19 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
dist: trusty

language: php
language: generic

services:
- docker
Expand All @@ -13,56 +13,83 @@ notifications:
cache:
directories:
- $HOME/.composer/cache
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is for older versions of Composer, see WPCS#1435 for reference, so maybe:

-    - $HOME/.composer/cache
+    # Cache directory for older Composer versions.
+    - $HOME/.composer/cache/files
+    # Cache directory for more recent Composer versions.
+    - $HOME/.cache/composer/files

Copy link
Member Author

Choose a reason for hiding this comment

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

I've confirmed that this is correct by running composer config cache-dir in the CI environment and getting /home/travis/.composer/cache.

Copy link
Member

Choose a reason for hiding this comment

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

Right, all 7 CI jobs in this PR use Composer version 1.5.2, the latest is 1.8.0.

When I run $ echo $(composer config cache-dir) on one of my CI jobs, it's using Composer version 1.8.0 and the cache directory is /home/travis/.cache/composer

Bumping the environemnt from Trusty to Xenial might be worth exploring here in a follow up PR

- $HOME/.phpbrew
- $HOME/.jest-cache
- $HOME/.npm

before_install:
- nvm install && nvm use
- npm install npm -g
- $HOME/.nvm/.cache
- $HOME/.phpbrew

branches:
only:
- master

before_install:
- nvm install

jobs:
include:
- stage: test
- name: JS unit tests
env: WP_VERSION=latest
before_install:
- nvm install --latest-npm
Copy link
Member

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?

Copy link
Member Author

@noisysocks noisysocks Jan 2, 2019 •
edited
Loading

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:
- npm ci
script:
- npm install || exit 1
- npm run ci || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this command not useful anymore? should we remove it?

Copy link
Member

Choose a reason for hiding this comment

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

It's no longer used once removed from here

gutenberg/package.json

Line 155 in 7ea5021

"ci": "concurrently \"npm run lint\" \"npm run test-unit:ci\" \"npm run check-local-changes\"",

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like npm run test-unit:ciis also useless? Unless we replace the next line npm run test-unit -- --ci --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache" with it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and stting a --maxWorkers value vs --runInBand can be troublesome on Travis CI

https://jestjs.io/docs/en/troubleshooting#tests-are-extremely-slow-on-docker-and-or-continuous-integration-ci-server

I suspect we have only 2 cores as we use Travis-CI.org rather than Travis-CI.com, some benchmark testing in a follow-up PR would be worthwhile to measure this.

Copy link
Member Author

@noisysocks noisysocks Dec 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we have only 2 cores on our environment.

I experimented with this yesterday and --maxWorkers=2 was slightly faster (about 10 seconds, from memory) on our CI environment. Again, this is something we'll want to re-evaluate periodically as our project evolves.

Will look into removing or changing npm run ci — good catch!

- npm run lint
Copy link
Member

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

Copy link
Member Author

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 🙂

Copy link
Member

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:

https://core.trac.wordpress.org/changeset/35150

- npm run check-local-changes
- npm run test-unit -- --ci --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache"

- stage: test
- name: PHP unit tests (Docker)
env: WP_VERSION=latest DOCKER=true
script:
- ./bin/run-wp-unit-tests.sh

- stage: test
- name: PHP unit tests (PHP 5.6)
language: php
php: 5.6
env: WP_VERSION=latest
script:
- ./bin/run-wp-unit-tests.sh
if: branch = master and type != "pull_request"

- stage: test
php: 7.1
- name: PHP unit tests (PHP 5.3)
env: WP_VERSION=latest SWITCH_TO_PHP=5.3
script:
- ./bin/run-wp-unit-tests.sh
if: branch = master and type != "pull_request"

- stage: test
php: 7.1
- name: PHP unit tests (PHP 5.2)
env: WP_VERSION=latest SWITCH_TO_PHP=5.2
script:
- ./bin/run-wp-unit-tests.sh

- stage: test
- name: E2E tests (Admin with plugins) (1/2)
env: WP_VERSION=latest POPULAR_PLUGINS=true
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 0' < ~/.jest-e2e-tests )

- name: E2E tests (Admin with plugins) (2/2)
env: WP_VERSION=latest POPULAR_PLUGINS=true
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests
Copy link
Member

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

Copy link
Member Author

@noisysocks noisysocks Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

👍 ab1faa1d7 see below

Copy link
Member Author

@noisysocks noisysocks Jan 3, 2019 •
edited
Loading

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.

Copy link
Member

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.

- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 1' < ~/.jest-e2e-tests )

- name: E2E tests (Author without plugins) (1/2)
env: WP_VERSION=latest E2E_ROLE=author
install:
- ./bin/setup-local-env.sh
script:
- ./bin/run-e2e-tests.sh || exit 1
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 0' < ~/.jest-e2e-tests )

- stage: test
- name: E2E tests (Author without plugins) (2/2)
env: WP_VERSION=latest E2E_ROLE=author
install:
- ./bin/setup-local-env.sh
script:
- ./bin/run-e2e-tests.sh || exit 1
- $( npm bin )/jest --config test/e2e/jest.config.json --listTests > ~/.jest-e2e-tests
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 1' < ~/.jest-e2e-tests )
2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@
"check-licenses": "concurrently \"wp-scripts check-licenses --prod --gpl2\" \"wp-scripts check-licenses --dev\"",
"precheck-local-changes": "npm run docs:build",
"check-local-changes": "( git diff -U0 | xargs -0 node bin/process-git-diff ) || ( echo \"There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!\" && exit 1 );",
"ci": "concurrently \"npm run lint\" \"npm run test-unit:ci\" \"npm run check-local-changes\"",
"predev": "npm run check-engines",
"dev": "npm run build:packages && concurrently \"cross-env webpack --watch\" \"npm run dev:packages\"",
"dev:packages": "node ./bin/packages/watch.js",
Expand Down Expand Up @@ -183,7 +182,6 @@
"test-unit": "wp-scripts test-unit-js --config test/unit/jest.config.json",
"test-unit:update": "npm run test-unit -- --updateSnapshot",
"test-unit:watch": "npm run test-unit -- --watch",
"test-unit:ci": "npm run test-unit -- --ci --runInBand",
"test-unit-php": "docker-compose run --rm wordpress_phpunit phpunit",
"test-unit-php-multisite": "docker-compose run -e WP_MULTISITE=1 --rm wordpress_phpunit phpunit"
},
Expand Down