-
Notifications
You must be signed in to change notification settings - Fork 159
chore: restore e2e tests #6789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
chore: restore e2e tests #6789
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdated Cypress E2E test infrastructure: added/removed env keys and README entries, increased global timeouts and introduced SMALL_TIMEOUT/LARGE_TIMEOUT constants, refactored token-selection helpers to a search flow, standardized timeouts in tests, cached signer addresses, added ResizeObserver suppression, and extended provider URL fallback to include Infura. Changes
Sequence Diagram(s)(omitted — changes are test infra, helpers, and configuration without new multi-component runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ea1536b to
ee493a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.env.local.example (1)
5-15: Optional: Address static analysis hints.The static analysis tool flagged:
- Extra blank line at line 7
- Keys not in alphabetical order (CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY should precede CYPRESS_INTEGRATION_TEST_PRIVATE_KEY)
These are minor formatting preferences. While the current order is logical (private key before API key), alphabetical ordering can improve consistency if that's the project standard.
♻️ Optional fix
REACT_APP_TG_DEV_BYPASS=true - ####################################### # End To End Test ####################################### # ⚠️ Required for E2E tests to run # Cypress Integration Tests +CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY= CYPRESS_INTEGRATION_TEST_PRIVATE_KEY= -CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY=apps/cowswap-frontend-e2e/src/support/commands.ts (1)
108-116: Good refactor: search-based token selection improves robustness.The new search-and-select approach is more resilient than clicking from a static list, as it better mimics user interaction and handles dynamic token lists.
Optional considerations:
The
{ force: true }on line 113 bypasses Cypress actionability checks. While it may be necessary for flaky tests, consider whether the underlying cause (e.g., element not fully interactive) could be addressed instead.The hard-coded
20_000timeout appears multiple times. Consider extracting it to a shared constant (e.g.,LARGE_TIMEOUTfrom config) for consistency with other test files.♻️ Optional: Extract timeout constant
At the top of the file, import the timeout:
+import { LARGE_TIMEOUT } from '../config'Then replace hard-coded values:
- cy.get(buttonSelector, { timeout: 20_000 }).should('not.be.disabled') + cy.get(buttonSelector, { timeout: LARGE_TIMEOUT }).should('not.be.disabled') cy.get(buttonSelector).click()- cy.get(searchSelector, { timeout: 20_000 }).should('be.visible').clear().type(tokenAddress) - cy.get(tokenSelector, { timeout: 20_000 }).should('be.visible').click({ force: true }) + cy.get(searchSelector, { timeout: LARGE_TIMEOUT }).should('be.visible').clear().type(tokenAddress) + cy.get(tokenSelector, { timeout: LARGE_TIMEOUT }).should('be.visible').click({ force: true })
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.env.env.local.exampleapps/cowswap-frontend-e2e/cypress.config.tsapps/cowswap-frontend-e2e/src/config.tsapps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.tsapps/cowswap-frontend-e2e/src/e2e/limit-orders.test.tsapps/cowswap-frontend-e2e/src/e2e/lists.test.tsapps/cowswap-frontend-e2e/src/e2e/search.test.tsapps/cowswap-frontend-e2e/src/e2e/swap.test.tsapps/cowswap-frontend-e2e/src/e2e/swapMod.test.tsapps/cowswap-frontend-e2e/src/support/commands.tsapps/cowswap-frontend-e2e/src/support/e2e.ts
💤 Files with no reviewable changes (1)
- .env
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/cowswap-frontend-e2e/src/e2e/swapMod.test.ts
- apps/cowswap-frontend-e2e/src/e2e/lists.test.ts
- apps/cowswap-frontend-e2e/src/e2e/search.test.ts
- apps/cowswap-frontend-e2e/src/config.ts
- apps/cowswap-frontend-e2e/src/e2e/swap.test.ts
- apps/cowswap-frontend-e2e/cypress.config.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6034
File: apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts:44-47
Timestamp: 2025-07-28T16:26:08.051Z
Learning: In the cowswap codebase, using trivial placeholder tests like `it('should be true', () => { expect(true).to.be.true })` in e2e test files is an intentional pattern when disabling broken tests to keep CI green while maintaining build efficiency.
📚 Learning: 2025-09-19T11:38:59.206Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6232
File: apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx:199-200
Timestamp: 2025-09-19T11:38:59.206Z
Learning: The makeBuildClickEvent function in apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx takes five parameters: defaultChainId, contextLabel, mode, isSwapMode, and chainsCount. The chainsCount parameter is used to determine the CrossChain flag in analytics events.
Applied to files:
apps/cowswap-frontend-e2e/src/support/commands.ts
📚 Learning: 2025-08-08T13:55:17.528Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/state/tokens/allTokensAtom.ts:78-78
Timestamp: 2025-08-08T13:55:17.528Z
Learning: In libs/tokens/src/state/tokens/allTokensAtom.ts (TypeScript/Jotai), the team prefers to wait for token lists to initialize (listsStatesListAtom non-empty) before returning tokens. No fallback to favorites/user-added/native tokens should be used when listsStatesList is empty.
Applied to files:
apps/cowswap-frontend-e2e/src/support/commands.ts
📚 Learning: 2025-07-28T16:26:08.051Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6034
File: apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts:44-47
Timestamp: 2025-07-28T16:26:08.051Z
Learning: In the cowswap codebase, using trivial placeholder tests like `it('should be true', () => { expect(true).to.be.true })` in e2e test files is an intentional pattern when disabling broken tests to keep CI green while maintaining build efficiency.
Applied to files:
apps/cowswap-frontend-e2e/src/e2e/limit-orders.test.tsapps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts
📚 Learning: 2025-09-25T08:49:32.256Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:62-67
Timestamp: 2025-09-25T08:49:32.256Z
Learning: In the cowswap-frontend codebase, when testing hooks that use multiple bridge providers, both providers are always properly mocked as complete jest.Mocked<BridgeProvider<BridgeQuoteResult>> objects with all required methods stubbed, ensuring no undefined returns that could break the hook logic.
Applied to files:
apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts
📚 Learning: 2025-09-25T08:48:53.495Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:58-60
Timestamp: 2025-09-25T08:48:53.495Z
Learning: In the cowswap-frontend codebase, when writing SWR tests, the team prefers maximum test isolation by using `provider: () => new Map()` in SWRConfig wrappers, even if it recreates cache on every render, to ensure tests don't share any state.
Applied to files:
apps/cowswap-frontend-e2e/src/support/e2e.ts
🧬 Code graph analysis (1)
apps/cowswap-frontend-e2e/src/e2e/limit-orders.test.ts (1)
apps/cowswap-frontend-e2e/src/config.ts (1)
SMALL_TIMEOUT(1-1)
🪛 dotenv-linter (4.0.0)
.env.local.example
[warning] 7-7: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 15-15: [UnorderedKey] The CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY key should go before the CYPRESS_INTEGRATION_TEST_PRIVATE_KEY key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (5)
apps/cowswap-frontend-e2e/src/e2e/limit-orders.test.ts (1)
1-2: Good refactor: centralized timeout constant.Extracting the hard-coded timeout into a shared constant improves maintainability and ensures consistency across the test suite.
Also applies to: 8-8
apps/cowswap-frontend-e2e/src/support/e2e.ts (1)
58-62: Good addition: suppressing benign ResizeObserver errors.This handler prevents test flakiness by suppressing a common, harmless browser error that occurs during rapid UI updates. The implementation correctly targets only this specific error message.
apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts (2)
15-18: Excellent improvement: mocking USD price for test stability.Mocking the USD price endpoint with a fixed value ensures deterministic test behavior and prevents flakiness from external API dependencies. The explicit
cy.wait('@usdPrice')ensures the price loads before assertions run.Also applies to: 24-24
35-35: Good refactor: using.should()for better retry logic.Switching from
.then()to.should()leverages Cypress's built-in retry mechanism, which will automatically retry the assertion until it passes or times out. This improves test reliability and aligns with Cypress best practices.apps/cowswap-frontend-e2e/src/support/commands.ts (1)
99-104: Good refactor: explicit disabled state check.Computing the button selector once and checking that it's not disabled before clicking improves clarity and prevents premature clicks on disabled elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @README.md:
- Around line 119-121: Change the inconsistent environment variable name in the
E2E test docs: replace the incorrect INTEGRATION_TESTS_ALCHEMY_KEY with
CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY so all three listed vars share the
CYPRESS_ prefix (matching the documented yarn e2e usage and the other entries
CYPRESS_INTEGRATION_TEST_PRIVATE_KEY and CYPRESS_INTEGRATION_TESTS_INFURA_KEY).
🧹 Nitpick comments (1)
.env.local.example (1)
7-16: Consider addressing dotenv-linter formatting suggestions.The static analysis tool flagged a couple of minor formatting issues:
- Line 7 has an extra blank line
- Keys on lines 15-16 could be alphabetically ordered before the PRIVATE_KEY on line 14
These are purely stylistic and don't affect functionality.
📝 Proposed formatting cleanup
REACT_APP_TG_DEV_BYPASS=true - ####################################### # End To End Test ####################################### # ⚠️ Required for E2E tests to run # Cypress Integration Tests -CYPRESS_INTEGRATION_TEST_PRIVATE_KEY= CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY= CYPRESS_INTEGRATION_TESTS_INFURA_KEY= +CYPRESS_INTEGRATION_TEST_PRIVATE_KEY=
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.local.example.github/workflows/ci.ymlREADME.mdapps/cowswap-frontend-e2e/src/support/e2e.tsapps/cowswap-frontend-e2e/src/support/ethereum.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cowswap-frontend-e2e/src/support/e2e.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6034
File: apps/cowswap-frontend-e2e/src/e2e/fiat-amounts.test.ts:44-47
Timestamp: 2025-07-28T16:26:08.051Z
Learning: In the cowswap codebase, using trivial placeholder tests like `it('should be true', () => { expect(true).to.be.true })` in e2e test files is an intentional pattern when disabling broken tests to keep CI green while maintaining build efficiency.
📚 Learning: 2025-09-25T08:48:53.495Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:58-60
Timestamp: 2025-09-25T08:48:53.495Z
Learning: In the cowswap-frontend codebase, when writing SWR tests, the team prefers maximum test isolation by using `provider: () => new Map()` in SWRConfig wrappers, even if it recreates cache on every render, to ensure tests don't share any state.
Applied to files:
apps/cowswap-frontend-e2e/src/support/ethereum.ts
🪛 dotenv-linter (4.0.0)
.env.local.example
[warning] 7-7: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
[warning] 15-15: [UnorderedKey] The CYPRESS_INTEGRATION_TESTS_ALCHEMY_KEY key should go before the CYPRESS_INTEGRATION_TEST_PRIVATE_KEY key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The CYPRESS_INTEGRATION_TESTS_INFURA_KEY key should go before the CYPRESS_INTEGRATION_TEST_PRIVATE_KEY key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (3)
.github/workflows/ci.yml (1)
204-204: LGTM! Infura key properly added as fallback.The new environment variable follows the same pattern as the Alchemy key and is correctly passed from GitHub secrets to the Cypress test environment.
apps/cowswap-frontend-e2e/src/support/ethereum.ts (2)
14-25: LGTM! Infura fallback properly implemented.The provider URL resolution now correctly falls back through: NETWORK_URL → Alchemy → Infura → undefined. The logic correctly prioritizes Alchemy over Infura as documented in the README.
29-29: LGTM! Assertion message updated.The error message now includes the Infura key variable, which will help with debugging provider configuration issues.
Summary
Fixes e2e test flakiness.
Builts on top of #6751
To Test
To launch it with our development server (so you have live-reloading):
If we want to use the Cypress UI, with the production build:
Summary by CodeRabbit
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.