Skip to content

fix: CI flakey unit tests TypeErrors on react-native/jest/setup.js global.performance#8046

Merged
cortisiko merged 9 commits into
mainfrom
fix/unbreak-ci-jest-node
Dec 8, 2023
Merged

fix: CI flakey unit tests TypeErrors on react-native/jest/setup.js global.performance#8046
cortisiko merged 9 commits into
mainfrom
fix/unbreak-ci-jest-node

Conversation

@leotm

@leotm leotm commented Dec 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Fix recent CI flakey failing unit tests:
TypeError: Cannot assign to read only property 'performance' of object '[object global]'

Failing at: https://github.com/facebook/react-native/blob/v0.71.14/jest/setup.js#L20-L22
Nb: https://github.com/facebook/react-native/blob/v0.72.0/jest/setup.js 404 file no longer exists in this future RN ver

Reconsider current node/jest/RN vers we're using (see issue and comment below)

it broke between node 18.18 and 18.19 - Jest relies on descriptor.writable but it's not longer returned in the descriptor

Nb: Node 18.19 changelog

Related issues

Fixes: #8051

Manual testing steps

Run unit tests

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions

github-actions Bot commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@leotm

leotm commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

some unit tests passing now: 6/9/10 in f3ea456

@leotm leotm mentioned this pull request Dec 8, 2023
Closed
13 tasks
@leotm leotm marked this pull request as ready for review December 8, 2023 00:09
@leotm leotm requested a review from a team as a code owner December 8, 2023 00:09
@github-actions

github-actions Bot commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/72b88636-8bdd-427a-83f3-eb4b5c648f26
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@leotm leotm changed the title fix: patch react-native/jest/setup.js fix: CI unit tests, patch react-native/jest/setup.js global.performance Dec 8, 2023
@codecov-commenter

codecov-commenter commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c482c7d) 37.06% compared to head (10794b1) 37.06%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #8046   +/-   ##
=======================================
  Coverage   37.06%   37.06%           
=======================================
  Files        1135     1135           
  Lines       29150    29150           
  Branches     2728     2728           
=======================================
  Hits        10803    10803           
  Misses      17724    17724           
  Partials      623      623

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leotm

leotm commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

just when appeared fixed in 4711633 ✅ CI passing

the last 2 commits resulting in no change indicate something else is flakey :suspect: TBC

@leotm leotm marked this pull request as draft December 8, 2023 12:12
@leotm

leotm commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

converted back to draft until CI flakiness resolved
solution appears to be mix of correct:

  • jest monorepo ver (current below)
    • "jest": "^28.1.3",
      - "jest-junit": "^15.0.0"
      - "babel-jest": "^28.1.3",
      - "@types/jest": "^28.1.8",
  • node ver (current below)
    • package.json > engines > ^18.17.1
    • .nvmrc v18
  • RN ver (current below)
    • "react-native": "0.71.14",
    • react-native/jest/setup.js removed in RN 0.72
  • react-native/jest/setup.js patch on global.performance, if still needed
  • disabling unit tests 1by1 (based on fail history) if rly needed
    • but as @cryptotavares mentioned, these already passing when run 1by1

so node's perf API global prop writable descriptor is defined (not in node v18.19.0)
which our ver of jest-environment-node (v28.1.3) expects
since our RN ver (v0.71.14) attempts to assign (not define) global.performance.now during jest setup

consider bumping jest monorepo devDeps to ^29.2.1 for official RN 0.71.14 compat
https://github.com/facebook/react-native/blob/v0.71.14/template/package.json#L27

nb: the flakey TypeError is unrelated to SES

leotm added 2 commits December 8, 2023 12:52
@leotm
Bump/restrict Node package.json engine (^18.17.1 to ~18.18.2)
fd4e8e3 
I.e. don't use Node v18.19, since global.performance's writable descriptor isn't defined
@leotm
Restrict .nvmrc Node version from 18 to 18.18
794e6bb 
I.e. don't use Node v18.19, since global.performance's writable descriptor isn't defined
(No need for "v" prefix: https://github.com/nvm-sh/nvm#nvmrc)
@leotm leotm force-pushed the fix/unbreak-ci-jest-node branch from a162985 to 794e6bb Compare December 8, 2023 12:53
@leotm leotm marked this pull request as ready for review December 8, 2023 13:31
@github-actions

github-actions Bot commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/16c6de0b-876a-47d6-9759-8531d9e8321b
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@leotm

leotm commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

can confirm CI breaking on Node 18.19.0
https://github.com/MetaMask/metamask-mobile/actions/runs/7135792916/job/19433229298#step:3:11

passing on 18.18.2
https://github.com/MetaMask/metamask-mobile/actions/runs/7135792916/job/19433229559#step:3:11

final minimal solution: RN patch reverted, Node vers restricted so CI no longer flakey

future consideration: upgrading to 18.19+ likely requires bumping jest and/or RN as mentioned above

@leotm leotm requested a review from Cal-L December 8, 2023 13:36
@leotm leotm changed the title fix: CI unit tests, patch react-native/jest/setup.js global.performance fix: CI unit tests TypeErrors on react-native/jest/setup.js global.performance Dec 8, 2023
@leotm leotm changed the title fix: CI unit tests TypeErrors on react-native/jest/setup.js global.performance fix: CI flakey unit tests TypeErrors on react-native/jest/setup.js global.performance Dec 8, 2023
@leotm leotm force-pushed the fix/unbreak-ci-jest-node branch from ab43449 to 10794b1 Compare December 8, 2023 13:56
@leotm leotm mentioned this pull request Dec 8, 2023
Merged
13 tasks
@sonarqubecloud

sonarqubecloud Bot commented Dec 8, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@leotm

leotm commented Dec 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/16c6de0b-876a-47d6-9759-8531d9e8321b You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

unit tests passing, Bitrise E2E git clones failing :suspect: re-running failed jobs w remote access

@leotm

leotm commented Dec 8, 2023

Copy link
Copy Markdown
Contributor Author

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/16c6de0b-876a-47d6-9759-8531d9e8321b You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

unit tests passing, Bitrise E2E git clones failing :suspect: re-running failed jobs w remote access

ios_e2e_test ✅
android_e2e_test ✅

@cortisiko cortisiko merged commit 9196c02 into main Dec 8, 2023
@cortisiko cortisiko deleted the fix/unbreak-ci-jest-node branch December 8, 2023 19:05
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 8, 2023
@metamaskbot metamaskbot added the release-7.14.0 Issue or pull request that will be included in release 7.14.0 label Dec 8, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-platform Mobile Platform team label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cortisiko cortisiko cortisiko approved these changes

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

@sethkfman sethkfman Awaiting requested review from sethkfman

@Cal-L Cal-L Awaiting requested review from Cal-L

Assignees

No one assigned

Labels

release-7.14.0 Issue or pull request that will be included in release 7.14.0 team-mobile-platform Mobile Platform team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flakey CI unit tests

5 participants

@leotm @codecov-commenter @cortisiko @gauthierpetetin @metamaskbot