test(e2e): market-orders specs - #7975
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR expands Playwright E2E coverage with shared fixtures, page objects, transaction and API mocks, lifecycle controls, and market and limit order scenarios. It also adds E2E polling controls and frontend selectors. ChangesPlaywright E2E foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant SwapPage
participant MockRPC
participant CowAPI
Test->>SwapPage: configure and submit trade
SwapPage->>MockRPC: send approval or transaction
MockRPC-->>Test: return receipt and state handle
SwapPage->>CowAPI: post order
CowAPI-->>Test: return open or executing order
Test->>CowAPI: fulfill or cancel order
CowAPI-->>Test: return updated order status
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (7)
apps/cowswap-e2e-tests/src/support/mockCancellableOrder.ts (1)
53-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
validToout ofbuildOrderso the seeded order stays byte-stable across polls.
buildOrderruns on everyaccountOrdersandorderrequest. Line 75 recomputesMath.floor(Date.now() / 1000) + 3600each time, so the same order reports a differentvalidToon each poll.creationDateis already hoisted at Line 48 for exactly this reason.The order identity is the uid, so this is unlikely to break the current assertions. A fixed
validTostill removes one source of nondeterminism and matches thecreationDatetreatment.♻️ Proposed change
const creationDate = new Date(Date.now() - createdSecondsAgo * 1000).toISOString() + const validTo = Math.floor(Date.now() / 1000) + 3600 @@ - validTo: Math.floor(Date.now() / 1000) + 3600, + validTo,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/support/mockCancellableOrder.ts` around lines 53 - 85, Hoist the computed validTo timestamp alongside the existing creationDate constant, then have buildOrder reuse that stable value instead of recalculating Date.now() on each invocation. Keep the order’s uid and all other seeded fields unchanged.apps/cowswap-e2e-tests/src/support/mockApproveTransaction.ts (1)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the decoded calldata before you treat it as
approve.
wallet.stubRpc('eth_sendTransaction', stub)intercepts every transaction the test sends, not only the approval.decodeFunctionDataat Line 58 then throws an opaque viem error, or assignsargs[0]/args[1]from a differenterc20Abifunction, if the app sends anything else.
mockUnwrapTransactionalready checks the selector first and throws a named error. Apply the same guard for a clearer failure.♻️ Proposed guard
+const APPROVE_SELECTOR = '0x095ea7b3' + const stub: RpcStub = ({ params }) => { const tx = params[0] as { data?: Hex } + const data = tx.data ?? '0x' + if (!data.toLowerCase().startsWith(APPROVE_SELECTOR)) { + throw new Error(`mockApproveTransaction: expected an approve() call, got calldata ${data}`) + } // Ground truth: decode the actual approve(spender, amount) calldata rather than trusting the // UI's rendered figure. - const { args } = decodeFunctionData({ abi: erc20Abi, data: tx.data as Hex }) + const { args } = decodeFunctionData({ abi: erc20Abi, data }) spender = args[0] as Hex approvedAmount = args[1] as bigint🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/support/mockApproveTransaction.ts` around lines 54 - 63, Update the eth_sendTransaction stub around decodeFunctionData to verify the transaction calldata selector identifies the ERC-20 approve function before decoding and assigning spender and approvedAmount. If it is not an approve call, throw a clear named error consistent with mockUnwrapTransaction, while preserving the existing allowance update for valid approvals.apps/cowswap-e2e-tests/src/tests/market-orders.spec.ts (2)
1118-1127: 🩺 Stability & Availability | 🔵 Trivial[CS-111] can spend up to 105 seconds on two real-time waits.
Lines 1122 and 1127 wait 45 s and 60 s for
PENDING_ORDERS_BUFFERto elapse in wall-clock time. The comment correctly identifies this as genuine time dependence rather than a mock defect. The test is also tagged@smoke, so it runs in the fast lane.Two options reduce the cost without weakening the assertion:
- Backdate
creationDatefurther throughmockCancellableOrder'screatedSecondsAgosoPENDING_ORDERS_BUFFERis nearly elapsed whenmarkCancelled()runs, leaving only the transient window to observe.- Drive the buffer through the same E2E polling-acceleration mechanism this PR adds via
getUpdaterInterval, so the constant is configurable under test.Removing the
@smoketag is a third option if the runtime must stay as is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/tests/market-orders.spec.ts` around lines 1118 - 1127, The cancellable-order smoke test can spend up to 105 seconds waiting for real-time cancellation transitions. Reduce this runtime without weakening both “Cancelling...” and final “Cancelled” assertions by either increasing `createdSecondsAgo` when configuring `mockCancellableOrder` so the buffer is nearly elapsed, or using the test polling-acceleration mechanism via `getUpdaterInterval`; otherwise remove the `@smoke` tag if the waits remain unchanged.
909-919: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the SDK order types instead of inline body shapes.
Lines 910 and 917 cast
req.bodyto hand-rolled{ fullAppData: string }and{ appDataHash?: string }.apps/cowswap-e2e-tests/src/support/mockOrderPosting.tsalready casts the samepostOrderbody toOrderCreationfrom the SDK. Reuse that type here so the two mocks agree on the payload contract.As per path instructions: "Prefer real CoW Protocol SDK types over hand-rolled interfaces when shaping a mock's request/response body."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/tests/market-orders.spec.ts` around lines 909 - 919, Update the request-body casts in the putAppData and postOrder mock handlers to use the appropriate CoW Protocol SDK order types, matching the OrderCreation usage in mockOrderPosting.ts. Remove the inline { fullAppData: string } and { appDataHash?: string } shapes while preserving the existing uploadedAppData, uploadedAppDataHash, and postedOrderAppDataHash assignments.Source: Path instructions
apps/cowswap-frontend/src/utils/orderUtils/getTokenFromMapping.ts (1)
3-12: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unnecessary EVM-shaped cast.
TokensByAddressusesgetAddressKeykeys, so the first lookup is correct.getAddressKeyacceptsstringat existing call sites; usegetAddressKey(address)directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/utils/orderUtils/getTokenFromMapping.ts` around lines 3 - 12, Update getTokenFromMapping to call getAddressKey(address) directly, removing the unnecessary `0x${string}` cast while preserving the existing token lookup fallback behavior.Source: Learnings
apps/cowswap-e2e-tests/src/support/mockEthFlowTransaction.ts (1)
230-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating the calldata selector before capturing the transaction.
The stub accepts any
eth_sendTransactionand returnsFAKE_ETH_FLOW_TX_HASH. If a test sends a different transaction on the same wallet, the mock records its value as the ETH-flow sell amount and the balance assertion fails with a confusing message.mockUnwrapTransactionguards this case by throwing on an unexpected selector (seeapps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.tslines 75-77). Apply the same guard here forcreateOrder.Note that
decodeEthFlowOrderParamsalready returnsundefinedon a decode failure, so a wrong transaction currently produces a silentundefinedrather than an error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/support/mockEthFlowTransaction.ts` around lines 230 - 236, Update the RPC stub in the createOrder mock around decodeEthFlowOrderParams to validate the transaction calldata selector before recording sentValue or orderParams. Reject unexpected selectors by throwing the same kind of error used by mockUnwrapTransaction, while preserving normal handling for valid createOrder calldata.apps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.ts (1)
1-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the shared native-balance plumbing into its own module.
This file imports
classifyEthCall,isFullyMocked, andresolveEthBalanceBatchfrommockEthFlowTransaction, and it redeclaresUINT256,ClassifiedEntry, andJsonRpcEntry. The unwrap mock therefore depends on the ETH-flow mock for generic Multicall3 decoding. Move the shared codec, the shared types, and the route-merge logic into a dedicated support module (for examplenativeBalanceRoute.ts). Both mocks then depend on that module instead of on each other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.ts` around lines 1 - 48, Extract the shared Multicall3/native-balance codec, route-merge logic, and types (including UINT256, ClassifiedEntry, and JsonRpcEntry) from mockEthFlowTransaction and mockUnwrapTransaction into a dedicated support module such as nativeBalanceRoute.ts. Update both mocks to import these shared symbols from the new module, removing their direct dependency on each other while preserving existing decoding and route behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cowswap-e2e-tests/AGENTS.md`:
- Around line 48-54: Update the mock helper reference in the scenario guidance
from SwapPage.mockOrderPosting to tradePage.mockOrderPosting, preserving the
existing description of its arguments, returned handle, and separate posting and
fulfillment calls.
In `@apps/cowswap-e2e-tests/src/pages/TokenSelector.ts`:
- Around line 32-34: Update the address branch in the TokenSelector locator to
derive the data-address value with getAddressKey, matching TokenListItem’s
attribute generation; keep the symbol selector behavior unchanged.
In `@apps/cowswap-e2e-tests/src/support/mockApproveTransaction.ts`:
- Around line 66-77: In
apps/cowswap-e2e-tests/src/support/mockApproveTransaction.ts#L66-L77, replace
the receipt-only fulfillment logic with per-entry classification and upstream
fetching so mixed batches preserve non-mocked results. In
apps/cowswap-e2e-tests/src/support/mockWrapTransaction.ts#L74-L135, extract
classify, isEntryFullyMocked, buildResult, and the route handler into the shared
module alongside classifyEthCall, isFullyMocked, and resolveEthBalanceBatch,
parameterized by fake transaction hash and native-balance callback, then consume
that shared helper here.
In `@apps/cowswap-e2e-tests/src/support/mockEthFlowTransaction.ts`:
- Around line 70-77: Replace the manual lowercased address comparison in the
GET_ETH_BALANCE_SELECTOR branch with the repository’s areAddressesEqual helper
from `@cowprotocol/cow-sdk`, preserving the existing ownBalance result for
matching addresses and OPAQUE fallback behavior.
In `@apps/cowswap-e2e-tests/src/support/mockOrderPosting.ts`:
- Around line 71-79: The fulfill method currently overwrites the trader’s
existing buy-token balance with the executed amount. Update fulfill and its
callers to obtain the pre-trade buy-token balance, then store that balance plus
BigInt(postedBody.buyAmount) while preserving the existing sell-token deduction.
In `@apps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.ts`:
- Around line 72-83: Enforce the single-unwrap assumption in the `stub` handler
by rejecting any subsequent `eth_sendTransaction` call after `sentValue` has
already been set, before overwriting it or updating balances. Preserve the
existing first-call debit and error behavior for non-withdraw calldata.
In `@apps/cowswap-e2e-tests/src/support/mockWrapTransaction.ts`:
- Around line 66-72: Update MockWrapTransactionOpts and mockWrapTransaction to
accept initialWethBalance, then set the WETH balance to initialWethBalance plus
sentValue instead of replacing it. Update the CS-103 call site in
market-orders.spec.ts to pass initialWethBalance: 0n and seed that same value
through seedTrader, keeping the mock symmetric with mockUnwrapTransaction.
- Around line 74-135: Extract the duplicated classify-and-patch route logic from
the local route handler into a shared helper in mockEthFlowTransaction,
parameterized by the fake transaction hash and a getEthBalance() callback. Reuse
this helper from mockWrapTransaction, mockUnwrapTransaction, and
mockApproveTransaction while preserving each file’s balance adjustment and
eth_sendTransaction behavior; keep classifyEthCall, isFullyMocked, and
resolveEthBalanceBatch in the shared implementation.
In `@apps/cowswap-e2e-tests/src/tests/limit-orders.spec.ts`:
- Around line 67-77: Move the post-confirmation flow into LimitPage by replacing
the direct placeOrderButton click with limitPage.placeOrder(), preserving its
enabled-state wait. Add LimitPage locators/actions for the “Order Submitted”
heading, Continue button, Open orders tab, and orders table, then use those
page-object APIs in the spec instead of direct page selectors.
In `@apps/cowswap-e2e-tests/src/tests/market-orders.spec.ts`:
- Around line 26-28: Add a test.beforeEach within the “Connected EOA wallet”
describe that seeds a sufficient default wallet balance for every test, using
the existing balance-seeding helper. Keep per-test seedTrader or
setupTestConditions calls able to override this default, and apply the same
default-balance setup to each other describe in market-orders.spec.ts so no test
falls back to a real balance fetch.
- Around line 1287-1299: Update mockCorrelatedQuote to handle decimal scaling in
both directions: when sellDecimals exceeds buyDecimals, divide sellAmount by the
corresponding power of ten; when buyDecimals exceeds sellDecimals, multiply it
instead. Preserve the unchanged amount when the decimals match and use a
non-negative exponent in every BigInt power operation.
- Around line 226-228: Move the listed raw selectors and related interactions
from the spec into readonly Locator properties on SwapPage, ConfirmModal, and
AccountModal, initialized in their constructors: approval mode,
settings/slippage controls, confirmation amount, Wrap, Unwrap, Request
cancellation, and Connect Wallet. Add SwapPage.setSlippage(percent) and update
the spec to use these page-object locators and action instead of inline
selectors or helper logic.
- Line 937: Replace the lowercase string comparisons for
permitSignRequest.domain.verifyingContract and hook.target with
areAddressesEqual from `@cowprotocol/cow-sdk`, preserving the existing assertions.
Leave the lowercase PermitInfo.json fixture-key construction unchanged because
it represents the external wire format.
- Around line 487-509: Extract the duplicated ETH-flow order mock into a shared
src/support helper named mockEthFlowOrderIndexing, accepting mocks.cowApi and
ethFlow and returning markIndexed() to control order availability; replace both
inline mocks.cowApi.set('order', ...) blocks with this setup and invoke
markIndexed() in each test. Also consolidate the duplicated rate, seeding, and
four-step progress arrangement/assertions shared by CS-59 and CS-118, removing
the redundant scenario or reusing a shared setup.
In `@apps/cowswap-frontend/src/legacy/state/orders/consts.ts`:
- Around line 12-16: Replace the E2E polling interval selection around the order
interval constants with cap semantics so E2E mode uses 800ms for market-order,
limit-order, pending-order, expired-order, allowance, and priority-token
polling; use Math.min or an equivalent approach rather than Math.max. Apply the
corresponding changes in apps/cowswap-frontend/src/legacy/state/orders/consts.ts
(lines 12-16), libs/balances-and-allowances/src/state/allowancesAtom.ts (lines
13 and 96), and
libs/balances-and-allowances/src/updaters/PriorityTokensUpdater.tsx (line 9),
preserving normal-mode intervals.
In `@libs/common-const/src/common.ts`:
- Around line 101-105: Update getUpdaterInterval to use Math.min instead of
Math.max when IS_E2E_FAST_POLLING is enabled, so E2E polling is capped at
E2E_FAST_POLL_INTERVAL while normal polling intervals remain unchanged.
---
Nitpick comments:
In `@apps/cowswap-e2e-tests/src/support/mockApproveTransaction.ts`:
- Around line 54-63: Update the eth_sendTransaction stub around
decodeFunctionData to verify the transaction calldata selector identifies the
ERC-20 approve function before decoding and assigning spender and
approvedAmount. If it is not an approve call, throw a clear named error
consistent with mockUnwrapTransaction, while preserving the existing allowance
update for valid approvals.
In `@apps/cowswap-e2e-tests/src/support/mockCancellableOrder.ts`:
- Around line 53-85: Hoist the computed validTo timestamp alongside the existing
creationDate constant, then have buildOrder reuse that stable value instead of
recalculating Date.now() on each invocation. Keep the order’s uid and all other
seeded fields unchanged.
In `@apps/cowswap-e2e-tests/src/support/mockEthFlowTransaction.ts`:
- Around line 230-236: Update the RPC stub in the createOrder mock around
decodeEthFlowOrderParams to validate the transaction calldata selector before
recording sentValue or orderParams. Reject unexpected selectors by throwing the
same kind of error used by mockUnwrapTransaction, while preserving normal
handling for valid createOrder calldata.
In `@apps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.ts`:
- Around line 1-48: Extract the shared Multicall3/native-balance codec,
route-merge logic, and types (including UINT256, ClassifiedEntry, and
JsonRpcEntry) from mockEthFlowTransaction and mockUnwrapTransaction into a
dedicated support module such as nativeBalanceRoute.ts. Update both mocks to
import these shared symbols from the new module, removing their direct
dependency on each other while preserving existing decoding and route behavior.
In `@apps/cowswap-e2e-tests/src/tests/market-orders.spec.ts`:
- Around line 1118-1127: The cancellable-order smoke test can spend up to 105
seconds waiting for real-time cancellation transitions. Reduce this runtime
without weakening both “Cancelling...” and final “Cancelled” assertions by
either increasing `createdSecondsAgo` when configuring `mockCancellableOrder` so
the buffer is nearly elapsed, or using the test polling-acceleration mechanism
via `getUpdaterInterval`; otherwise remove the `@smoke` tag if the waits remain
unchanged.
- Around line 909-919: Update the request-body casts in the putAppData and
postOrder mock handlers to use the appropriate CoW Protocol SDK order types,
matching the OrderCreation usage in mockOrderPosting.ts. Remove the inline {
fullAppData: string } and { appDataHash?: string } shapes while preserving the
existing uploadedAppData, uploadedAppDataHash, and postedOrderAppDataHash
assignments.
In `@apps/cowswap-frontend/src/utils/orderUtils/getTokenFromMapping.ts`:
- Around line 3-12: Update getTokenFromMapping to call getAddressKey(address)
directly, removing the unnecessary `0x${string}` cast while preserving the
existing token lookup fallback behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1073a2d0-2e85-4311-b6e6-fccc3b72d4c1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
apps/cowswap-e2e-tests/AGENTS.mdapps/cowswap-e2e-tests/package.jsonapps/cowswap-e2e-tests/scripts/run-test.shapps/cowswap-e2e-tests/src/fixtures/shared.tsapps/cowswap-e2e-tests/src/mocks/allowances/index.tsapps/cowswap-e2e-tests/src/pages/AccountModal.tsapps/cowswap-e2e-tests/src/pages/ConfirmModal.tsapps/cowswap-e2e-tests/src/pages/HeaderPage.tsapps/cowswap-e2e-tests/src/pages/LimitPage.tsapps/cowswap-e2e-tests/src/pages/SwapPage.tsapps/cowswap-e2e-tests/src/pages/TokenSelector.tsapps/cowswap-e2e-tests/src/pages/TwapPage.tsapps/cowswap-e2e-tests/src/support/expectActivityStatus.tsapps/cowswap-e2e-tests/src/support/mockApproveTransaction.tsapps/cowswap-e2e-tests/src/support/mockCancellableOrder.tsapps/cowswap-e2e-tests/src/support/mockEthFlowTransaction.tsapps/cowswap-e2e-tests/src/support/mockFixedRateQuote.tsapps/cowswap-e2e-tests/src/support/mockOrderPosting.tsapps/cowswap-e2e-tests/src/support/mockUnwrapTransaction.tsapps/cowswap-e2e-tests/src/support/mockWrapTransaction.tsapps/cowswap-e2e-tests/src/support/readTitledAmount.tsapps/cowswap-e2e-tests/src/support/seedTrader.tsapps/cowswap-e2e-tests/src/support/selectTokens.tsapps/cowswap-e2e-tests/src/support/tokens.test.tsapps/cowswap-e2e-tests/src/support/tokens.tsapps/cowswap-e2e-tests/src/tests/limit-orders.spec.tsapps/cowswap-e2e-tests/src/tests/market-orders.spec.tsapps/cowswap-frontend/src/common/hooks/useTokenAllowance.tsapps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsxapps/cowswap-frontend/src/common/pure/PriceImpactIndicator/index.tsxapps/cowswap-frontend/src/legacy/state/orders/consts.tsapps/cowswap-frontend/src/modules/account/containers/AccountDetails/index.tsxapps/cowswap-frontend/src/modules/erc20Approve/pure/Toggle/Toggle.tsxapps/cowswap-frontend/src/modules/ethFlow/pure/EthFlowStepper/Step.tsxapps/cowswap-frontend/src/modules/ethFlow/pure/EthFlowStepper/index.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/OrdersTable.pure.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTable/OrdersTable.styled.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTabs/OrdersTabs.pure.tsxapps/cowswap-frontend/src/modules/trade/pure/ConfirmDetailsItem/index.tsxapps/cowswap-frontend/src/modules/trade/pure/ReviewOrderModalAmountRow/index.tsxapps/cowswap-frontend/src/utils/orderUtils/getTokenFromMapping.tslibs/balances-and-allowances/src/state/allowancesAtom.tslibs/balances-and-allowances/src/updaters/PriorityTokensUpdater.tsxlibs/common-const/src/common.tslibs/snackbars/src/pure/SnackbarPopup/index.tsx
💤 Files with no reviewable changes (1)
- apps/cowswap-e2e-tests/src/support/tokens.test.ts
There was a problem hiding this comment.
✅ AI Review (Claude Sonnet 5, worked ~15m): follow-up addressed
Rechecked
- Finding 1 —
Toggle'sid="approve-mode-selector"collision: Fixed.Toggle.tsx:31now usesclassName="approve-mode-selector"instead ofid, andSwapPage.approveModeSelector(apps/cowswap-e2e-tests/src/pages/SwapPage.ts:107) selects by that class. Confirmed no other reference to the oldidremains in the codebase. - Finding 2 —
E2E_FAST_POLL_INTERVAL=2smakingMath.mina no-op for the market-order poll interval: confirmed intentional by the PR author. No change needed.
Result: Both items resolved — the id-collision fix landed, and the polling-interval question was answered as intentional, not a bug.
🤖 Verification notes for AI agents
Verify the prior findings against current code only.
1. apps/cowswap-frontend/src/modules/erc20Approve/pure/Toggle/Toggle.tsx:31 should use
className="approve-mode-selector", not id, since Toggle can render more than once
simultaneously (per-order in the account activity list, plus the swap form). Confirm
apps/cowswap-e2e-tests/src/pages/SwapPage.ts's approveModeSelector locator selects
`.approve-mode-selector`.
2. libs/common-const/src/common.ts's E2E_FAST_POLL_INTERVAL = ms`2s` is intentional per author
confirmation — do not re-flag it.
Generated using the pr-review skill from the CoW Protocol skills repo.
Covers toggling Hooks on in swap settings, the nav entry appearing behind the "Trading mode" dropdown, persistence across a reload, and navigating into the Hooks-enabled swap widget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
||
| await expect(swapPage.sellBalance).toHaveAttribute('title', '1500 USDC') | ||
| await expect(swapPage.buyBalance).toHaveAttribute('title', '0 WETH') | ||
| await expect(swapPage.inputAmount).toHaveValue('1000') |
| overrides: Map<string, string>, | ||
| ): Promise<void> { | ||
| try { | ||
| const upstream = await route.fetch() |
There was a problem hiding this comment.
Not sure how much of this we plan on writing manually, but these functions all look alike, so maybe we could have something like:
const fulfillFromUpstream = getFulfillFromUpstreamFnFor('eth_getCode', { jsonrpc: '2.0', id: original.id, result: HARDCODED_BLOCK_NUMBER })
Similar comment for the function above. All these mocks for RPC call could just be a few lines of code to define the filter (e.g. eth_getCode, or isApproveSimulationCall, so that apps/cowswap-e2e-tests/src/support/mockApproveSimulation.ts is also covered, for example), the mocked object, and call the util function(s) that have most of the logic that's basically the same for all mocks. I think this way it will be a easier to write, review and reason about them.
|
|
||
| let body: JsonRpcEntry | JsonRpcEntry[] | ||
| try { | ||
| body = JSON.parse(request.postData() ?? '') as JsonRpcEntry | JsonRpcEntry[] |
There was a problem hiding this comment.
Why not request.postDataJSON()?
Also, while this one might not share the same util I mentioned above, as the filtering steps below are a bit different, some of it could still be a util. Something like this:
type RPCMockFn<T> = (entries: T[]) => void;
function mockRPC<T>(fn: RPCMockFn<T>): (route: Route) => Promise<Something> {
return (route: Route): Promise<Something> = {
const request = route.request()
if (request.method() !== 'POST') return route.fallback()
if (isConfiguredHost(request.url(), configuredChainIdByUrl)) return route.fallback()
let body: JsonRpcEntry | JsonRpcEntry[]
try {
body = request.postDataJSON() as JsonRpcEntry | JsonRpcEntry[]
} catch {
return route.fallback()
}
const entries = Array.isArray(body) ? body : [body]
fn(entries)
}
}
So here you'll do:
mockRPC<JsonRpcEntry >((entries) => {
const classified = entries.map((entry) => {
[...]
})
Also, we probably mention these utils in AGENTS.md or a skill so that future AI-written tests use them instead of duplicating code structures.
| export function logUnmockedRpcRequests(opts: UnmockedRpcLoggerOpts): void { | ||
| const { context, worker, test, logPath = DEFAULT_LOG_PATH } = opts | ||
|
|
||
| void context.route('**/*', async (route: Route) => { |
There was a problem hiding this comment.
Similar comment here: You could use a helper/util to create these handlers without that much repetition, in this case maybe something like mockPOST or mockNonRPCPost instead of mockRPC, that is, the helper would include this check too: if (!looksLikeJsonRpc(body)) return route.fallback()
| } | ||
|
|
||
| /** Matches the preflight `eth_call` simulating `approve(address,uint256)` against the same token this mock was set up for, before the real `eth_sendTransaction` is ever asked for. */ | ||
| function isApproveSimulationCall(entry: JsonRpcEntry | null | undefined, token: string): boolean { |
There was a problem hiding this comment.
I'd rename to isApproveTokenSimulationCall or something like that to avoid confusions with the one in the file above.
|
|
||
| return ( | ||
| <styledEl.ToggleWrapper> | ||
| <styledEl.ToggleWrapper className="approve-mode-selector"> |
There was a problem hiding this comment.
I think it's better to use data-testid (same comment for ids in apps/cowswap-frontend/src/modules/account/containers/AccountDetails/index.tsx and apps/cowswap-frontend/src/modules/ethFlow/pure/EthFlowStepper/index.tsx, even when an id or className might already exist for styling or other purposes, just to convey the idea that that's supposed to be a stable reference, as opposed to classes which I'd argue we should safely assume their main purpose is styling, so one might assume if no styling is attached to a specific class, it's safe to remove it.
In fact, I'll probably define the test IDs in come constant file, either:
- We hardcode them in the JSX, then have a script using grep or similar to extract them to a constants file to use in tests.
- Or we define the constants file and use it it both JSX and tests, but this has a small runtime cost and makes the bundle larger, while the other one doesn't, I we might argue test files and instrumentation should affect the real app as little as possible.
| const isSigning = tab.id === 'signing' | ||
| return ( | ||
| <styledEl.TabButton | ||
| className="orders-table_tab" |
There was a problem hiding this comment.
What's-with_theCasing here? 😵💫
Danziger
left a comment
Danziger
left a comment
There was a problem hiding this comment.
I think it would be great to have some shared utils to create the handlers so that PRs that add or change tests are easier to review and reason about. Also, this should help missing subtle differences, which might or might not be intentional, between different mocks.
It might also be good to settle on using only one type of selector to find elements for testing purposes. I'd go for data-testid and avoid id or className, even if those already exist for styling or other reasons. I might be fine with using data-testid + id if you want to distinguish those that are guaranteed to return only one match, but I'd definitely avoid classes.
4 participants
Coded smoke tests of market-orders: https://cow-protocol.eu1.qasphere.com/project/CS/tcase?folders=14&tags=198
What changed
market-orders.spec.ts(insufficient balance/allowance, approve-mode toggle, eth-flow wrap/unwrap, order fulfillment, cancellation, snackbars, activity list) plus one new limit-order case (LO-02).mockSwapFulfillment/manualSwapPagesetup with focused, reusable helpers undersrc/support/(mockOrderPosting,mockApproveTransaction,mockCancellableOrder,mockEthFlowTransaction,mockEthFlowOrderIndexing,mockFixedRateQuote,mockWrapTransaction,mockUnwrapTransaction,seedTrader,selectTokens,readTitledAmount,expectActivityStatus) and a newAccountModalpage object.multicall3,ethBlockNumber,ethEstimateGas,ethGetCode,ethGetTransactionCount,launchDarkly) pluslogUnmockedRpcRequestsfor diagnosing real traffic that reaches Infura/WalletConnect-relay/public-node hosts instead of the suite's own mocks; removes the narrowermocks/bungee.ts/mocks/nearIntents.tsthis generalizes.installNativeBalanceRoutehelper (mockEthFlowTransaction.ts) for the classify/patch-or-fetch-upstream JSON-RPC logic duplicated acrossmockEthFlowTransaction/mockWrapTransaction/mockUnwrapTransaction, and reuses the same per-entry classification inmockApproveTransaction— fixing a real bug where a mixed RPC batch nulled out entries that weren't ours to answer.mockWrapTransactionandmockOrderPosting.fulfill()were overwriting the trader's existing WETH/buy-token balance instead of adding to it;mockUnwrapTransactionnow rejects a second unwrap per handle instead of silently mis-computing the balance.toLowerCase()/===address comparisons withareAddressesEqual/getAddressKey(TokenSelector,mockEthFlowTransaction,market-orders.spec.ts) per the repo's address-handling rule.getUpdaterInterval(libs/common-const) — it usedMath.max, which left every polling interval unchanged in e2e mode; nowMath.minso mocked e2e tests don't have to wait out real-world poll intervals. Production cadence (outsidewindow.__COWSWAP_E2E__) is unchanged.SwapPage/ConfirmModal/AccountModal/LimitPagepage objects, and adds abeforeEachdefault balance so no "Connected EOA wallet" test falls back to a real balance fetch.getTokenFromMappingto normalize addresses viagetAddressKey(@cowprotocol/cow-sdk) instead of viem'sgetAddress, matching the same address-handling rule.data-testid/id/className) to several components (fiat amount, price impact, account activities list, approve toggle, orders table/tabs, eth-flow stepper, confirm-order amount row, snackbar) so specs have stable locators — no visual or behavioral change.apps/cowswap-e2e-tests/AGENTS.mdwith the new conventions.Why
feat/e2e-playwright-3).getUpdaterIntervalremoves multi-second real-time waits from specs without touching production polling behavior outside the e2e flag — theMath.max→Math.minfix was needed for that to actually take effect.getAddressKey/areAddressesEqualswaps bring the touched code in line with the rootAGENTS.mdrule against manual address normalization/comparison.QA Testing
Preview URL QA:
data-testid/id/classNamehooks (fiat amount, price impact, account activity list, approve toggle, orders table, eth-flow stepper, snackbar) didn't change layout or behavior: https://swap-dev-git-e2e-market-orders-cowswap-dev.vercel.appDeveloper verification:
market-orders.spec.ts,limit-orders.spec.ts) run under this PR's CITest/smokechecks, both passing: Test, smoke.npx tsx --test "src/**/*.test.ts"inapps/cowswap-e2e-tests) cover the fixed balance/classify logic directly — 119 passing.Reviewer note:
getUpdaterInterval, a no-op outside the e2e flag) and the twogetAddressKey/areAddressesEqualnormalization fixes, which are functionally equivalent to the prior checksum-based address handling.