-
Notifications
You must be signed in to change notification settings - Fork 5.4k
test: add back skipped solana tests #38449
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: main
Are you sure you want to change the base?
test: add back skipped solana tests #38449
Conversation
adding driver delay as these tests rely on snap balances which are async
|
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. |
Builds ready [4cf490a]
UI Startup Metrics (1228 ± 102 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| driver, | ||
| ); | ||
|
|
||
| await driver.delay(5000); |
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.
I would like QA thoughts on this.
These tests do pass, but balances are async and require a little bit of time to load (even if the API requests are mocked)
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.
NOTE, that these tests for solana are with the BIP-44 state flag set to 0 - BIP-44 is not enabled).
If BIP-44 is enabled, it would require a much larger UI change for these e2e tests to pass. Example changes:
- solana is added directly into each account, so there is no need to add a solana account.
- the home screen will show an aggregated balance across EVM and non-EVM.
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.
Bug: Missing delay in similar non-zero balance test
The test "full flow of USD with a positive balance account" is missing the driver.delay(5000) that was added to "full flow of SOL with a positive balance account" at line 201. Both tests have identical configurations (showNativeTokenAsMainBalance: true, mockGetTransactionSuccess: true) and expect a non-zero balance of '50'. If async balance loading requires a delay for the SOL test before checkPageIsLoaded('50'), the USD test with the same setup likely needs it too, otherwise it may be flaky.
test/e2e/tests/solana/send-flow.spec.ts#L64-L75
metamask-extension/test/e2e/tests/solana/send-flow.spec.ts
Lines 64 to 75 in 4cf490a
| it('full flow of USD with a positive balance account', async function () { | |
| this.timeout(120000); | |
| await withSolanaAccountSnap( | |
| { | |
| title: this.test?.fullTitle(), | |
| showNativeTokenAsMainBalance: true, | |
| mockGetTransactionSuccess: true, | |
| }, | |
| async (driver) => { | |
| const homePage = new NonEvmHomepage(driver); | |
| await homePage.checkPageIsLoaded('50'); |
test/e2e/tests/solana/send-flow.spec.ts#L200-L203
metamask-extension/test/e2e/tests/solana/send-flow.spec.ts
Lines 200 to 203 in 4cf490a
| async (driver) => { | |
| await driver.delay(5000); | |
| const homePage = new NonEvmHomepage(driver); | |
| await homePage.checkPageIsLoaded('50'); |
Bug: Missing delay before identical balance check method call
The "and transaction fails" test in send-flow.spec.ts calls checkGetBalance('50', 'SOL') without a delay before it, while the nearly identical "For a non 0 balance account - SOL balance" test in check-balance.spec.ts has driver.delay(5000) added before the same call. Both tests have showNativeTokenAsMainBalance: true and default non-zero balance configuration. If async balance loading requires a delay before this assertion in one file, it likely requires it in the other as well.
test/e2e/tests/solana/send-flow.spec.ts#L317-L320
metamask-extension/test/e2e/tests/solana/send-flow.spec.ts
Lines 317 to 320 in 4cf490a
| }, | |
| async (driver) => { | |
| const homePage = new NonEvmHomepage(driver); | |
| await homePage.checkGetBalance('50', 'SOL'); |
test/e2e/tests/solana/check-balance.spec.ts#L108-L110
metamask-extension/test/e2e/tests/solana/check-balance.spec.ts
Lines 108 to 110 in 4cf490a
| const homePage = new NonEvmHomepage(driver); | |
| await driver.delay(5000); | |
| await homePage.checkGetBalance('50', 'SOL'); |
Description
adding driver delay as these tests rely on snap balances which are async
Changelog
CHANGELOG entry: null
Related issues
Fixes: #37824, https://consensyssoftware.atlassian.net/browse/ASSETS-1809
Manual testing steps
N/A
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Re-enables previously skipped Solana E2E tests and adds short driver delays to stabilize async balance and activity assertions.
test/e2e/tests/send/send-solana.spec.tstest/e2e/tests/solana/send-flow.spec.tstest/e2e/tests/solana/transaction-activity-list.spec.tstest/e2e/tests/solana/check-balance.spec.ts.await driver.delay(5000)before balance checks, activity assertions, and send flows to wait for snap-driven async data (e.g., incheck-balance.spec.ts,send-solana.spec.ts,send-flow.spec.ts,transaction-activity-list.spec.ts).50 SOLbefore proceeding insend-solana.spec.ts.Written by Cursor Bugbot for commit 4cf490a. This will update automatically on new commits. Configure here.