-
Notifications
You must be signed in to change notification settings - Fork 345
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
base: master
Are you sure you want to change the base?
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 |
.callServiceExtensionOnMainIsolate('ext.networking_app.exit'); | ||
logStatus(response.toString()); | ||
|
||
await Future.delayed(const Duration(milliseconds: 200)); |
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.
What is the delayed
here for? (I'm wondering if there is a future we could explicitly wait on instead...)
registerMakeRequestExtension(testServer); | ||
} | ||
|
||
void registerMakeRequestExtension(io.HttpServer testServer) { |
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.
nit: method can be private
|
||
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
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.