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: [POM] Migrate token tests #29375

Merged
merged 32 commits into from
Jan 8, 2025
Merged

test: [POM] Migrate token tests #29375

merged 32 commits into from
Jan 8, 2025

Conversation

cmd-ob
Copy link
Contributor

@cmd-ob cmd-ob commented Dec 20, 2024

Description

  • Updates asset-list Page Object - Adds methods for interacting with token list (i.e sorting, getting list, assertion on increase/decrease price and percentage)
  • Adds new method for importing a custom token using contract address
  • Adds new method for adding multiple tokens by search in one step
  • Minor update to send-token page for warning message
  • New page object for token-overview
  • Tests Updated - import-tokens, send-erc20-to-contract, token-list and token-sort

Related issues

Fixes:

Manual testing steps

  • All tests must pass on CI

Screenshots/Recordings

Before

After

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 Dec 20, 2024
private readonly swapButton = {
text: 'Swap',
css: '.icon-button',
};
Copy link
Contributor Author

@cmd-ob cmd-ob Dec 20, 2024

Choose a reason for hiding this comment

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

Using text based selectors here, it's more accessible and means we don't have to create multiple selectors depending on the type of token screen we are looking at. i.e Is the token enabled for swap, bridge, send etc.

The other selector would be either coin-overview-* or token-overview-* or eth-overview-*
This would cause flakiness in the e2e test. I do think there is a bug on the correct selector/element being rendered. It is not affecting the user but they might need to cover it with unit tests so something else does not break

@cmd-ob cmd-ob requested review from seaona and chloeYue December 20, 2024 07:57
@cmd-ob cmd-ob marked this pull request as ready for review December 20, 2024 07:58
@cmd-ob cmd-ob enabled auto-merge December 20, 2024 09:32
@metamaskbot
Copy link
Collaborator

Builds ready [7f54477]
Page Load Metrics (1801 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27220811725368177
domContentLoaded14932069175814268
load14982084180116278
domInteractive2293462010
backgroundConnect7288436029
firstReactRender16101503015
getState565212211
initialActions01000
loadScripts10431559129412560
setupStore68713178
uiStartup183528322192278134
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

const percentageElement = await this.driver.findElement(
this.tokenPercentage(assetAddress),
);
const percentage = await percentageElement.getText();
Copy link
Contributor

@seaona seaona Dec 20, 2024

Choose a reason for hiding this comment

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

mmm this is an anti-pattern that opens the door to race conditions, where the element is rendered but it does not have yet the value we expect.
We should try to avoid finding an element and then getting its text where possible. I think in this case it's possible to avoid this. In the specs below, we should directly try to find the element by its expected inner value instead of this assert

const percentage =
  await assetListPage.getAssetPercentageIncreaseDecrease(tokenAddress);
assert.equal(percentage, '');

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I agree with you here. I have used isElementPresent to check for the existence of the element for a given token address


private readonly swapButton = {
text: 'Swap',
css: '.icon-button',
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of small things:

  • could we sort these selectors alphabetically maybe?
  • I think these buttons are the exact same buttons we have in the home screen. Maybe we could separate them in their own component, and re-use them in the homepage class and in the token overview class? Just a thought, it doesn't have to be in this PR. The advantage would be that if a button from this group changes, we'll only need to update it in one place. The disadvantage might be that it might complicate a bit the component? though not too much. What are your thoughts? In any case, it can be left outside this PR, if we decide to have this separate component

Screenshot from 2024-12-20 10-45-01

Screenshot from 2024-12-20 10-45-09

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I also had the same thought. It could make sense (if we keep it text based as I have done here)
the only doubt that I have is the depending on the token type different buttons are displayed so that is still something to think about. I do agree though there is a way we could make this easier to deal with

await driver.findElement(`[data-testid="${testId}"]`)
).getText();
const percentage =
await assetListPage.getAssetPercentageIncreaseDecrease(tokenAddress);
assert.equal(percentage, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

with these 2 asserts race conditions can happen (see above comment)

chloeYue
chloeYue previously approved these changes Dec 20, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [c09fd7d]
Page Load Metrics (1613 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14581852162210952
domContentLoaded1414176515919445
load1458181216139847
domInteractive23117382311
backgroundConnect8102212210
firstReactRender1673412512
getState44715157
initialActions01000
loadScripts1036131711668239
setupStore66711136
uiStartup16592040184010450
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cmd-ob cmd-ob requested a review from seaona December 20, 2024 15:11
@metamaskbot
Copy link
Collaborator

Builds ready [7888d66]
Page Load Metrics (1571 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1424179315739345
domContentLoaded1410174115409043
load1424179515719747
domInteractive207738189
backgroundConnect863312010
firstReactRender1567292010
getState4569115
initialActions01000
loadScripts1034134811618340
setupStore65518199
uiStartup15921992177210349

@metamaskbot
Copy link
Collaborator

Builds ready [15854d4]
Page Load Metrics (1717 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30820471586424204
domContentLoaded15181955168812359
load15522057171714168
domInteractive25162463718
backgroundConnect8129383517
firstReactRender16107613316
getState5112222713
initialActions01000
loadScripts11181495125010450
setupStore678222412
uiStartup176129412106277133

@chloeYue
Copy link
Contributor

chloeYue commented Jan 7, 2025

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

@metamaskbot
Copy link
Collaborator

Builds ready [9f39c02]
Page Load Metrics (1670 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14552152168015574
domContentLoaded14472081164915072
load14562086167014871
domInteractive20107442311
backgroundConnect86327188
firstReactRender15100432713
getState468202211
initialActions01000
loadScripts9991610121713665
setupStore68712178
uiStartup16232396195217885

@cmd-ob cmd-ob requested a review from chloeYue January 8, 2025 09:40
Copy link
Contributor

@chloeYue chloeYue left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@seaona seaona left a comment

Choose a reason for hiding this comment

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

Overall the new changes look good to me! I've just added a couple of small nits. Feel free to take them or leave them

private readonly tokenSymbolInput =
'[data-testid="import-tokens-modal-custom-symbol"]';

private readonly modalWarningBanner = 'div.mm-banner-alert--severity-warning';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe we can improve the selector by adding a data test id in the Import Token component (here), like this:

<BannerAlert
    severity={
       isDynamicTokenListAvailable
          ? Severity.Warning
           : Severity.Info
     }
     data-testid='custom-token-warning'
>

and then use it as the selector

Suggested change
private readonly modalWarningBanner = 'div.mm-banner-alert--severity-warning';
private readonly modalWarningBanner = '[data-testid="custom-token-warning"]';

Comment on lines +340 to +348
const isPresent = await this.driver.isElementPresentAndVisible({
css: this.tokenPercentage(address),
text: expectedChange,
});
if (!isPresent) {
throw new Error(
`Token general change percentage ${expectedChange} not found for address ${address}`,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this simplifies a bit the code and readability. Note that waitForSelector already waits for an element with visible state by default

Suggested change
const isPresent = await this.driver.isElementPresentAndVisible({
css: this.tokenPercentage(address),
text: expectedChange,
});
if (!isPresent) {
throw new Error(
`Token general change percentage ${expectedChange} not found for address ${address}`,
);
}
await this.driver.waitForSelector({
css: this.tokenPercentage(address),
text: expectedChange,
});

Comment on lines +381 to +389
const isPresent = await this.driver.isElementPresentAndVisible({
css: this.tokenIncreaseDecreaseValue,
text: expectedChangeValue,
});
if (!isPresent) {
throw new Error(
`Token general change value ${expectedChangeValue} not found`,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this simplifies a bit the code and readability. Note that waitForSelector already waits for an element with visible state by default

Suggested change
const isPresent = await this.driver.isElementPresentAndVisible({
css: this.tokenIncreaseDecreaseValue,
text: expectedChangeValue,
});
if (!isPresent) {
throw new Error(
`Token general change value ${expectedChangeValue} not found`,
);
}
await this.driver.waitForSelector({
css: this.tokenIncreaseDecreaseValue,
text: expectedChangeValue,
});

);
const initialTokenList = await assetListPage.getTokenListNames();
assert.ok(initialTokenList[0].includes('Ethereum'));
await assetListPage.sortTokenList('alphabetically');
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this check could be a method inside the asset-list class, something like:

assetListPage.isTokenInAssetsList(tokenName)

So it can be utilized in more specs if that's needed. What do you think?

@cmd-ob cmd-ob added this pull request to the merge queue Jan 8, 2025
Merged via the queue into main with commit 7fe00dc Jan 8, 2025
76 checks passed
@cmd-ob cmd-ob deleted the qq-more-token-tests branch January 8, 2025 10:40
@github-actions github-actions bot locked and limited conversation to collaborators Jan 8, 2025
@metamaskbot metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.11.0 Issue or pull request that will be included in release 12.11.0 team-qa QA team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants