-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
I thought it redundant. Alas, I was wrong
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: gutenberg/packages/block-serialization-spec-parser/package.json Lines 29 to 33 in 87d7fe4
It may be enough to just do This is accounted for normally in the pre-steps of Lines 158 to 160 in 87d7fe4
|
Initially, it was integrated as |
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.
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 |
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, ideally we don't need npm run build
at all.
Related: #14353 We removed the I guess it's by virtue of the Jest configuration remapping for built files (combined with So it seems we could either:
|
This is what I chose with 00358d8 |
While performance is not necessarily the primary objective here, some rough metrics look positive:
Before:
After:
While it uses 3 additional containers, it also saves roughly 1m40s as far as overall build time for these tasks (by longest runtime). |
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.
These changes are looking good and especially like the splits which makes for easier "at a glance" feedback on any fails.
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.
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.
I have just realized that |
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:
Testing instructions:
Verify the build passes.