-
Notifications
You must be signed in to change notification settings - Fork 348
Use service extension in networking integration test #9323
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
I don't know why DCM is mad. I didn't touch a single file for which a DCM warning is reported. :/ |
I think you just need to sync with the master branch - Kenzie recently updated the DCM version we are using:#9310 |
packages/devtools_app/integration_test/test/live_connection/network_screen_test.dart
Show resolved
Hide resolved
packages/devtools_app/test/test_infra/fixtures/networking_app/bin/main.dart
Outdated
Show resolved
Hide resolved
|
||
registerExtension('ext.networking_app.exit', (_, parameters) async { | ||
unawaited( | ||
Future.delayed(const Duration(milliseconds: 200)).then((_) => io.exit(0)), |
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.
Same question about delayed here
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.
Ah, I'll add a comment here. The issue is that this service extension triggers io.exit(0)
, but we still need to return a value. I tried unawaited(Future.microtask(() => io.exit(0)));
, but that resulted in a test error, that the connected app shutdown unexpectedly. So I think we need to return a ServiceExtensionResponse, let it get through some code layers asynchronously via microtasks, so that the response makes it all the way to DevTools, and then we can exit.
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.
Got it, thanks!
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.
Thank you!
packages/devtools_app/test/test_infra/fixtures/networking_app/bin/main.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/integration_test/test/live_connection/network_screen_test.dart
Show resolved
Hide resolved
|
||
registerExtension('ext.networking_app.exit', (_, parameters) async { | ||
unawaited( | ||
Future.delayed(const Duration(milliseconds: 200)).then((_) => io.exit(0)), |
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.
Ah, I'll add a comment here. The issue is that this service extension triggers io.exit(0)
, but we still need to return a value. I tried unawaited(Future.microtask(() => io.exit(0)));
, but that resulted in a test error, that the connected app shutdown unexpectedly. So I think we need to return a ServiceExtensionResponse, let it get through some code layers asynchronously via microtasks, so that the response makes it all the way to DevTools, and then we can exit.
This change is a no-op. We change the communication between the integration test itself and the test app, from an HTTP connection to a VM service extension system. This vastly simplifies the test.
The motivation for this PR is to support a "test app" hot restart, in the integration test. Since this PR is quite large already, I'm saving that for the next PR.