-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[wasm][debugger] Fix some racy tests #73524
Conversation
.. contains the assembly contents which can be large .
.. the updated method becomes callable, instead of `Thread.Sleep`.
… spot in test failures
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsSome of the hot reload tests fail randomly because they call the updated methods too early. Instead, wait for the Fixes #66024 .
|
.. and notification handlers, since they can be modified from different threads. Fixes dotnet#69144 .
The debugger test failure is #73528 . |
@@ -215,6 +218,15 @@ async Task OnMessage(string method, JObject args, CancellationToken token) | |||
default: _logger.LogInformation(line); break; | |||
} | |||
|
|||
if (!_gotAppReady && line == "console.debug: #debugger-app-ready#") |
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.
cool!
- ChromeProvider starts the browser and then waits for a special string which indicates that the browser is ready for connections. - Firefox doesn't emit anything similar, so it waits for `console.log: ready` message we get from `debugger-driver.html`. - but since we wait for this in `FirefoxProvider`, when we start the `Inspector` after this, the ready message is already gone, so it will never see it. - To avoid this, if we change the connection logic to instead a. start the browser b. wait till we can connect successfully c. and then start `Inspector`, then it can still be too late if the page starts up, and the message is emitted before the Inspector was started. - A proper fix would be to start the browser but with no url specified. And once we establish a connection to it, *then* send a command to open a url. For now, we'll emit the old message `console.log: ready` too, just for consumption by the firefox tests. And this can be fixed property in the future.
1. Fix random HotReload test failures
Some of the hot reload tests fail randomly because they call the updated methods too early.
The tests used
Thread.Sleep(3000)
to wait for the method to get updated, and the proxyto respond to that. And that's essentially racy.
Instead, wait for the
breakpointResolved
event, or thescriptParsed
events for the methods,as appropriate.
Fixes #66024
Fixes #72946
2. Fix race in adding/removing event handlers
Use ConcurrentDictionary for event listeners, and notification handlers, since they can be modified
from different threads.
Fixes #69144 .
3. Fix a race condition where the tests start calling methods before the app is ready
Fixes #73528