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

Framework: Improve Travis build performance #14289

Closed
wants to merge 9 commits into from
Closed

Framework: Improve Travis build performance #14289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
33cd071
Framework: Run Travis JS Unit Test tasks concurrently
aduth Mar 6, 2019
ef23f7b
Framework: Remove postinstall script
aduth Mar 6, 2019
f4c926a
Framework: Avoid npm install / build in PHP unit tests
aduth Mar 6, 2019
d69d056
Framework: Run build for JS unit tests
aduth Mar 6, 2019
732f500
Testing: Ensure directory exists before writing in ensure_editor_styles
aduth Mar 6, 2019
651d562
Testing: Remove core-block-theme-test.php
aduth Mar 6, 2019
7f31ee4
Testing: mkdir recursive
aduth Mar 6, 2019
da40885
Testing: mkdir the dir, not the path
aduth Mar 6, 2019
6379756
Testing: Fix docker-compose run arg order
aduth Mar 6, 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
9 changes: 6 additions & 3 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@ jobs:
install:
- npm ci
script:
- npm run lint
- npm run check-local-changes
- npm run test-unit -- --ci --maxWorkers=2 --cacheDirectory="$HOME/.jest-cache"
- npm run build
Copy link
Member

@gziolo gziolo Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In #14432 I'm proposing changes which would ensure that we always use the source code to run unit tests. If that change lands, we could do one of the following:

  1. remove npm run build altogether from this job
  2. add a flag which will allow Jest config to remove override which ensures that build folders aren't used

As far as I remember, if we do (2) we would have quite good coverage for all our codebase both transpiled with Babel and original source code. In development, we would always use source code and on Travis we would use setup closer to what you have when using code from npm.

- $( npm bin )/concurrently --kill-others-on-fail \
"npm run lint" \
"npm run check-local-changes" \
"npm run check-licenses" \
"npm run test-unit -- --ci --maxWorkers=2 --cacheDirectory=\"$HOME/.jest-cache\""
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be interested in seeing what putting npm run lint, npm run check-local-changes, and npm run check-licenses into their own job adds to the build times. I don't like the idea of doing concurrently here because it means for someone relying on travis has no insight into whether the subsequent tasks would pass or not.

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'd be interested in seeing what putting npm run lint, npm run check-local-changes, and npm run check-licenses into their own job adds to the build times.

Yeah, this is what prompted my most recent thread comments in Slack. I agree that ideally they'd be defined as separate jobs, but it's not clear how to do that without increasing build times (kinda contrary to the point of the pull request).

On the general topic of "making sense of the build output", there might be some other options to explore for organizing the results in a way which is more easy to read. Right now, the concurrent output is a bit of a mess, since it just spews everything to stdout in the order it's received (i.e. parallel scripts each with their own outputs are intermixed). I think it comes down to: Can we leverage / find new / develop the tool to organize the output in a more readable fashion?

I don't like the idea of doing concurrently here because it means for someone relying on travis has no insight into whether the subsequent tasks would pass or not.

I guess it depends if it's more in the interest of the developer to have faster build times, or a more thorough report of every issue in the build if there are multiple issues. I don't really have a strong leaning one way or the other, but I'd be inclined to think the former would benefit more people in the vast majority of cases.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tested with --maxWorkers=4? Travis VMs have two CPUs available, so it's usually a good idea to have more workers than that, so the CPUs are fully utilised: the workers will likely have I/O wait time to deal with.


- name: PHP unit tests (Docker)
env: WP_VERSION=latest DOCKER=true
Expand Down
8 changes: 3 additions & 5 deletions bin/run-wp-unit-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,8 @@ else
# Run the build because otherwise there will be a bunch of warnings about
# failed `stat` calls from `filemtime()`.
composer install || exit 1
npm install || exit 1
fi

npm run build || exit 1

echo Running with the following versions:
if [[ $DOCKER = "true" ]]; then
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm wordpress_phpunit php -v
Expand All @@ -32,8 +29,9 @@ fi

# Run PHPUnit tests
if [[ $DOCKER = "true" ]]; then
npm run test-php || exit 1
npm run test-unit-php-multisite || exit 1
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm composer run-script lint
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for calling those commands directly rather than continue using npm run *?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the reason for calling those commands directly rather than continue using npm run *?

I guess I assumed if we weren't installing dependencies, we shouldn't run npm scripts. In retrospect, it's probably not strictly a problem. Overall, it's part of a move of "we don't need NPM (or even Node) at all here", maybe even opening up future possibilities to avoid having it be installed in the environment at all.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, should we keep those npm scripts moving forward? There might be value in it but I'm afraid that at some point they will diverge from what is added to Travis config.

Copy link
Member Author

@aduth aduth Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good, should we keep those npm scripts moving forward? There might be value in it but I'm afraid that at some point they will diverge from what is added to Travis config.

Agreed on the concern. I know I've used the NPM script variants, likely out of convenience / familiarity over what would be the docker-compose equivalent, but I'm not overly compelled that they should stay. There's some nice uniformity on the developer experience front to have all test commands available in a single location. The Travis environment is the exception here.

docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm wordpress_phpunit phpunit
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -e WP_MULTISITE=1 wordpress_phpunit phpunit
else
phpunit || exit 1
WP_MULTISITE=1 phpunit || exit 1
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@
"lint-css": "wp-scripts lint-style '**/*.scss'",
"lint-css:fix": "npm run lint-css -- --fix",
"package-plugin": "./bin/build-plugin-zip.sh",
"postinstall": "npm run check-licenses && npm run build:packages",
"pot-to-php": "./bin/pot-to-php.js",
"precommit": "lint-staged",
"publish:check": "npm run build:packages && lerna updated",
Expand Down
106 changes: 0 additions & 106 deletions phpunit/class-core-block-theme-test.php
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions phpunit/class-extend-styles-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ protected function ensure_editor_styles( $should_exist = true ) {
$this->original_file = $path . '.bak';
}
} elseif ( $should_exist ) {
// Ensure directory exists before writing style contents, since
// it's likely if this is reached there's no build directory.
if ( ! is_dir( dirname( $path ) ) ) {
mkdir( dirname( $path ), 0777, true );
}

$this->style_contents = '';
file_put_contents( $path, $this->style_contents );
$this->original_file = null;
Expand Down