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

Bump node to version 16 #27970

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Conversation

WiNloSt
Copy link
Member

@WiNloSt WiNloSt commented Jan 31, 2023

Why?

  1. Fixes frontend test failures related to nock library
  2. Enforces the same version for dev and production environments

Since the introduction of nock in #27337, we starts to see some inconsistency in the unit tests behavior run on Node v14 vs v16+. This leads to the conclusion that we should be using Node v16+ for both development + CI environment to make the tests more repeatable.

Also, please note that we're using exactly Node 16.10.0 on our CI since a lot of people reported here jestjs/jest#11956. Any other 16 or 18 versions don't seem to work at all.

Example failed runs

Other runs are hard to find since the PR is amended, but there was at least a single commit that I reran the tests a few times and it failed consistently when this couldn't be reproduced on machines using node V16+

@@ -1,4 +1,3 @@
import { XMLHttpRequest } from "xmlhttprequest";
Copy link
Member Author

Choose a reason for hiding this comment

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

This was introduced in #27337, but doesn't seem to be needed to work in Node v16+ anymore

@WiNloSt WiNloSt marked this pull request as ready for review January 31, 2023 11:45
@WiNloSt WiNloSt requested review from ranquild, nemanjaglumac and a team January 31, 2023 11:45
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Base: 66.26% // Head: 66.26% // No change to project coverage 👍

Coverage data is based on head (5ae802f) compared to base (9ef4161).
Patch has no changes to coverable lines.

❗ Current head 5ae802f differs from pull request most recent head 707a478. Consider uploading reports for the commit 707a478 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #27970   +/-   ##
=======================================
  Coverage   66.26%   66.26%           
=======================================
  Files        3283     3283           
  Lines       95174    95174           
  Branches    12141    12141           
=======================================
  Hits        63069    63069           
  Misses      27083    27083           
  Partials     5022     5022           
Flag Coverage Δ
back-end 85.29% <ø> (ø)
front-end 48.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@WiNloSt WiNloSt force-pushed the bump-node-version-to-fix-flaky-unit-test branch from 26cfeea to 0991652 Compare January 31, 2023 13:13
@deploysentinel
Copy link

deploysentinel bot commented Jan 31, 2023

No failed tests 🎉

@WiNloSt WiNloSt force-pushed the bump-node-version-to-fix-flaky-unit-test branch 2 times, most recently from 4cba97c to 82e8b09 Compare January 31, 2023 13:57
@WiNloSt
Copy link
Member Author

WiNloSt commented Jan 31, 2023

There are still problem presents when upgrading node version to 16 or 18.

  1. Only upgrading the Node version will fail the unit tests due to memory error

    FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

  2. Then I tried running it with NODE_OPTIONS="--max-old-space-size=4096" which suppress the error, now the tests don't raise any error, but are frozen instead.

So, there are still some stuff left to investigate about a proper and better fix. Now we'll just delete the unit failed unit tests as a short term solution.

@WiNloSt WiNloSt closed this Jan 31, 2023
@kulyk kulyk deleted the bump-node-version-to-fix-flaky-unit-test branch January 31, 2023 15:13
@kulyk
Copy link
Member

kulyk commented Jan 31, 2023

Oops, I accidentally clicked "delete branch" 🤦‍♂️

@WiNloSt
Copy link
Member Author

WiNloSt commented Jan 31, 2023

@kulyk I was going to do it anyway. No worries!

@WiNloSt WiNloSt restored the bump-node-version-to-fix-flaky-unit-test branch February 1, 2023 15:55
@WiNloSt WiNloSt reopened this Feb 1, 2023
@WiNloSt WiNloSt force-pushed the bump-node-version-to-fix-flaky-unit-test branch 3 times, most recently from e7064de to 3438613 Compare February 1, 2023 20:15
@WiNloSt WiNloSt force-pushed the bump-node-version-to-fix-flaky-unit-test branch from 3438613 to 5ae802f Compare February 1, 2023 20:43
@@ -5,7 +5,7 @@ runs:
- name: Prepare Node.js
uses: actions/setup-node@v3
with:
node-version: 14.x
node-version: 16.10.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Lots of people reported that they need to use exactly version 16.10.0 to not crash on their CI.

Copy link
Member

Choose a reason for hiding this comment

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

I'd maybe add a comment in the code here so we don't forget about this and change it by mistake.
If you decide to do that, please commit with [ci skip] and I'll force-merge it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

Copy link
Member

@nemanjaglumac nemanjaglumac left a comment

Choose a reason for hiding this comment

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

🚀

@nemanjaglumac nemanjaglumac changed the title nock behave differently between v14 and v16+ Bump node to version 16 Feb 2, 2023
@nemanjaglumac nemanjaglumac modified the milestones: 0.45.3, 0.46 Feb 2, 2023
@kulyk kulyk merged commit 10a56f0 into master Feb 2, 2023
@kulyk kulyk deleted the bump-node-version-to-fix-flaky-unit-test branch February 2, 2023 18:42
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.

None yet

4 participants