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(deps-dev): update to node 16 #5343

Merged
merged 14 commits into from
Apr 29, 2022
Merged

chore(deps-dev): update to node 16 #5343

merged 14 commits into from
Apr 29, 2022

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Apr 8, 2022

Details

This PR bumps our default/recommended build environment to Node 16.

There aren't any real breaking changes affecting us here, except that our unit tests appear to be much slower (timing out after 10m instead of taking 6-7m in Node 14). We could work around this by just sharding the unit tests like we do for e2e tests already, but it's not ideal; besides "still being slower for local development", it also breaks code coverage reporting (unless we add some new job to merge the results before publishing coverage).

The most relevant looking Jest/Node/V8 issues seem to be covered in the discussion of jestjs/jest#11956 - it would be good to experiment with some of the options on that thread to see if we can get the unit test time back down without needing to shard them. Some options to explore:

  • Test Node 16.10.0 vs 16.11.0 (we wouldn't want to pin to 16.10.0, but it would be good to try it out to verify that the slowness really is caused by the V8 update in question on that issue)
  • Try out the assorted node/v8 command line argument combinations suggested by that thread

Alternatively, we could explore more generic ways of making the tests faster; I think https://github.com/nicolo-ribaudo/jest-light-runner would be promising to look into.

Motivation

Keep dependencies up to date; 16 has been an LTS release for several months now.

Also, accessibility-insights-service updated to Node 16 yesterday, and we want to ensure that the packages we publish for it to consume are tested in the same environment.

Context

n/a

Pull request checklist

  • [n/a] Addresses an existing issue: #0000
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • [n/a] Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@dbjorge dbjorge requested a review from a team as a code owner April 8, 2022 23:24
@dbjorge dbjorge marked this pull request as draft April 8, 2022 23:25
@karanbirsingh
Copy link
Contributor

karanbirsingh commented Apr 26, 2022

I'm looking into the test timing (only looking at unit tests in following experiments):

  • This PR shards and brings unit tests to a similar length as e2e (2 jobs, 6:35 and 4:50)
  • Sharding with 16.10.0 does speed things up. The sharded unit-test steps finished in 2:19 and 2:40.
  • No sharding with 16.10.0 shows a step time of 5:45.
  • No sharding with 16.14.2, add no additional flags (baseline, what Dan started with). Unit test step timed out at 10 min (635 tests completed).
  • No sharding with 16.14.2, log heap usage of baseline. Substantial memory use. Unit test step timed out at 10 min (226 tests completed).
  • No sharding with 16.14.2, try adding no-compilation-cache alongside expose-gc / logHeapUsage. Reasonable memory use. Unit test step timed out at 10 min (443 tests completed).
  • No sharding with 16.14.2, try adding no-compilation-cache alone. Unit test step timed out at 10 min (744 tests completed).
  • No sharding with 16.14.2, try adding no-compilation-cache alone while removing old-max-space-size. Unit test step finishes just at 10 minutes (784 tests completed).
  • No sharding with 16.14.2 with just removing old-max-space-size. Unit test step times out at 10 minutes (727 tests completed).

I didn't try the vm.compileScript / vm.Script patch (Not sure if we'd want a patch & unclear whether it'd help with execution time). Jest is unable to mainline this patch because of jestjs/jest#12205 (comment)

I haven't tried the jest-light-runner yet.

I think we may want to go with sharding for now (possibly removing max-old-space-size & adding no-compilation-cache). Pinning to 16.10.0 won't work for long.

@ghost
Copy link

ghost commented Apr 29, 2022

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ dbjorge sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@dbjorge dbjorge marked this pull request as ready for review April 29, 2022 21:48
@dbjorge
Copy link
Contributor Author

dbjorge commented Apr 29, 2022

Thanks @karanbirsingh for recording the different test cases! I've implemented a "sharding + remove max-old-space-size" approach (this involved adding a new separate job for publishing merged code coverage results).

Even with these mitigations in place, it seems inconsistent about how quick it is. This most recent run shows a comparable time (sharded) to Node 14 (without sharding), but the checks against 5128d7e showed the unit tests taking ~8m per job.

@dbjorge dbjorge merged commit 83aa914 into microsoft:main Apr 29, 2022
@dbjorge dbjorge deleted the node-16 branch April 29, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants