fix: MMQA-1546 address PR review comments for perps e2e POM files#40661
Merged
fix: MMQA-1546 address PR review comments for perps e2e POM files#40661
Conversation
Contributor
✨ Files requiring CODEOWNER review ✨👨🔧 @MetaMask/perps (10 files, +412 -346)
🧪 @MetaMask/qa (5 files, +286 -315)
|
Contributor
Builds ready [39cd291]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
7 tasks
- Replace hardcoded inline CSS selectors with named class properties using testId objects where possible - Extract shared position-related selectors and methods into PerpsPositionsBase to reduce duplication between PerpsTabPage and PerpsHomePage - Remove unused methods: waitForSubmitButtonEnabled, waitForOrderFormClosed (not used, untestable until consumed) - Remove flaky getPositionCardsCount pattern from both PerpsHomePage and PerpsTabPage - Extract inline selector literals (tutorial buttons, recent activity, balance actions, modify/close buttons) to named class properties - Order selectors and methods alphabetically in all POM files - Add JSDoc explaining window.location.hash SPA navigation approach - Convert searchInput from raw CSS to testId object format Co-authored-by: Ramon AC <racitores@users.noreply.github.com>
39cd291 to
c3e94f6
Compare
Contributor
|
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. |
Contributor
Builds ready [b90d329]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Contributor
Builds ready [a16bd81]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Contributor
Builds ready [3a2dbbe]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
seaona
reviewed
Mar 11, 2026
Comment on lines
+79
to
+82
| await this.driver.waitForSelector(this.addFundsButton); | ||
| const addFundsElement = await this.driver.findElement(this.addFundsButton); | ||
| await this.driver.scrollToElement(addFundsElement); | ||
| await this.driver.clickElement(this.addFundsButton); |
Member
There was a problem hiding this comment.
We could avoid all these code, and just clickEment, that should handle any logic needed
Suggested change
| await this.driver.waitForSelector(this.addFundsButton); | |
| const addFundsElement = await this.driver.findElement(this.addFundsButton); | |
| await this.driver.scrollToElement(addFundsElement); | |
| await this.driver.clickElement(this.addFundsButton); | |
| await this.driver.clickElement(this.addFundsButton); |
| */ | ||
| async clickLearnBasics(): Promise<void> { | ||
| await this.driver.clickElement({ testId: 'perps-learn-basics' }); | ||
| async waitForBalanceSection(timeout?: number): Promise<void> { |
Member
There was a problem hiding this comment.
why are we passing an optional timeout here? Is this needed? I don't see it used anywhere 🤔
test/e2e/tests/perps/helpers.ts
Outdated
|
|
||
| /** Base route for Perps market detail. Append encoded symbol, e.g. PERPS_MARKET_DETAIL_ROUTE + '/AVAX' */ | ||
| export const PERPS_MARKET_DETAIL_ROUTE = '#/perps/market'; | ||
|
|
Member
There was a problem hiding this comment.
could we move the perps-fixture-config.ts file out from the generic fixtures folder?
That folder is used for the general setup of the fixtures, not for a specific test setup.
The file perps-fixture-config.ts just has a specific fixture config which could go either in helpers or directly in the e2e file
| ...getConfig(this.test?.fullTitle()), | ||
| }, | ||
| async ({ driver }: { driver: Driver }) => { | ||
| await loginWithoutBalanceValidation(driver); |
Member
There was a problem hiding this comment.
could we use loginWithBalanceValidation? that is the preferred method to avoid race conditions
| async waitForPositionsSection(): Promise<void> { | ||
| await this.driver.waitForSelector(this.perpsPositionsSection); | ||
| async checkPageIsLoaded(): Promise<void> { | ||
| await this.driver.waitForSelector(this.perpsTabView); |
Member
There was a problem hiding this comment.
for checking the page is loaded, could we wait for multiple selectors to make it more robust? (it's a general convention)
Example:
await this.driver.waitForMultipleSelectors([
this.accountMenuButton,
this.threeDotMenuButton,
]);
| async clickShort(): Promise<void> { | ||
| await this.driver.clickElement(this.shortCtaButton); | ||
| async clickPercentPreset25(): Promise<void> { | ||
| await this.driver.clickElement(this.percentPreset25); |
Member
There was a problem hiding this comment.
instead of having a specific method like this, could we pass a percentage in the param, so then we could re-use this for other percentages?
| async clickPercentPreset25(): Promise<void> { | ||
| await this.driver.clickElement(this.percentPreset25); | ||
| async clickSubmitOrder(): Promise<void> { | ||
| await this.driver.clickElement(this.submitOrderButton); |
Member
There was a problem hiding this comment.
here we possibly want this as well (if it takes long, then we can pass a custom timeout here ie
Suggested change
| await this.driver.clickElement(this.submitOrderButton); | |
| await this.driver.clickElementAndWaitToDisappear(this.submitOrderButton, 10000); |
| timeout, | ||
| }); | ||
| async waitForPageLoaded(): Promise<void> { | ||
| await this.driver.waitForSelector(this.marketDetailPage); |
Member
There was a problem hiding this comment.
for following the conventions could we rename this to checkPageIsLoaded and also use multiple selectors to ensure the page is fully loaded? `waitForMultipleSelectorsà
| * Waits for the market list view to be visible. | ||
| */ | ||
| async waitForPageLoaded(): Promise<void> { | ||
| await this.driver.waitForSelector(this.marketListView); |
Member
There was a problem hiding this comment.
same: for following the conventions could we rename this to checkPageIsLoaded and also use multiple selectors to ensure the page is fully loaded? `waitForMultipleSelectorsà
| import { PerpsHomePage } from '../../page-objects/pages/perps/perps-home-page'; | ||
| import { getConfig } from './helpers'; | ||
|
|
||
| async function waitForWebsocketConnections( |
Member
There was a problem hiding this comment.
could we rename this for clarity
waitForPerpsWebsocketConnections
Contributor
Builds ready [bbec66d]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
|
Contributor
Builds ready [5599513]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
seaona
approved these changes
Mar 17, 2026
Member
seaona
left a comment
There was a problem hiding this comment.
Added a couple non-blocking comments but can be addressed in subsequent PRs
| async clickSearchButton(): Promise<void> { | ||
| await this.driver.clickElement(this.perpsExploreMarketsRow); | ||
| async navigateToPerpsHome(): Promise<void> { | ||
| await this.driver.waitForSelector(this.accountOverviewPerpsTab); |
Member
There was a problem hiding this comment.
this is not needed, the clickElement already does this for you
| await this.driver.clickElement(this.submitOrderButton); | ||
| async navigateToMarket(symbol: string): Promise<void> { | ||
| const marketRowTestId = `market-row-${symbol.replaceAll(':', '-')}`; | ||
| await this.driver.waitForSelector({ testId: marketRowTestId }); |
Member
There was a problem hiding this comment.
this is not needed, the clickElement does this for you
| */ | ||
| async navigateToMarketList(): Promise<void> { | ||
| await this.driver.waitForSelector(this.exploreMarketsRow); | ||
| await this.driver.clickElementSafe(this.toastCloseButton, 1500); |
Member
There was a problem hiding this comment.
it would be interesting to know when this toast appears and when it doesn't, to have full control of the expects
aganglada
approved these changes
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Changelog
CHANGELOG entry:
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Low risk: changes are confined to E2E test page objects and websocket mock utilities, but navigation/click behavior was adjusted to reduce flakiness and could affect existing perps specs.
Overview
Refactors Perps E2E page objects to remove hash-based SPA navigation and centralize shared position helpers in a new
PerpsPositionsBase, withPerpsTabPage/PerpsHomePagenow navigating via tab clicks and usingwaitForMultipleSelectors/waitUntilfor more robust waits.Updates market list/detail flows to navigate by clicking UI rows (including toast dismissal and safer mouse-move clicks), adds/renames small helper actions (e.g. generic percent preset click, submit waits for disappearance), and adjusts perps specs to follow the new navigation sequence.
Unifies WebSocket mock typing via a new shared
test/e2e/websocket/types.ts(supporting function responses) and updates Solana/Perps/AccountActivity mock senders accordingly; also adds a new skippedweb-socket-connection.spec.tsfor future PerpsController integration.Written by Cursor Bugbot for commit 5599513. This will update automatically on new commits. Configure here.