-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: flaky test Token List..., Token Details, Send ERC20because they broke with error can't convert undefined to object
#37302
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
Conversation
✨ Files requiring CODEOWNER review ✨🧪 @MetaMask/qa (1 files, +7 -0)
|
|
I wonder what the root cause of the failure is. Most initialization steps should be completed by the time the UI loads, I'm not aware of anything else in the background that would be possible to call "too early" |
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [3121209]
UI Startup Metrics (1309 ± 85 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
I'm not sure neither 🤔 Is it possible that changes on the multichain work have caused this? ie if the multichain asset list is not yet 'created', trying to Import a token might break the wallet, if we try to access it? This is a guess, but also unsure. I opened a ticket here to investigate on the wallet side: #37303 |
|
|
||
| private readonly multichainTokenListButton = { | ||
| testId: 'multichain-token-list-button', | ||
| }; |
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: Invalid Selector Format Causes Runtime Failures
The multichainTokenListButton selector uses an invalid format. It's defined as { testId: 'multichain-token-list-button' }, but the driver's waitForSelector method doesn't recognize the testId property. It should be formatted as a string selector '[data-testid="multichain-token-list-button"]' to match the existing pattern in this class (see tokenListItem at line 106-107). This will cause runtime failures when waitForSelector(this.multichainTokenListButton) is called in the import methods.
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.
mmm this is not correct 🤔 passing a test id key like this as a selector, does work. @HowardBraham added the logic for it here:
https://github.com/MetaMask/metamask-extension/blob/main/test/e2e/webdriver/driver.js#L238
Token List Sorting should sort tokens alphabetically and by decreasing balanceToken List..., Token Details, Send ERC20because they broke with error can't convert undefined to object
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [5c4023d]
UI Startup Metrics (1205 ± 112 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
chloeYue
left a comment
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.
the fix looks good to me !
Gudahtt
left a comment
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.
LGTM!
Description
The following specs are flaky at the same point:
If we login to the Homepage and very fast click Import, the wallet breaks with the error
can't convert undefined to object. This is a race condition on the wallet side (issue here).However, we can fix the test by waiting deterministically for the multichain asset list (whichever we have) to be loaded, before clicking import. This solves the issue and it's mitigated on the page class, so all tests are indirectly more stable because of this now.
See repro:
broken-import.mov
Changelog
CHANGELOG entry:
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Ensure e2e token import flows wait for the multichain asset list to load to reduce flakiness.
multichainTokenListButtonselector.multichainTokenListButtonbefore initiating import flows inimportCustomTokenByChain,importTokenBySearch, andimportMultipleTokensBySearchto ensure asset list is loaded.Written by Cursor Bugbot for commit 5c4023d. This will update automatically on new commits. Configure here.