-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: flaky test 4byte setting does not try to get contract method name from 4byte when the setting is off
#27560
Conversation
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. |
assert.equal( | ||
await actionElement.getText(), | ||
contractInteraction.toUpperCase(), | ||
); |
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.
this way of asserting can cause race conditions, when the element it's present but not the value we expected. This is an anti-pattern, that needs to be switch by waiting for a value (text) we want. This way we can safely remove the delay
@@ -83,28 +78,23 @@ describe('4byte setting', function () { | |||
await openDapp(driver, contractAddress); | |||
|
|||
// wait for deployed contract, calls and confirms a contract method where ETH is sent | |||
await driver.findClickableElement('#depositButton'); |
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.
not needed
text: 'Deposit initiated', | ||
}); | ||
|
||
await driver.waitUntilXWindowHandles(3); |
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.
not needed
|
||
// deploy contract | ||
await openDapp(driver, contractAddress); | ||
|
||
// wait for deployed contract, calls and confirms a contract method where ETH is sent | ||
await driver.delay(largeDelayMs); |
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.
if we make sure we login with balance validation, we can safely remove this delay
await driver.waitForSelector({ | ||
css: 'span', | ||
text: 'Deposit initiated', | ||
}); |
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.
we don't care if the test dapp loads this value in time or not
text: 'Deposit', | ||
}); | ||
assert.equal(await actionElement.getText(), 'DEPOSIT'); |
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.
asserting an element by it's inner value to match an expect value, opens the door to race conditions. We should find the element by its value directly. I've also added the 2 conditions we want (deposit is there, and contract interaction is not there)
Quality Gate passedIssues Measures |
Builds ready [fdfa487]
Page Load Metrics (1953 ± 141 ms)
Bundle size diffs
|
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! Thanks for the fix !
Description
This test is failing as it's trying to look for the
Deposit initiated
element in the test dapp and it doesn't appear in time.We shouldn't care if the test dapp sets the value to Deposit initiated into its div element, as long as the popup is open (which it does). This removes any potential race condition on the test dapp side
There are several things to fix/improve around the 2 specs for 4byte, so I took the opportunity to fix those too.
Related issues
Fixes: #21494
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist