Skip to content

refactor(tests): remove tree.root traversal anti-pattern from RN test suite#1184

Closed
georgewrmarshall wants to merge 4 commits into
mainfrom
refactor/remove-tree-root-test-traversal
Closed

refactor(tests): remove tree.root traversal anti-pattern from RN test suite#1184
georgewrmarshall wants to merge 4 commits into
mainfrom
refactor/remove-tree-root-test-traversal

Conversation

@georgewrmarshall

@georgewrmarshall georgewrmarshall commented May 21, 2026

Copy link
Copy Markdown
Contributor

Description

This PR prepares the React Native test suite for the upcoming React Native Testing Library v14 upgrade. It is intentionally staying as a draft and is not ready to merge until the RNTL v14 upgrade path is finalized.

The branch updates the native test setup to use @testing-library/react-native@14.0.0-rc.3 and replaces react-test-renderer with test-renderer where RNTL v14 requires it. It also runs the RNTL v14 async migration across @metamask/design-system-react-native tests, so render, renderHook, fireEvent, and related assertions are awaited where needed.

This also continues the cleanup from #1182: tests avoid tree.root, react-test-renderer, UNSAFE_*, and direct composite prop traversal patterns that RNTL documents as anti-patterns. Where those internal-tree assertions previously covered implementation-only branches, this PR documents the reduced branch coverage with per-file Jest thresholds rather than reintroducing brittle tests.

Additional follow-up changes in this branch:

  • Updates SVG mocks so they do not render raw SVG/XML strings as text under the v14 renderer.
  • Refactors BottomSheet imperative ref tests to use public refs with await act.
  • Keeps coverage thresholds passing after removing RNTL anti-pattern coverage.

Related issues

See: #1182

Manual testing steps

  1. Run yarn workspace @metamask/design-system-react-native test:verbose --runInBand --coverage
  2. Run yarn lint:eslint packages/design-system-react-native packages/design-system-twrnc-preset
  3. Run yarn constraints
  4. Run yarn lint:dependencies

Screenshots/Recordings

Before

Tests used react-test-renderer, tree.root, UNSAFE_*, and direct prop traversal to inspect internal implementation details.

const tree = createRenderer(<ButtonPrimary>Press me</ButtonPrimary>);
const buttonAnimated = tree.root.findByProps({ accessibilityRole: 'button' });
const styleFn = buttonAnimated.props.style as (p: { pressed: boolean }) => unknown[];
const pressedStyles = flattenStyles(styleFn({ pressed: true }));
expectBackground(pressedStyles, 'bg-icon-default-pressed');

After

Tests use RNTL public APIs with async render, renderHook, and fireEvent calls. Coverage gaps from removed internal-tree assertions are documented in jest.config.js with per-file thresholds and references to the RNTL anti-pattern follow-up.

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs
  • I've completed the PR template to the best of my ability
  • 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.

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

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Describe block label contradicts its remaining test
    • Renamed the describe label to reflect iconClassName/textClassName being omitted, matching the remaining test.

Create PR

Or push these changes by commenting:

@cursor push 523553ffe2
Preview (523553ffe2)
diff --git a/packages/design-system-react-native/src/components/ButtonBase/ButtonBase.test.tsx b/packages/design-system-react-native/src/components/ButtonBase/ButtonBase.test.tsx
--- a/packages/design-system-react-native/src/components/ButtonBase/ButtonBase.test.tsx
+++ b/packages/design-system-react-native/src/components/ButtonBase/ButtonBase.test.tsx
@@ -299,7 +299,7 @@
     });
   });
 
-  describe('when iconClassName and textClassName are set', () => {
+  describe('when iconClassName and textClassName are omitted', () => {
     it('renders icons without extra classes when hooks are omitted', () => {
       const { getByText } = render(
         <ButtonBase startIconName={IconName.Add} endIconName={IconName.Close}>

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 2fd0146. Configure here.

Base automatically changed from codex/align-rn-expo-dependencies to main May 21, 2026 22:47
@georgewrmarshall georgewrmarshall self-assigned this May 22, 2026
@georgewrmarshall georgewrmarshall force-pushed the refactor/remove-tree-root-test-traversal branch from 2fd0146 to d80aaa8 Compare June 4, 2026 01:07
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@socket-security

socket-security Bot commented Jun 4, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedtest-renderer@​1.1.07610010093100
Updated@​testing-library/​react-native@​13.3.3 ⏵ 14.0.0-rc.39910010099100

View full report

@socket-security

socket-security Bot commented Jun 4, 2026

Copy link
Copy Markdown

Caution

MetaMask internal reviewing guidelines:

  • Do not ignore-all
  • Each alert has instructions on how to review if you don't know what it means. If lost, ask your Security Liaison or the supply-chain group
  • Copy-paste ignore lines for specific packages or a group of one kind with a note on what research you did to deem it safe.
    @SocketSecurity ignore npm/PACKAGE@VERSION
Action Severity Alert  (click "▶" to expand/collapse)
Block Low
Publisher changed: npm @jest/diff-sequences is now published by simenb instead of cpojer

New Author: simenb

Previous Author: cpojer

From: ?npm/@testing-library/react-native@14.0.0-rc.3npm/@jest/diff-sequences@30.4.0

ℹ Read more on: This package | This alert | What is new author?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support@socket.dev.

Suggestion: Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@jest/diff-sequences@30.4.0. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

📖 Storybook Preview

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.

1 participant