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

chore(js): update jest to v24; drop --runInBand #3672

Merged
merged 4 commits into from
Jul 9, 2019
Merged

Conversation

IanLondon
Copy link
Contributor

overview

make test-js is taking ~16.5 minutes on Travis. This is causing some builds to time out and fail. There's a forceful workaround: adding using the travis_wait util to run this step via travis_wait make test-js in .travis.yml will allow the build to not time out.

On my machine, it takes ~1.5 minutes real using yarn jest --runInBand --coverage. But, unlike Travis, I have a jest cache built up locally from running this before. When I add --no-cache and do yarn jest --runInBand --coverage --no-cache, it takes ~10 minutes. It seems like Running coverage on untested files... accounts for the bulk of that difference. And without --coverage, running yarn jest --runInBand --no-cache takes ~1.25 mins. Also, yarn jest --coverage --no-cache (parallelization enabled, no cache) takes only ~1 minute!

In summary: coverage without parallelization is very slow and is only performant if you have a jest cache already built up. So the options are

  1. Enable parallelization (may cause nondeterministic flaky failures in Travis sometimes, that's why we added --runInBand see Flaky failure mode when running tests jestjs/jest#1874 )
  • Try bumping Jest version to 24 and hope the flakiness is fixed
  1. Enable jest caching on Travis
  2. Disable or restrict coverage (even though we seldom use it, seems like a bad idea to turn it off/down!)
  3. Use travis_wait for the make test-js step so Travis doesn't time out. I confirmed that this works on a test branch build. But feels like a band-aid, the make test-js step will still take ~16.5 min this way

This PR takes option 1!

changelog

  • upgrade jest
  • remove --runInBand

review requests

For additional context, see #2815 and jestjs/jest#1874

  1. make install-js to install the new jest
  2. make test-js should pass on your machine
  3. Travis builds should pass and not time out!

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

🚤

@IanLondon
Copy link
Contributor Author

Wow FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory https://travis-ci.org/Opentrons/opentrons/jobs/553813705#L848

@IanLondon IanLondon added DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available WIP and removed ready for review labels Jul 3, 2019
@codecov
Copy link

codecov bot commented Jul 4, 2019

Codecov Report

Merging #3672 into edge will increase coverage by 5.54%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3672      +/-   ##
==========================================
+ Coverage   28.84%   34.38%   +5.54%     
==========================================
  Files         733      714      -19     
  Lines       13313    11173    -2140     
==========================================
+ Hits         3840     3842       +2     
+ Misses       9473     7331    -2142
Impacted Files Coverage Δ
components/src/tooltips/HoverTooltip.js 11.76% <0%> (-1.57%) ⬇️
components/src/forms/SelectField.js 13.63% <0%> (-1.37%) ⬇️
...onents/src/interaction-enhancers/HandleKeypress.js 6.66% <0%> (-1.03%) ⬇️
components/src/deck/Deck.js 7.69% <0%> (-0.31%) ⬇️
...omponents/DeckSetup/LabwareOverlays/LabwareName.js 0% <0%> (ø) ⬆️
protocol-designer/src/analytics/actions.js 0% <0%> (ø) ⬆️
...designer/src/components/FileSidebar/FileSidebar.js 0% <0%> (ø) ⬆️
...onents/analytics-settings/AnalyticsSettingsCard.js 0% <0%> (ø) ⬆️
...p/src/components/TipProbe/InstrumentMovingPanel.js 0% <0%> (ø) ⬆️
app/src/components/MenuPanel/index.js 0% <0%> (ø) ⬆️
... and 391 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea9fb82...ca5652c. Read the comment docs.

Makefile Outdated
--coverage=$(cover) \
--watch=$(watch) \
--updateSnapshot=$(updateSnapshot)
--updateSnapshot=$(updateSnapshot) \
--no-cache=$(if $(CI),true,false) \
Copy link
Contributor

Choose a reason for hiding this comment

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

With #3683 we should see if this cache removal is really necessary. On my Ubuntu VM (2 cores, 4GB memory) I'm seeing:

  • No cache: ~110 seconds
  • Cache enabled, cold: ~110 seconds
  • Cache enabled, subsequent run: ~75 seconds
  • No cache, 2 workers: ~220 seconds
  • Cache enabled, cold, 2 workers: ~85 seconds
  • Cache enabled, subsequent run, 2 workers: ~60 seconds

Given the speedup from caching (understanding that the numbers above are unscientific), I think Travis caching of the jest cache is definitely worth exploring in this PR or a followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One downside is that on Travis, you have to spend time downloading the cache. My local jest cache for the monorepo is 143.5MB, but I'm not sure what that size might be contingent upon

package.json Show resolved Hide resolved
@IanLondon IanLondon added ready for review and removed DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available WIP labels Jul 9, 2019
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

CI is green and make test-js CI=true worked in my testing on macOS, Ubuntu 18.04, and Windows 10. We should see how this shakes out this week and then ping the Jest folks to let them know if the flakiness without --run-in-band is (or isn't) fixed

🚢

@IanLondon IanLondon merged commit ced8878 into edge Jul 9, 2019
@IanLondon IanLondon deleted the js_update-jest-24 branch July 9, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants