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 Tooling: Split JavaScript tasks to individual jobs #15229

Merged
merged 3 commits into from
May 9, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 26, 2019

Related: #15159
Closes #10625

This pull request seeks to update the Travis configuration to run each individual script of the current "JS unit tests" job as its own separate job. As part of this, and as noted by @gziolo at #14289 (comment) , there is an added benefit here in that none of these scripts require npm run build, despite the fact that we run the build task. This has been removed here, and should help avoid any one of these containers from taking a particularly long time to run.

The proposed benefit is:

  • Easier to determine what caused a build to fail
  • Reduce the total time to run all of these tests, as each occurs in their own container
    • Note: There is some added overhead here in that each container must install its own dependencies.

Testing instructions:

Verify the build passes.

@aduth aduth added [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts labels Apr 26, 2019
I thought it redundant. Alas, I was wrong
@danielbachhuber
Copy link
Member

Sweet! Looks like there's still a failure though.

@aduth
Copy link
Member Author

aduth commented Apr 26, 2019

Sweet! Looks like there's still a failure though.

Yeah, it seems the build may be required for the unit tests specifically after all. Or, at least, there's one package which has its own build script which needs to be run to generate an output relied upon by the test:

"scripts": {
"build": "concurrently \"npm run build:js\" \"npm run build:php\"",
"build:js": "pegjs --format commonjs -o ./parser.js ./grammar.pegjs",
"build:php": "node bin/create-php-parser.js"
}

It may be enough to just do npm lerna build as a minimum.

This is accounted for normally in the pre-steps of npm run build:

gutenberg/package.json

Lines 158 to 160 in 87d7fe4

"prebuild:packages": "npm run clean:packages && lerna run build",
"build:packages": "node ./bin/packages/build.js",
"build": "npm run build:packages && wp-scripts build",

@gziolo
Copy link
Member

gziolo commented Apr 27, 2019

Yeah, it seems the build may be required for the unit tests specifically after all. Or, at least, there's one package which has its own build script which needs to be run to generate an output relied upon by the test:

Initially, it was integrated as postinstall step because it used to run builds for all packages. We can try to mirror it by adding postinstall script to the package itself and see if it solves the issue.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I think the initial idea was to have all those tasks in one job to minimize the impact of npm install command and all other bootstrap tasks involved.

before_install:
- nvm install --latest-npm
install:
- npm ci
script:
- npm run build
Copy link
Member

Choose a reason for hiding this comment

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

Yes, ideally we don't need npm run build at all.

@aduth
Copy link
Member Author

aduth commented May 8, 2019

I think the initial idea was to have all those tasks in one job to minimize the impact of npm install command and all other bootstrap tasks involved.

Related: #14353

We removed the postinstall step because it was a slow and often unnecessary hurdle to installing dependencies. I think running postinstall for only custom build scripts of plugins wouldn't be quite so problematic, but it seems a bit puzzling that we'd perform some (but not all) of the build.

I guess it's by virtue of the Jest configuration remapping for built files (combined with babel-jest) that we're able to avoid npm run build at all. But this obviously doesn't cover build tasks not accounted for by Babel.

So it seems we could either:

  1. "Cherry-pick" what we decide to build, i.e. as part of the Travis build, at least make sure custom packages are build (npx lerna run build)
  2. Avoid possibility for overlooked build tasks by running the whole build (npm run build). This would obviously be slower (and partially unnecessary).

@aduth
Copy link
Member Author

aduth commented May 8, 2019

  1. "Cherry-pick" what we decide to build, i.e. as part of the Travis build, at least make sure custom packages are build (npx lerna run build)

This is what I chose with 00358d8

@aduth
Copy link
Member Author

aduth commented May 8, 2019

While performance is not necessarily the primary objective here, some rough metrics look positive:

Before:

  • JS unit tests: 6 min 40 sec

After:

  • Lint: 2 min 3 sec
  • Build artifacts: 1 min 38 sec
  • License compatibility: 1 min 34 sec
  • JavaScript unit tests: 5 min

While it uses 3 additional containers, it also saves roughly 1m40s as far as overall build time for these tasks (by longest runtime).

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

These changes are looking good and especially like the splits which makes for easier "at a glance" feedback on any fails.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This should work as suggested. I'm still not 100% confident that avoiding lerna run build in Gutenberg's postinstall is a good thing overall as it still may cause issues for developers who run npm run test-unit locally. We can tackle it separately as it's not the main goal of this PR.

Let's see how this new setup works in action.

@gziolo gziolo merged commit 8e7f37c into master May 9, 2019
@gziolo gziolo deleted the update/travis-split-js-tasks branch May 9, 2019 11:33
@gziolo
Copy link
Member

gziolo commented May 9, 2019

I have just realized that @wordpress/block-serialization-spec-parser is not used at all in the application. It exists only as a reference package which is used for comparison with other parsers. It is the only package which requires the build step which raises the question if we shouldn't rethink this setup and simplify it. Maybe this build step should be embedded in the test itself to ensure it always operates on the fresh versions of parser files generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it easier for a contributor to determine what failed their build
5 participants