Skip to content
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

test: migrate more token tests and update related page objects #29651

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

cmd-ob
Copy link
Contributor

@cmd-ob cmd-ob commented Jan 13, 2025

Description

This PR introduces significant improvements to the token-related E2E tests and page objects, focusing on better test coverage and maintainability. The changes include:

  1. Enhanced token management functionality:

    • New page objects for token approval and transfer modals
    • Improved token existence checks
    • Streamlined gas fee editing capabilities
    • Better navigation controls for token-related features
  2. Test infrastructure improvements:

    • Added data-testid attributes for more reliable testing
    • Consolidated mock functions
    • Improved transaction validation checks
    • Enhanced custom token creation and approval process testing
    • Fixed flaky tests
  3. Code quality updates:

    • Reorganised dapp page object structure
    • Improved code maintainability through refactoring

Related issues

N/A

Manual testing steps

  1. Run the E2E test suite focusing on token-related tests
  2. Verify token test flows with the new page objects

Screenshots/Recordings

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

Copy link
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.

@metamaskbot metamaskbot added the team-qa QA team label Jan 13, 2025
@cmd-ob cmd-ob changed the title E2e/tokens v2 test: migrate more token tests and update related page objects Jan 13, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [50f87f5]
Page Load Metrics (1617 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1480192016259746
domContentLoaded1462189216009646
load1482190116179445
domInteractive25182634823
backgroundConnect85120136
firstReactRender1597502814
getState45313157
initialActions01000
loadScripts1056141911838139
setupStore65912147
uiStartup16632371192819091
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 108 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cmd-ob cmd-ob requested review from chloeYue and seaona January 13, 2025 08:49
@metamaskbot metamaskbot added INVALID-PR-TEMPLATE PR's body doesn't match template and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Jan 13, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [af93374]
Page Load Metrics (1816 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34423011648456219
domContentLoaded147324551792222106
load150424631816223107
domInteractive219043178
backgroundConnect971282210
firstReactRender1691512613
getState56017168
initialActions01000
loadScripts10791859132318086
setupStore66318199
uiStartup168830012093288138
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 108 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cmd-ob cmd-ob marked this pull request as ready for review January 13, 2025 09:34
@cmd-ob cmd-ob requested review from a team as code owners January 13, 2025 09:34
@hjetpoluru
Copy link
Contributor

@cmd-ob There is lint and CI failure for the test could you please check.

Comment on lines +36 to +43
async check_SuggestedTokensCount(expectedTokenCount: number) {
const multipleSuggestedTokens = await this.driver.findElements(
this.tokenListItem,
);

// Confirm the expected number of tokens are present as suggested token list
assert.equal(multipleSuggestedTokens.length, expectedTokenCount);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async check_SuggestedTokensCount(expectedTokenCount: number) {
const multipleSuggestedTokens = await this.driver.findElements(
this.tokenListItem,
);
// Confirm the expected number of tokens are present as suggested token list
assert.equal(multipleSuggestedTokens.length, expectedTokenCount);
}
async check_suggestedTokensCount(
expectedTokenCount: number,
): Promise<void> {
console.log(
`Check ${expectedTokenCount} suggested tokens are displayed`,
);
await this.driver.wait(async () => {
const multipleSuggestedTokens = await this.driver.findElements(
this.tokenListItem,
);
return multipleSuggestedTokens.length === expectedTokenCount;
}, 10000);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i made a code suggestion to make the method more robust and mitigate flakiness

Comment on lines +313 to +318
await this.driver.waitUntil(
async () => {
return await this.driver.isElementPresentAndVisible(this.priceChart);
},
{ timeout: 2000, interval: 100 },
);
Copy link
Contributor

Choose a reason for hiding this comment

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

small question: is there a reason why we don't use await driver.waitForSelector(this.priceChart); here?

css: 'p',
text: tEn('networkFee') as string,
};
async confirmTx(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: could we have this as clickConfirmTx or clickConfirmButton

Copy link
Contributor

Choose a reason for hiding this comment

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

Same question why there is duplicate function

* @param gasLimit - The gas limit value to set
* @param gasPrice - The gas price value to set
*/
async editGasFee(gasLimit: string, gasPrice: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the editGasFee duplicate function in all the files

async editGasFee(gasLimit: string, gasPrice: string): Promise<void> {

async editGasFee(gasLimit: string, gasPrice: string): Promise<void> {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-qa QA team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants