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

Faster CI #1388

Merged
merged 10 commits into from
Feb 15, 2019
Merged

Faster CI #1388

merged 10 commits into from
Feb 15, 2019

Conversation

redallen
Copy link
Contributor

@redallen redallen commented Feb 14, 2019

What: Revamp Travis to use parallel tasks and add CircleCI to deploy lengthy PR previews instead of Travis for faster PR approvals.

Travis Builds

Before After CircleCI
Build 🏴
Jest tests
Lint
Integration test 🏁⏩
Build Docs 🏁⏩ 🏴
Publish PR Docs 🏴 🏴
Publish Main Docs 🏁 🏁⏩
Lerna publish 🏁 🏁⏩

⏩: In parallel
🏁: If release (commit to master on patternfly/patternfly-react)
🏴: If pull request

Use Travis to mainly handle PR testing. It is still used for deploys (for now). Use CircleCI to deploy PR Docs.

Time comparison

Old Travis New Travis CircleCI
PR Time 28:23 14:12 5:56 to get PR link
Deploy Time 27:25 19:47 N/A

Continuing issues:

  1. yarn install and yarn build still takes 6-9 minutes on Travis. Need to conditionally yarn install based off the checksum of yarn.lock (like with CircleCI) to save 3-4 minutes here. Need to look into cleaning dependencies to save on install time.
  2. Building docs for @patternfly/react-docs takes 8 minutes (or 5-6 on CircleCI). Need to look into optimizing Gatsby.

Additional issues:

  1. Lerna publishing is untested. Should be the same as before since Travis caches all the dist folders, but unfortunately it's impossible to test without doing a release.
  2. The integration test doesn't work on Travis for some unknown reason, so I've disabled it for now.
  3. See how to disable the CircleCI PR block that doesn't need to be there.

@patternfly-build
Copy link
Contributor

Deploy preview for patternfly-react-gone ready!

Built with commit a85ba13

https://deploy-preview-1388--patternfly-react-gone.netlify.com

@redallen
Copy link
Contributor Author

Well, I had to rewrite upload_preview.sh just now, but it's working!

@patternfly-build
Copy link
Contributor

PatternFly-React preview: https://1388-pr-patternfly-react-patternfly.surge.sh

@redallen
Copy link
Contributor Author

This should fix the double preview commenting. As an aside, I have no idea what bot made the netlify upload!

git:
depth: 1
depth: 10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Travis won't trip over itself checking out an old commit after a new build has fired

.travis.yml Show resolved Hide resolved
@@ -1,8 +1,5 @@
{
"lerna": "3.10.5",
"nohoist": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hoisting babel doesn't break any task that I'm aware of

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
"test:current": "jest --watch",
"test:integration": "lerna run integration:setup && lerna run build:demo && lerna run integration:test",
"test:watch": "jest --watchAll",
"travis-deploy-once": "travis-deploy-once"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer need this thanks to Travis build stages

package.json Show resolved Hide resolved
]
},
"resolutions": {
"cssom": "0.3.4"
"cssom": "0.3.4",
"terser": "3.14.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pr-build occasionally fails on CircleCI for no known reason. This might fix it? gatsbyjs/gatsby#11508

@@ -4,40 +4,6 @@
"config": {
"access": "private"
},
"devDependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are ignored by yarn since the package has no version, and there's nothing we can do about it :(

@@ -4,33 +4,12 @@
"config": {
"access": "private"
},
"devDependencies": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are ignored by yarn since the package has no version, and there's nothing we can do about it :(

@@ -32,7 +32,7 @@
"@patternfly/react-styles": "^2.3.2",
"@patternfly/react-tokens": "^2.0.1",
"@tippy.js/react": "^1.1.1",
"emotion": "^10.0.6",
"emotion": "^9.2.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistent emotion version

"build:tsd:copy": "node ./scripts/copyTS.js",
"lint": "tslint --project tsconfig.json",
"postbuild": "yarn build:typecheck && node ./build/copyStyles.js"
"build:ts": "node ./scripts/copyTS.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consistent naming

@coveralls
Copy link

coveralls commented Feb 14, 2019

Pull Request Test Coverage Report for Build 4562

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 38 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.02%) to 78.878%

Files with Coverage Reduction New Missed Lines %
packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js 5 48.81%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownMenu.js 9 46.97%
packages/patternfly-4/react-core/src/components/ApplicationLauncher/ApplicationLauncherMenu.js 11 8.7%
packages/patternfly-4/react-core/src/helpers/util.ts 13 63.1%
Totals Coverage Status
Change from base Build 4507: -0.02%
Covered Lines: 4620
Relevant Lines: 5464

💛 - Coveralls

@@ -37,8 +37,8 @@
"gatsby"
],
"scripts": {
"docbuild": "node build/copyDocs.js && node --max_old_space_size=4096 ./node_modules/.bin/gatsby build",
"pr-build": "node build/copyDocs.js && node --max_old_space_size=4096 ./node_modules/.bin/gatsby build --prefix-paths",
Copy link
Collaborator

Choose a reason for hiding this comment

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

these were added to increase the available memory for the node process as gatsby is sometimes hitting it and then erroring out. Is this not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The module resolution is the bigger problem. If Gatsby gets hoisted, it won't be in that path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we adjust the path to go to the relative root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but could you convince me of when Gatsby could run out of memory? I haven't experienced it on Travis or CircleCI, which have 7.5GB and 4GB of RAM respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm more concerned about fixing this rare Gatsby issue with CircleCI, which happened with and without the --max_old_space flag. Maybe it is related to RAM... any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gatsby definitely uses a lot of ram, but if it runs out it would typically error out like this:

success createPages — 16.948 s
success createPagesStatefully — 0.120 s
success onPreExtractQueries — 0.014 s
success update schema — 6.081 s
success extract queries from components — 0.265 s
⠁
<--- Last few GCs --->

[11566:0x37ab5d0]    73868 ms: Mark-sweep 1416.3 (1495.9) -> 1416.3 (1479.9) MB, 915.0 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 915 ms) last resort
[11566:0x37ab5d0]    74780 ms: Mark-sweep 1416.3 (1479.9) -> 1416.3 (1479.9) MB, 912.6 / 0.0 ms  last resort


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x196898628799 <JSObject>
    0: builtin exit frame: captureStackTrace(aka captureStackTrace)(this=0x393b29c02311 <undefined>,0x2f691ed37cd1 <JSFunction CapturedTrace (sfi = 0x25975f15c491)>,0x328575c09329 <CapturedTrace map = 0x38644cc40001>)

    1: _then [/mnt/c/Users/brand/code/admitbrain/node_modules/bluebird/js/release/promise.js:~219] [pc=0x2c92629e6e1f](this=0x3470fb292b09 <Promise map = 0x38644cc400b1>,didFulfil...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

I have not seen the terser error before though, not too sure where that's coming from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found an environment variable env NODE_OPTIONS=--max_old_space_size=4096, that way a relative path isn't necessary.

Copy link
Collaborator

@jschuler jschuler left a comment

Choose a reason for hiding this comment

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

Nice work @redallen !

@dgutride dgutride merged commit 0e70736 into master Feb 15, 2019
@redallen redallen deleted the feature/circleci branch February 15, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants