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
Migrate devicelab to package:vm_service #71882
Conversation
@@ -69,7 +69,7 @@ class _TaskRunner { | |||
}); | |||
registerExtension('ext.cocoonRunnerReady', | |||
(String method, Map<String, String> parameters) async { | |||
return ServiceExtensionResponse.result('"ready"'); | |||
return ServiceExtensionResponse.result('{"result":"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.
The docs on this say that it has to be a valid "JSON object", which I assume means a {}
. package:vm_service
actually expects this to correspon to a Map
object in Dart, whereas vm_service_client
tolerated a String. A quoted string is valid JSON, but I guess not a valid JSON object.
/cc @bkonyi FYI.
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.
lgtm with one comment.
@@ -92,32 +94,54 @@ Future<TaskResult> runTask( | |||
} | |||
} | |||
|
|||
Future<VMIsolateRef> _connectToRunnerIsolate(Uri vmServiceUri) async { | |||
class _ClientAndIsolate { |
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.
I don't love the name, but having a rough time coming up with something better. Maybe just Runner
or RunnerClient
? The client/isolate are sort of incidental just for the mechanics of the call, but conceptually this is a mechanism for firing off calls to the runner's service extension interface.
I sort of wonder if the naming issue is more of an abstraction issue -- this feels like a bit of an in-between abstraction like Pair. You could go all the way and name it Runner
then rather than exposing callServiceExtention
, expose the actual methods you care about runnerReady
and runTask
, basically a sort of cocoon service interface. Given that this is private API, up to you if you want to go that far in this patch, which is more about migration, but it's definitely worth adding a small doc comment for future visitors to this code.
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.
Changing it to RunnerClient
and doing a quick refactor here.
This reverts commit 9d1d0cf.
Description
Drop last references to package:vm_service_client from devicelab usage.
Related Issues
Fixes #37499
Tests
Update relevant tests. Will have to run through devicelab to be sure that this works, but it works locally on some spot checks.