Fix race condition in EventTest.PreventDefault_DoNotApplyByDefault #63384
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a race condition in the EventTest.PreventDefault_DoNotApplyByDefault test that was causing flaky test failures. The test was quarantined due to intermittent failures when the browser didn't complete the URL navigation in time for the assertion.
Key changes:
- Removes the quarantine attribute and fixes test name typo
- Replaces immediate assertion with WebDriverWait to handle timing issues
- Adds explanatory comment for the expected behavior
| // The URL should change because the submit event is not prevented | ||
| var wait = new WebDriverWait(Browser, TimeSpan.FromSeconds(3)); | ||
| wait.Until(driver => driver.Url.Contains("about:blank")); |
There was a problem hiding this comment.
Check the extensions on Browser they centralize things like the wait times and stuff like that, it might be better to use those.
There was a problem hiding this comment.
Thanks for the suggestion. I looked into the existing extensions and did not find one that would cover this kind of wait, so I added one. Is it ok like this?
| @@ -284,8 +284,7 @@ public void PreventDefault_DoNotApplyByDefault() | |||
| appElement.FindElement(By.Id("form-2-button")).Click(); | |||
|
|
|||
| // The URL should change because the submit event is not prevented | |||
| var wait = new WebDriverWait(Browser, TimeSpan.FromSeconds(3)); | |||
| wait.Until(driver => driver.Url.Contains("about:blank")); | |||
| Browser.WaitForUrlToContain("about:blank"); | |||
There was a problem hiding this comment.
I think so? But you can also search for Browser.Url usages and you might find an existing pattern.
There was a problem hiding this comment.
Ok, there should be implicit waits for this with 'Browser.Contains' or 'Browser.True'. (I see now what extensions you meant.)
There was a problem hiding this comment.
I reverted the unneeded extension method.
oroztocil
deleted the
oroztocil/62533-fix-test-PreventDefault_DoNotApplyByDefault
branch
The quarantined test was flaky due to a race condition. The assert failed when the test driver did not manage to change the URL in time.
I used a script to execute the test in a loop (on an M2 Mac):
Fixes #62533