chore: runtime upgrade to v18.15.0 #12243

ovasdi · 2023-04-13T20:29:12Z

The type of this PR is: Chore

This PR solves PLATFORM-5064

Description

This upgrades the project to run on v18.15.0.

Other notable changes:

remove pyton from docker image
upgrade babel-loader
upgrade jest and set worker limit to 500MB to workaround heap leak affecting Node JS 16.11.0+
fix various test failures following runtime and jest upgrades

Note that CI test job run appears to have increased by ~30s.

More details in comments.

Review app

ovasdi · 2023-04-13T20:38:08Z

Dockerfile

@@ -65,8 +65,7 @@ RUN apk --no-cache --quiet add \
  bash \
  build-base \
  curl \
-  git \
-  python


python2 package is no longer provided in Alpine (as of 3.12) (python3 is available).
Not clear to me why this was added but it may be OK to get rid of it now. Old commit that added python.

ovasdi · 2023-04-13T20:41:34Z

package.json

@@ -277,7 +277,7 @@
    "@typescript-eslint/parser": "4.18.0",
    "@wojtekmaj/enzyme-adapter-react-17": "0.6.6",
    "babel-jest": "^28.1.3",
-    "babel-loader": "8.0.6",
+    "babel-loader": "8.2.3",


Node +v17 closed a security hole with SSL and I saw the following error:

#30 7.854 Module build failed (from ./node_modules/babel-loader/lib/index.js): #30 7.854 Error: error:0308010C:digital envelope routines::unsupported #30 7.854 at new Hash (node:internal/crypto/hash:71:19) #30 7.854 at Object.createHash (node:crypto:133:10) #30 7.854 at filename (/app/node_modules/babel-loader/lib/cache.js:94:23) #30 7.854 at /app/node_modules/babel-loader/lib/cache.js:122:39 ...

Upgrading babel-loader to v8.2.3 resolved this.

ovasdi · 2023-04-13T20:45:39Z

src/Utils/Hooks/useAuthValidation.ts

@@ -17,7 +17,7 @@ export const useAuthValidation = () => {
          }
        `,
        {}
-      ).toPromise()
+      )?.toPromise()


Resolves TypeError: Cannot read properties of undefined (reading 'toPromise').

This should always resolve correctly vs being guarded since its on the critical path; where was the error coming from?

I just tested this again, running node v18.15.0 and latest version of Jest.

As suggested I ran yarn relay tho there were no changes to generated files.

Full trace

➜ yarn jest src/Apps/Components/__tests__/AppShell.jest.tsx Browserslist: caniuse-lite is outdated. Please run: npx browserslist@latest --update-db Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating /Users/lalalala/code/force/src/Utils/Hooks/useAuthValidation.ts:37 return (0, _reactRelay.fetchQuery)(relayEnvironment, _useAuthValidationQuery !== void 0 ? _useAuthValidationQuery : (_useAuthValidationQuery = require("../../__generated__/useAuthValidationQuery.graphql"), _useAuthValidationQuery.hash && _useAuthValidationQuery.hash !== "2f2ccc765f2b0623e0c21ddb7e46eb08" && console.error("The definition of 'useAuthValidationQuery' appears to have changed. Run `relay-compiler` to update the generated files to receive the expected data."), _useAuthValidationQuery), {}).toPromise(); ^ TypeError: Cannot read properties of undefined (reading 'toPromise') at _callee$ (/Users/lalalala/code/force/src/Utils/Hooks/useAuthValidation.ts:37:522) at tryCatch (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:45:40) at Generator.invoke [as _invoke] (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:274:22) at Generator.prototype.<computed> [as next] (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:97:21) at asyncGeneratorStep (/Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24) at _next (/Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9) at /Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:32:7 at new Promise (<anonymous>) at /Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:21:12 at exec (/Users/lalalala/code/force/src/Utils/Hooks/useAuthValidation.ts:73:21) at /Users/lalalala/code/force/src/Utils/Hooks/useAuthValidation.ts:78:5 at invokePassiveEffectCreate (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:23487:20) at HTMLUnknownElement.callCallback (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:3945:14) at HTMLUnknownElement.callTheUserObjectsOperation (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/generated/EventListener.js:26:30) at innerInvokeEventListeners (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:350:25) at invokeEventListeners (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:286:3) at HTMLUnknownElementImpl._dispatch (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:233:9) at HTMLUnknownElementImpl.dispatchEvent (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:104:17) at HTMLUnknownElement.dispatchEvent (/Users/lalalala/code/force/node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:241:34) at Object.invokeGuardedCallbackDev (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:3994:16) at invokeGuardedCallback (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:4056:31) at flushPassiveEffectsImpl (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:23574:9) at unstable_runWithPriority (/Users/lalalala/code/force/node_modules/scheduler/cjs/scheduler.development.js:468:12) at runWithPriority$1 (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:11276:10) at flushPassiveEffects (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:23447:14) at performSyncWorkOnRoot (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:22269:3) at /Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:11327:26 at unstable_runWithPriority (/Users/lalalala/code/force/node_modules/scheduler/cjs/scheduler.development.js:468:12) at runWithPriority$1 (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:11276:10) at flushSyncCallbackQueueImpl (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:11322:9) at flushSyncCallbackQueue (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:11309:3) at batchedUpdates$1 (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom.development.js:22387:7) at act (/Users/lalalala/code/force/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1042:14) at render (/Users/lalalala/code/force/node_modules/@testing-library/react/dist/pure.js:97:26) at _callee$ (/Users/lalalala/code/force/src/Apps/Components/__tests__/AppShell.jest.tsx:46:5) at tryCatch (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:45:40) at Generator.invoke [as _invoke] (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:274:22) at Generator.prototype.<computed> [as next] (/Users/lalalala/code/force/node_modules/regenerator-runtime/runtime.js:97:21) at asyncGeneratorStep (/Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24) at _next (/Users/lalalala/code/force/node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9) at processTicksAndRejections (node:internal/process/task_queues:95:5) Node.js v18.15.0 FAIL src/Apps/Components/__tests__/AppShell.jest.tsx ● Test suite failed to run Jest worker encountered 4 child process exceptions, exceeding retry limit at ChildProcessWorker.initialize (node_modules/jest-cli/node_modules/jest-worker/build/workers/ChildProcessWorker.js:181:21) Test Suites: 1 failed, 1 total Tests: 0 total Snapshots: 0 total Time: 25.061 s Ran all test suites matching /src\/Apps\/Components\/__tests__\/AppShell.jest.tsx/i. error Command failed with exit code 1.

Test passed with the change discussed here.

Can we remove the question mark here? This should fail loudly if for some reason this object is null (which it should never be)

ovasdi · 2023-04-13T20:46:52Z

src/Apps/Order/Components/__tests__/BankAccountPicker.jest.tsx

@@ -205,7 +205,7 @@ describe("BankAccountFragmentContainer", () => {
        }),
      })
      const page = new BankAccountPickerTestPage(wrapper)
-      page.clickRadio(3)
+      page.clickRadio(2)


Resolves Error: Method “simulate” is meant to be run on 1 node. 0 found instead.

ovasdi · 2023-04-13T20:51:20Z

src/Apps/PriceDatabase/__tests__/PriceDatabaseApp.jest.tsx

-      <HeadProvider>
-        <PriceDatabase />
-      </HeadProvider>
+      <SystemContextProvider relayEnvironment={createMockEnvironment() as any}>


This test was failing with:

RUNS src/Apps/PriceDatabase/__tests__/PriceDatabaseApp.jest.tsx /Users/ozzievasdi/code/force/node_modules/relay-runtime/lib/query/fetchQueryInternal.js:268 requestCachesByEnvironment.set(environment, requestCache); ^ TypeError: Invalid value used as weak map key

After debugging I discovered that the relayEnvironment was not mocked out and failed this test. Im not clear on why this was working in v14 and doesn't in v18, perhaps due to a more strict check in more recent versions? In any case, providing context with relayEnvironment made this test pass.

This works fine, but can also use <MockBoot relayEnvironment={...}> as that's more idiomatic (and will remove the need for any)

Done in eeb9658

scripts/jest.sh

…runtime-upgrade

ovasdi · 2023-04-19T13:39:32Z

package.json

@@ -313,8 +313,8 @@
    "husky": "3.0.5",
    "immer": "9.0.7",
    "inquirer": "1.2.3",
-    "jest": "^28.1.3",
-    "jest-environment-jsdom": "^28.1.3",
+    "jest": "^29.5.0",


To resolve the issue outlined in this comment jest version was upgraded and a new config option is now used (available starting v29+) that limits jest worker memory.

ovasdi · 2023-04-19T13:48:59Z

These two commits: e83ce71, 1a88b3f address the many snapshot related failures that came up after jest upgrade.

expect(received).toMatchInlineSnapshot(snapshot)
...
    - Array [
    -   Object {
    + [
    +   {

There were a number of snapshot related changes in v29 and alpha releases. I wasn't able to pin point which one introduced the breaking change.

Re-recording the failing snapshots using --updateSnapshot flag seems to have fixed the issue. For an example of failures, see this run.

ovasdi · 2023-04-19T14:00:33Z

jest.config.js

@@ -27,4 +27,5 @@ module.exports = {
      ? ["@swc/jest", swcConfig]
      : "babel-jest",
  },
+  workerIdleMemoryLimit: "500MB",


I looked at a few jobs and noticed that in CI the workers end up consuming ~300MB across the test containers. I figured to try 500MB. I tried decreasing but it didn't seem to make a difference locally so I kept it at 500MB tho there may be more to do as far as optimizing this number.

Note that Jest advises to use units to denote the limit when running tests in CircleCI using Linux workers.

ovasdi · 2023-04-19T14:27:23Z

Jest v29.4.3 fixed/changed how restoreAllMocks() and resetAllMocks() behave. There are a number of tests that use these methods. I noticed that a few tests were generating a lot of console.error noise, resulting in a 400000 character output during test runs.

example error from this run

console.error
      Warning: Relay: Relay Container for fragment `SaveButton_artwork` has missing data and would suspend. When using features such as @defer or @module, use `useFragment` instead of a Relay Container.

This issue was addressed in 4aa7c72. Most noise came from src/Components/ArtworkFilter/__tests__/ArtworkFilter.jest.tsx.

ovasdi · 2023-04-19T14:42:44Z

cc @artsy/grow-devs

ovasdi · 2023-04-20T14:06:46Z

Added review app: https://runtime-upgrade.artsy.net/

damassi

One change request, otherwise LGTM 👍

- #12243 (comment)

…runtime-upgrade

ovasdi added 5 commits April 12, 2023 09:30

chore: remove python from dockerfile

97c4e0e

chore: upgrade node runtime, babel loader

029cca8

chore: update versions across configs

c254bef

fix: tests that errored following version upgrade

0b615cb

feat: add --no-compilation-cache flag to jest.sh

0c0feb7

artsy-peril bot added the Jira Synced label Apr 13, 2023

ovasdi self-assigned this Apr 13, 2023

ovasdi commented Apr 13, 2023

View reviewed changes

scripts/jest.sh Outdated Show resolved Hide resolved

ovasdi and others added 9 commits April 14, 2023 11:53

chore: upgrade jest to latest

e5291e2

fix: snapshot failed errors

e83ce71

feat: run jest with worker memory limit at 500MB

665e02d

fix snapshot failed errors

1a88b3f

refactor: switch test to use MockBoot

eeb9658

test(authenticationRoutes): fix failing tests

51b61fd

test: fix failing Counter suite

0bd3974

test: adjust tests to remedy lots of console errors

4aa7c72

Merge remote-tracking branch 'origin/main' into ovasdi/PLATFORM-5064/…

a590f04

…runtime-upgrade

ovasdi commented Apr 19, 2023

View reviewed changes

ovasdi requested a review from a team April 19, 2023 14:42

ovasdi marked this pull request as ready for review April 19, 2023 14:42

ovasdi requested a review from damassi April 19, 2023 14:42

damassi previously approved these changes Apr 24, 2023

View reviewed changes

ovasdi added 2 commits April 24, 2023 11:48

refactor: remove ? from useAuthValidation.ts as requested

cbb2c51

- #12243 (comment)

test: mock useAuthValidation in AppShell test to fix error

4d4e703

ovasdi dismissed damassi’s stale review via 4d4e703 April 24, 2023 15:56

damassi added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Apr 24, 2023

ovasdi removed the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Apr 24, 2023

Merge remote-tracking branch 'origin/main' into ovasdi/PLATFORM-5064/…

adbba74

…runtime-upgrade

ovasdi added Do not merge and removed Do not merge labels Apr 24, 2023

ovasdi merged commit 7a85ea1 into main Apr 24, 2023
11 checks passed

ovasdi deleted the ovasdi/PLATFORM-5064/runtime-upgrade branch April 24, 2023 17:37

artsy-peril bot mentioned this pull request Apr 24, 2023

Deploy #12296

Merged

This was referenced Apr 25, 2023

test: skip cypress settings redirect #12300

Merged

refactor: add timeout to cypress install #12311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: runtime upgrade to v18.15.0 #12243

chore: runtime upgrade to v18.15.0 #12243

ovasdi commented Apr 13, 2023 •

edited by jira bot

ovasdi Apr 13, 2023

ovasdi Apr 13, 2023

ovasdi Apr 13, 2023

damassi Apr 14, 2023

ovasdi Apr 14, 2023

damassi Apr 24, 2023

ovasdi Apr 13, 2023

ovasdi Apr 13, 2023

damassi Apr 14, 2023

ovasdi Apr 19, 2023

ovasdi Apr 19, 2023

ovasdi commented Apr 19, 2023 •

edited

ovasdi Apr 19, 2023 •

edited

ovasdi commented Apr 19, 2023

ovasdi commented Apr 19, 2023

ovasdi commented Apr 20, 2023

damassi left a comment

chore: runtime upgrade to v18.15.0 #12243

chore: runtime upgrade to v18.15.0 #12243

Conversation

ovasdi commented Apr 13, 2023 • edited by jira bot

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ovasdi commented Apr 19, 2023 • edited

ovasdi Apr 19, 2023 • edited

Choose a reason for hiding this comment

ovasdi commented Apr 19, 2023

ovasdi commented Apr 19, 2023

ovasdi commented Apr 20, 2023

damassi left a comment

Choose a reason for hiding this comment

ovasdi commented Apr 13, 2023 •

edited by jira bot

ovasdi commented Apr 19, 2023 •

edited

ovasdi Apr 19, 2023 •

edited