Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
fionnachan
added
bug
There was a problem hiding this comment.
Pull request overview
Migrates the arb-token-bridge-ui E2E test harness from Synpress v3 to Synpress v4, updating Cypress/Synpress integration, MetaMask command APIs, and CI execution to support the new wallet cache architecture.
Changes:
- Bump
@synthetixio/synpressto v4 and update Cypress plugin/support initialization (configureSynpressForMetaMask,synpressCommandsForMetaMask). - Replace v3 MetaMask Cypress commands with v4 equivalents across support utilities and E2E specs.
- Add Synpress wallet-cache setup (wallet-setup files + CI step) and adjust E2E runner scripts/config (incl.
testIsolation: false).
Reviewed changes
Copilot reviewed 20 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/arb-token-bridge-ui/tests/tsconfig.json | Removes v3 Synpress support types from test TS config. |
| packages/arb-token-bridge-ui/tests/support/index.ts | Switches to v4 Synpress Cypress support + rewrites MetaMask bootstrap flow. |
| packages/arb-token-bridge-ui/tests/support/common.ts | Replaces v3 “accept access” helper with v4 connectToDapp. |
| packages/arb-token-bridge-ui/tests/support/commands.ts | Updates login/network switching and tx approval/rejection commands for v4. |
| packages/arb-token-bridge-ui/tests/e2e/specs/withdrawNativeToken.cy.ts | Updates tx confirmation calls to v4 APIs. |
| packages/arb-token-bridge-ui/tests/e2e/specs/withdrawERC20.cy.ts | Updates tx confirmation calls to v4 APIs. |
| packages/arb-token-bridge-ui/tests/e2e/specs/withdrawCctp.cy.ts | Updates tx/network switching calls to v4 APIs. |
| packages/arb-token-bridge-ui/tests/e2e/specs/redeemRetryable.cy.ts | Updates tx confirmation calls to v4 APIs. |
| packages/arb-token-bridge-ui/tests/e2e/specs/depositCctp.cy.ts | Updates tx confirm/reject calls and comments for v4 APIs. |
| packages/arb-token-bridge-ui/tests/e2e/specs/approveToken.cy.ts | Updates tx rejection call to v4 API. |
| packages/arb-token-bridge-ui/tests/e2e/getCommonSynpressConfig.ts | Sets testIsolation: false per Synpress v4 requirements. |
| packages/arb-token-bridge-ui/tests/e2e/cypress.d.ts | Removes v3 Synpress type reference. |
| packages/arb-token-bridge-ui/test/wallet-setup/tsconfig.json | Adds TS config for wallet cache setup scripts. |
| packages/arb-token-bridge-ui/test/wallet-setup/basic.setup.ts | Adds Synpress wallet cache setup using Playwright MetaMask helper. |
| packages/arb-token-bridge-ui/synpress.config.ts | Replaces v3 plugin with configureSynpressForMetaMask. |
| packages/arb-token-bridge-ui/synpress.cctp.config.ts | Replaces v3 plugin with configureSynpressForMetaMask. |
| packages/arb-token-bridge-ui/synpress-plugins.d.ts | Removes v3 plugin module declaration. |
| packages/arb-token-bridge-ui/package.json | Bumps @synthetixio/synpress to 4.0.5. |
| package.json | Adds wallet-cache script and switches E2E execution to cypress run flow. |
| SYNPRESS_V4_MIGRATION.md | Documents v3→v4 migration steps and command mapping. |
| CLAUDE.md | Adds guidance about not reading .env files. |
| .gitignore | Ignores Synpress cache directory. |
| .github/workflows/e2e-tests.yml | Adds CI step to build Synpress wallet cache before running E2E tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| cy.switchNetwork({ networkName: networkNameWithDefault, isTestnet: true }).then(() => { | ||
| _startWebApp(); | ||
| }); |
There was a problem hiding this comment.
login() now always calls cy.switchNetwork({ ..., isTestnet: true }). Several specs call cy.login({ networkName: 'Ethereum' }) to run against mainnet; forcing isTestnet: true will likely prevent switching to Ethereum mainnet. Consider deriving isTestnet from networkNameWithDefault (or accepting it as a parameter) so mainnet and testnet flows both work.
| Cypress.on('uncaught:exception', () => false); | ||
|
|
There was a problem hiding this comment.
Cypress.on('uncaught:exception', () => false) suppresses all uncaught exceptions, which can mask real application/test failures and make debugging harder. Prefer filtering to known/expected MetaMask/Synpress errors (e.g., by inspecting the error message) or removing this handler and fixing the underlying exception source.
| - New file: `packages/arb-token-bridge-ui/test/wallet-setup/basic.setup.ts` | ||
| - Uses `defineWalletSetup()` and `MetaMask` class to import wallet from seed phrase | ||
|
|
There was a problem hiding this comment.
This step says the wallet is imported from a seed phrase, but basic.setup.ts uses importWalletFromPrivateKey(). Update the migration doc to reflect the actual implementation (private key) to avoid confusion for future maintenance.
| - name: Build Synpress wallet cache | ||
| run: xvfb-run npx synpress ./packages/arb-token-bridge-ui/test/wallet-setup | ||
| env: | ||
| DISPLAY: :0.0 | ||
| PRIVATE_KEY_USER: ${{ secrets.E2E_PRIVATE_KEY_USER }} | ||
|
|
There was a problem hiding this comment.
The workflow builds the Synpress wallet cache here, but the test:e2e / test:e2e:cctp scripts also run yarn synpress:cache first. This duplication adds runtime and another potential failure point; consider removing this workflow step or gating the script cache build behind an env flag when running in CI.
| // The wallet cache only bootstraps MetaMask with an initialized state. | ||
| // The actual test wallet (PRIVATE_KEY_USER) is imported at runtime in support/index.ts | ||
| // via cy.importWalletFromPrivateKey(). | ||
| const PRIVATE_KEY = process.env.PRIVATE_KEY_USER; |
There was a problem hiding this comment.
The comment says the wallet cache only initializes MetaMask and that the actual test wallet is imported at runtime via cy.importWalletFromPrivateKey(), but this setup file already calls metamask.importWalletFromPrivateKey(PRIVATE_KEY_USER). Either update the comment (and migration doc) or adjust the setup so the cache build doesn’t import the same wallet that tests later import again.
No description provided.