-
-
Notifications
You must be signed in to change notification settings - Fork 443
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 FrameWaitForFunctionTests.ShouldPollOnMutation #2225
Fix FrameWaitForFunctionTests.ShouldPollOnMutation #2225
Conversation
public FrameWaitForFunctionTests(ITestOutputHelper output) : base(output) | ||
{ | ||
DefaultOptions = TestConstants.DefaultBrowserOptions(); |
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 is clever. Can would document what we're trying to accomplish here?
// We intercept the poller.start() call to prevent tests from continuing before the polling has started. | ||
_connectionTransportInterceptor.MessageSent += (_, message) => | ||
{ | ||
if (message.Contains("poller => poller.start()")) |
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.
Let's add a comment where we are executing the poller => poller.start()
. Mentioning that if we change the code there we need to update this test.
return startedPolling; | ||
} | ||
|
||
private sealed class ConnectionTransportInterceptor : IConnectionTransport |
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.
Let's move this to its own file and make it public. This is a great idea.
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.
Should I create a new folder, or leave it in WaitTaskTests?
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.
Let's move it to the root folder by now. There are many classes there that could be moved to an Utils
directory.
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.
Let's move it to the root folder by now. There are many classes there that could be moved to an Utils
directory.
@@ -216,5 +232,59 @@ public async Task ShouldSurviveNavigations() | |||
await Page.EvaluateFunctionAsync("() => window.__done = true"); | |||
await watchdog; | |||
} | |||
|
|||
private TaskCompletionSource<bool> ListenForStartPolling() |
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 can return only the task and call this WaitForStartPollingAsync
…tart()`. The previous attempt still did not guarantee the correct execution, but this should.
d860afb
to
b6cee6c
Compare
The additional
EvaluateExpressionAsync
ensures that the poller has started before the test proceeds.I also took the liberty to refactor and simplify the
EvaluateExpressionAsync
to avoid any changes to DOM. This seems to have the same effect and wouldn't interfere with the mutation observer.