Skip to content

Conversation

@racitores
Copy link
Contributor

@racitores racitores commented Nov 6, 2025

Description

Introduces a Component-view testing framework with engine-only mocks, state-driven presets, and deterministic rendering helpers to speed up view-level tests without full E2E.

Adds initial tests for BridgeView and Wallet views leveraging presets and renderers.
Updates CI workflow to run component-view tests reliably across OS, and minor ESLint/test setup adjustments.

Adds local rules and documentation for Component-view tests.

Changelog

CHANGELOG entry:

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Note

Introduces a component-view testing framework with mocks/presets/renderers, adds BridgeView and Wallet tests, and updates CI and test setup.

  • Tests:
    • Framework: Add component-view testing utilities in app/util/test/component-view/ (mocks, render.tsx, state fixtures, presets, and view-specific renderers for Bridge and Wallet).
    • View Tests: New tests for BridgeView and Wallet in app/components/UI/Bridge/Views/BridgeView/BridgeView.view.test.tsx and app/components/Views/Wallet/Wallet.view.test.tsx.
    • Docs/Rules: Add local testing rules and documentation (.cursor/rules/*, INTEGRATION_TEST_RULES.md, README.md).
  • CI/Config:
    • Update GitHub Actions workflow ci.yml to run component-view tests across environments.
    • Adjust test setup in app/util/test/testSetup.js and platform helpers.
    • Minor config updates in package.json and eslintrc.js to support the new tests.

Written by Cursor Bugbot for commit 74b633a. This will update automatically on new commits. Configure here.

@racitores racitores added No QA Needed Apply this label when your PR does not need any QA effort. skip-e2e skip E2E test jobs labels Nov 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

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.

@metamaskbot metamaskbot added the team-qa QA team label Nov 6, 2025
@racitores racitores changed the title test: refactor bridgeView tests test: PoC integration test: refactor bridgeView tests Nov 11, 2025
@racitores racitores force-pushed the raci/refactor-bridgeview-test branch from 44f8f09 to 464984b Compare November 13, 2025 15:57
@racitores racitores marked this pull request as ready for review November 13, 2025 15:58
@racitores racitores requested a review from a team as a code owner November 13, 2025 15:58
@racitores racitores added the no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label Nov 13, 2025
@racitores racitores changed the title test: PoC integration test: refactor bridgeView tests test: Add integration test framework Nov 13, 2025
} catch (_e) {
// Best-effort guard; if state is not available yet, skip enforcement
}
return Reflect.apply(target, thisArg, args);
Copy link

Choose a reason for hiding this comment

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

Bug: Silent Error Suppression Breaks Runtime Guard

The runtime guard's error handling silently suppresses the intentional error thrown for forbidden mocks. The catch (_e) block catches both the expected "state not available" errors and the intentional "Forbidden jest.mock" errors, making the guard ineffective. The forbidden mock error should be re-thrown after being caught, or the try-catch should only wrap the state retrieval logic.

Fix in Cursor Fix in Web

package.json Outdated
"test": "yarn test:unit",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts --testPathIgnorePatterns='.*\\.integration(\\..*)?\\.test\\.(ts|tsx)$'",
"test:integration": "jest --runInBand --no-watchman --verbose --testPathPattern='.*\\.integration(\\..*)?\\.test\\.(ts|tsx)$'",
Copy link

Choose a reason for hiding this comment

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

Bug: Align Test Patterns with Supported File Types

The test script patterns only match .ts and .tsx files but the ESLint rules, documentation, and Cursor rules all specify support for .js and .jsx integration tests as well. The testPathIgnorePatterns and testPathPattern regexes should include (ts|tsx|js|jsx) instead of just (ts|tsx) to match the documented behavior and ESLint configuration.

Fix in Cursor Fix in Web

@racitores racitores force-pushed the raci/refactor-bridgeview-test branch from 7009fb7 to 94c3a78 Compare November 17, 2025 10:24
return Reflect.apply(target, thisArg, args);
},
});
})();
Copy link

Choose a reason for hiding this comment

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

Bug: Silent Error Catch Undermines Mock Security

The runtime guard wraps jest.mock in a Proxy but catches and silently ignores all errors in the try-catch block at line 154. This means if global.expect?.getState?.() throws an error (not just returns undefined), the guard fails silently and allows unauthorized mocks through. The catch should only suppress errors when state is unavailable, not all errors including the intentional throw new Error at line 145 that enforces the whitelist.

Fix in Cursor Fix in Web

return Reflect.apply(target, thisArg, args);
},
});
})();
Copy link

Choose a reason for hiding this comment

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

Bug: Guard's Silent Catch Undermines Enforcement

The runtime guard's catch block silently swallows all errors, including the enforcement error itself. When jest.mock is called with a forbidden module in an integration test, the thrown error gets caught and ignored, defeating the guard's purpose. The catch should only suppress errors from accessing expect.getState(), not from the enforcement logic.

Fix in Cursor Fix in Web

package.json Outdated
"release:android:qa": "./scripts/build.sh android QA && open android/app/build/outputs/apk/release/",
"test": "yarn test:unit",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts --testPathIgnorePatterns='.*\\.integration(\\..*)?\\.test\\.(ts|tsx)$'",
Copy link

Choose a reason for hiding this comment

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

Bug: CLI flags break Jest test path exclusion.

The --testPathIgnorePatterns CLI flag in test:unit overrides the testPathIgnorePatterns array in jest.config.js, causing unit tests to no longer ignore e2e specs, pages, and selectors. Jest CLI arguments replace rather than merge with config file settings, so the patterns for e2e files ('.*/e2e/specs/.*\\.spec\\.(ts|js)$', '.*/e2e/pages/', '.*/e2e/selectors/') are lost.

Fix in Cursor Fix in Web

@racitores racitores force-pushed the raci/refactor-bridgeview-test branch from 7c6dd34 to 2b4b451 Compare November 17, 2025 12:00
const testPath = state?.testPath ?? '';
const isComponentViewTest = /\.[iI]view(\..*)?\.test\.(t|j)sx?$/.test(
testPath,
);
Copy link

Choose a reason for hiding this comment

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

Bug: Case-Insensitive Regex Fails View Enforcement

The regex pattern /\.[iI]view(\..*)?\.test\.(t|j)sx?$/ uses [iI] which matches both lowercase i and uppercase I, but the intended pattern is .view (with lowercase v). This causes the guard to match files like .iview.test.ts or .Iview.test.ts instead of the documented .view.test.ts pattern, failing to enforce mocking restrictions on actual component-view test files.

Fix in Cursor Fix in Web

"test": "yarn test:unit",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts",
"test:unit": "jest ./app/ ./locales/ ./e2e/**/*.test.ts .github/**/*.test.ts --testPathIgnorePatterns='.*/e2e/specs/.*\\.spec\\.(ts|tsx|js)$|.*\\.view(\\..*)?\\.test\\.(ts|tsx)$'",
"test:view": "jest --runInBand --no-watchman --verbose --testPathPattern='.*\\.view(\\..*)?\\.test\\.(ts|tsx)$'",
Copy link

Choose a reason for hiding this comment

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

Bug: JS/JSX test files lack proper isolation.

The test patterns in test:unit and test:view only match .ts and .tsx files via (ts|tsx), but documentation and ESLint rules specify support for .js and .jsx files as well (*.view.test.{ts,tsx,js,jsx}). This causes .view.test.js and .view.test.jsx files to run in unit tests instead of being properly isolated.

Fix in Cursor Fix in Web

@racitores racitores changed the title test: Add integration test framework test: Add component view test framework Nov 17, 2025
// Expect state is available when evaluating test files
const state = global.expect?.getState?.();
const testPath = state?.testPath ?? '';
const isComponentViewTest = /\.[iI]view(\..*)?\.test\.(t|j)sx?$/.test(
Copy link

Choose a reason for hiding this comment

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

Bug: Unauthorized Mocks Infiltrate View Tests

The regex pattern /\.[iI]view(\..*)?\.test\.(t|j)sx?$/ matches files with .iview or .Iview but not .view (lowercase 'v'). The actual test files use .view.test.tsx naming. This prevents the runtime guard from enforcing mock restrictions in component-view tests, allowing unauthorized mocks that should be blocked.

Fix in Cursor Fix in Web

@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

After thorough analysis of all 20 changed files, I conclude that NO E2E test tags need to be run:

Analysis Summary:

  1. CRITICAL FILE - package.json: Could not get git diff, but based on the file content review, this appears to be version 7.60.0 with standard dependencies. Without a diff, I cannot confirm changes, but the file content looks stable with typical MetaMask Mobile dependencies.

  2. Test Infrastructure Changes (Primary Focus): The bulk of changes are concentrated in a NEW component-view testing framework:

    • app/util/test/component-view/* - New test framework directory with:
      • README.md, INTEGRATION_TEST_RULES.md - Documentation
      • mocks.ts, render.tsx, stateFixture.ts - Test utilities
      • presets/bridge.ts, presets/wallet.ts - Test presets
      • renderers/bridge.ts, renderers/wallet.ts - Render helpers
    • BridgeView.view.test.tsx, Wallet.view.test.tsx - New UNIT tests using the framework
    • These are unit/integration tests for React components, NOT E2E tests
  3. Configuration Updates:

    • .eslintrc.js - ESLint rules to enforce the new component-view test patterns
    • app/util/test/testSetup.js - Jest setup with runtime guards for test purity
    • app/util/test/platform.ts - Platform testing helpers
  4. Cursor/Documentation Changes:

    • .cursor/rules/component-view-testing.mdc - Cursor AI context rules
    • .cursor/rules/pr-creation-guidelines.mdc - PR guidelines
    • These are developer experience improvements
  5. CI Workflow (.github/workflows/ci.yml): Could not get diff, but the file shows it already has component-view test job (component-view-test) that runs yarn test:view. This will validate the new framework.

  6. State logs (state-logs-v7.59.0-(2968).json): This appears to be debug/telemetry data, not production code

Key Findings:

  • These changes introduce a NEW unit testing framework for React component views
  • The framework focuses on testing UI components with mocked Engine/Redux state
  • No production code changes detected
  • No changes to E2E test specs, pages, selectors, or infrastructure
  • The CI workflow already includes validation for component-view tests
  • This is purely a testing infrastructure enhancement

Why No E2E Tags:

  1. Changes are test infrastructure only (unit/integration tests, not E2E)
  2. No production application code modified
  3. No E2E test framework changes (e2e/framework, e2e/specs, etc.)
  4. Component-view tests run separately via yarn test:view in CI
  5. These tests validate React components in isolation, not end-to-end user flows

Risk Assessment: LOW

  • No user-facing functionality changes
  • Test infrastructure is self-contained and validated by CI
  • The framework is additive (new capability) without removing existing tests
  • ESLint and runtime guards ensure proper usage

View GitHub Actions results

// Expect state is available when evaluating test files
const state = global.expect?.getState?.();
const testPath = state?.testPath ?? '';
const isComponentViewTest = /\.[iI]view(\..*)?\.test\.(t|j)sx?$/.test(
Copy link

Choose a reason for hiding this comment

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

Bug: Security Guard's Blind Spot

The runtime guard regex /\.[iI]view(\..*)?\.test\.(t|j)sx?$/ expects files matching .iview.test.* or .Iview.test.*, but the actual component-view test files follow the pattern .view.test.* (lowercase 'v' without 'i' prefix). The regex [iI]view matches only iview or Iview, not view. This causes the runtime enforcement to never trigger for component-view tests, allowing unauthorized mocks that should be blocked.

Fix in Cursor Fix in Web

Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

left a few comments related to using TestIDs. otherwise it looks good @racitores

});

// Type 9.5 using keypad buttons
const scroll = getByTestId('bridge-view-scroll');
Copy link
Member

Choose a reason for hiding this comment

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

Can we, similar to the e2e tests use the testIDs defined in the testID object? this way we have a consistent way of storing and using testIDs within e2e and integration tests? In this case, we can import selector string here and use it


// Ensure keypad is visible
await waitFor(() => {
expect(queryByTestId('keypad-delete-button')).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

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

});

// Close possible banner to reveal keypad
const closeBanner = queryByTestId('banner-close-button-icon');
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No QA Needed Apply this label when your PR does not need any QA effort. no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed size-XL skip-e2e skip E2E test jobs team-qa QA team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants