Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1670283002:
add sendRequest to VMIsolateRef
Descriptionadd sendRequest to VMIsolateRef
BUG=2
Patch Set 1 #Patch Set 2 : rename to invokeExtension; enforce ext. prefix #
Total comments: 13
Patch Set 3 : more semantic ArgumentError; test params #
Total comments: 2
Patch Set 4 : onServiceExtensionAdded; Future<Object> #
Total comments: 6
Patch Set 5 : unwrap VMServiceExtension; add extensionRPCs #Patch Set 6 : onExtensionEvent, selectExtensionEvents, waitForExtension #Patch Set 7 : dartdocs #
Total comments: 27
Patch Set 8 : address review comments #Patch Set 9 : address comments 2 #
Messages
Total messages: 22 (2 generated)
Description was changed from ========== add sendRequest to VMIsolateRef BUG=2 ========== to ========== add sendRequest to VMIsolateRef BUG=2 ==========
yjbanov@google.com changed reviewers: + nweiz@google.com
I was thinking of something more specifically targeted to service extensions in particular. I'm not sure how they're invoked (I filed https://github.com/dart-lang/sdk/issues/25724 to document this), but even if they do share a namespace with the built-in RPCs, I'd like to name the method to invoke them something like `invokeExtension()`. I don't want to officially support people using this to access the VM service itself. I'd also love to see support for more of the extension API in the protocol. It seems strange to only support invoking methods without also supporting isolate metadata for extensions or extension events.
On 2016/02/08 23:21:27, nweiz wrote: > I was thinking of something more specifically targeted to service extensions in > particular. I'm not sure how they're invoked (I filed > https://github.com/dart-lang/sdk/issues/25724 to document this), but even if > they do share a namespace with the built-in RPCs, I'd like to name the method to > invoke them something like `invokeExtension()`. I don't want to officially > support people using this to access the VM service itself. +1 on `invokeExtension`. Just got a note from John saying that the enforcement of "ext_" prefix in custom extensions is imminent. I guess all we need to do is enforce it on the client side. > I'd also love to see support for more of the extension API in the protocol. It > seems strange to only support invoking methods without also supporting isolate > metadata for extensions I think at the level of VMIsolateRef we should aim at matching the "dart:developer" API rather than the protocol. Extensions cannot influence anything about the isolate metadata. It would be weird for it to participate in `invokeExtension` somehow. > or extension events. Agreed. This is orthogonal to method invocation though. Could be a separate CL?
On 2016/02/09 02:24:43, yjbanov wrote: > On 2016/02/08 23:21:27, nweiz wrote: > > I was thinking of something more specifically targeted to service extensions > in > > particular. I'm not sure how they're invoked (I filed > > https://github.com/dart-lang/sdk/issues/25724 to document this), but even if > > they do share a namespace with the built-in RPCs, I'd like to name the method > to > > invoke them something like `invokeExtension()`. I don't want to officially > > support people using this to access the VM service itself. > > +1 on `invokeExtension`. Just got a note from John saying that the enforcement > of "ext_" prefix in custom extensions is imminent. I guess all we need to do is > enforce it on the client side. > > > I'd also love to see support for more of the extension API in the protocol. It > > seems strange to only support invoking methods without also supporting isolate > > metadata for extensions > > I think at the level of VMIsolateRef we should aim at matching the > "dart:developer" API rather than the protocol. Extensions cannot influence > anything about the isolate metadata. It would be weird for it to participate in > `invokeExtension` somehow. I just meant the built-in Isolate.extensionRPCs field. > > or extension events. > > Agreed. This is orthogonal to method invocation though. Could be a separate CL? Sure.
> I'd like to name the method to invoke them something like `invokeExtension()`. Done. > I don't want to officially support people using this to access the VM service > itself. Done, by enforcing prefix "ext." > I just meant the built-in Isolate.extensionRPCs field. Ah, yes. This list can change dynamically so I'm not sure a snapshot would be very useful. A couple of options: 1. `waitUntilExtensionRegistered(String methodName)` 2. add the `Isolate.extensionRPCs` field, but make it an `ObservableList` Any thoughts?
https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:271: /// isolate and it must begin with prefix "ext.". It sort of feels like this should just automatically add "ext." rather than complaining that the user didn't do so manually. If you think that's too confusing, though, I could be convinced. https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:275: Future<Map> invokeExtension(String method, [Map<String, Object> params]) { Based on the ServiceExtensionHandler API, I think this signature should be Future<String> invokeExtension(String method, [Map<String, String> params]). https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:277: throw new ArgumentError('Extension method names must begin with "ext." prefix: ${method}'); Long line. Also, you can make this a little more semantic using ArgumentError.value: https://api.dartlang.org/1.14.1/dart-core/ArgumentError/ArgumentError.value.html https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart#... test/isolate_test.dart:395: expect(response, {'type': 'pong'}); Oh jeez, does the protocol automatically JSON-deserialize its return value? If so, that should definitely be documented in the registerExtension docs. https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart#... test/isolate_test.dart:396: }); Test passing in arguments as well.
> Ah, yes. This list can change dynamically so I'm not sure a snapshot would be > very useful. A couple of options: > > 1. `waitUntilExtensionRegistered(String methodName)` > 2. add the `Isolate.extensionRPCs` field, but make it an `ObservableList` > > Any thoughts? I don't want to add a dependency on observe—I don't want to require either mirrors or transformers. Providing a snapshot fits with how we handle other mutable fields like breakpoints, and once we have support for the extension-registration events we can add a helper function like you describe.
https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:271: /// isolate and it must begin with prefix "ext.". > It sort of feels like this should just automatically add "ext." rather than > complaining that the user didn't do so manually. If you think that's too > confusing, though, I could be convinced. I think there's value in having `invokeExtension` and `registerExtension` match each other. Also, I expect people will want to store method names as constants in shared libraries. They'll have to declare more constants if the names do not match. https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:275: Future<Map> invokeExtension(String method, [Map<String, Object> params]) { > Based on the ServiceExtensionHandler API, I think this signature should be > Future<String> invokeExtension(String method, [Map<String, String> params]). I thought so too initially, but turns out the JSON RPC front-end accepts non-strings, but encodes the values to strings prior to passing to the extension callback. Again, I'm all for matching signatures, as that's what users see and there's no loss of generality that I can think of. Your call. https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:277: throw new ArgumentError('Extension method names must begin with "ext." prefix: ${method}'); On 2016/02/10 21:28:28, nweiz wrote: > Long line. > > Also, you can make this a little more semantic using ArgumentError.value: > https://api.dartlang.org/1.14.1/dart-core/ArgumentError/ArgumentError.value.html Done. https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart#... test/isolate_test.dart:395: expect(response, {'type': 'pong'}); On 2016/02/10 21:28:28, nweiz wrote: > Oh jeez, does the protocol automatically JSON-deserialize its return value? If > so, that should definitely be documented in the registerExtension docs. Yep https://codereview.chromium.org/1670283002/diff/20001/test/isolate_test.dart#... test/isolate_test.dart:396: }); On 2016/02/10 21:28:28, nweiz wrote: > Test passing in arguments as well. Done.
> I don't want to add a dependency on observe—I don't want to require either > mirrors or transformers. Providing a snapshot fits with how we handle other > mutable fields like breakpoints, and once we have support for the > extension-registration events we can add a helper function like you describe. Actually, ServiceExtensionAdded is a built-in event delievered through on StreamManager.isolate, similar to IsolateStart and others. Should I add VMIsolateRef.onServiceExtensionAdded?
> Actually, ServiceExtensionAdded is a built-in event > delievered through on StreamManager.isolate, similar to > IsolateStart and others. Should I add > VMIsolateRef.onServiceExtensionAdded? Yeah, that would be awesome. https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:268: /// corresponding to the ID [number]. "the isolate ..." -> "this isolate." https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:271: /// isolate and it must begin with prefix "ext.". On 2016/02/10 22:06:06, yjbanov wrote: > > It sort of feels like this should just automatically add "ext." rather than > > complaining that the user didn't do so manually. If you think that's too > > confusing, though, I could be convinced. > > I think there's value in having `invokeExtension` and `registerExtension` match > each other. Also, I expect people will want to store method names as constants > in shared libraries. They'll have to declare more constants if the names do not > match. SGTM https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... lib/src/isolate.dart:275: Future<Map> invokeExtension(String method, [Map<String, Object> params]) { On 2016/02/10 22:06:06, yjbanov wrote: > > Based on the ServiceExtensionHandler API, I think this signature should be > > Future<String> invokeExtension(String method, [Map<String, String> params]). > > I thought so too initially, but turns out the JSON RPC front-end accepts > non-strings, but encodes the values to strings prior to passing to the extension > callback. Again, I'm all for matching signatures, as that's what users see and > there's no loss of generality that I can think of. > > Your call. I think we should only allow strings here. We definitely don't want people thinking they can transparently pass through map values and stuff like that. Given that the protocol effectively JSON-decodes the response, it seems like this should be a Future<Object> rather than a Future<Map>. https://codereview.chromium.org/1670283002/diff/40001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/40001/test/isolate_test.dart#... test/isolate_test.dart:410: Map response = await isolate.invokeExtension('ext.params', { Nit: "var response" (also above).
> > Should I add VMIsolateRef.onServiceExtensionAdded? > > Yeah, that would be awesome. Gave it a shot. Have a look. > https://codereview.chromium.org/1670283002/diff/20001/lib/src/isolate.dart#ne... > lib/src/isolate.dart:268: /// corresponding to the ID [number]. > "the isolate ..." -> "this isolate." Done. > I think we should only allow strings here. We definitely don't want people > thinking they can transparently pass through map values and stuff like that. Done. > Given that the protocol effectively JSON-decodes the response, it seems like > this should be a Future<Object> rather than a Future<Map>. Done. Also changed Scope. Also checked that VM service is actually capable of responding with non-maps. It happily returned an array.
https://codereview.chromium.org/1670283002/diff/40001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/40001/test/isolate_test.dart#... test/isolate_test.dart:410: Map response = await isolate.invokeExtension('ext.params', { On 2016/02/10 22:17:11, nweiz wrote: > Nit: "var response" (also above). Ugh, Flutter code style is getting into me. Done.
https://codereview.chromium.org/1670283002/diff/60001/lib/src/extension.dart File lib/src/extension.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/extension.dart#... lib/src/extension.dart:16: class VMServiceExtension { Since Isolate.extensionRPCs is a set of strings, I think we can reasonably assume that extensions won't have additional metadata added in the future. As such, I think VMIsolateRef.onServiceExtensionAdded can safely emit strings rather than custom objects. https://codereview.chromium.org/1670283002/diff/60001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/isolate.dart#ne... lib/src/isolate.dart:303: class VMIsolate extends VMIsolateRef { Could you add VMIsolate.extensionRpcs as well? https://codereview.chromium.org/1670283002/diff/60001/lib/src/scope.dart File lib/src/scope.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/scope.dart#newc... lib/src/scope.dart:49: Future<Object> sendRequest(String method, [Map<String, Object> params]) async { Nit: long line
https://codereview.chromium.org/1670283002/diff/60001/lib/src/extension.dart File lib/src/extension.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/extension.dart#... lib/src/extension.dart:16: class VMServiceExtension { On 2016/02/16 22:03:20, nweiz wrote: > Since Isolate.extensionRPCs is a set of strings, I think we can reasonably > assume that extensions won't have additional metadata added in the future. As > such, I think VMIsolateRef.onServiceExtensionAdded can safely emit strings > rather than custom objects. Done. https://codereview.chromium.org/1670283002/diff/60001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/isolate.dart#ne... lib/src/isolate.dart:303: class VMIsolate extends VMIsolateRef { On 2016/02/16 22:03:20, nweiz wrote: > Could you add VMIsolate.extensionRpcs as well? Done. https://codereview.chromium.org/1670283002/diff/60001/lib/src/scope.dart File lib/src/scope.dart (right): https://codereview.chromium.org/1670283002/diff/60001/lib/src/scope.dart#newc... lib/src/scope.dart:49: Future<Object> sendRequest(String method, [Map<String, Object> params]) async { On 2016/02/16 22:03:20, nweiz wrote: > Nit: long line Done.
https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:83: transform(_onExtensionEvent, new _VMExtensionEventSelector(kind, prefix)); I'd rather this just be an anonymous function. https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:361: final List<String> extensionRPCs; Nit: Following the Dart style guide, this should be "extensionRpcs" since the acronym is longer than two letters. https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:376: : const <String>[], // it's null when no extensions are registered Hmm, this is not what's documented. I've filed https://github.com/dart-lang/sdk/issues/25801 about it. Also, I think it's fine to just write `new UnmodifiableListView(json["extensionRPCs"] ?? [])` here. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:184: test("emits extension events", () async { Nit: this test and the selectExtensionEvents test should be in the group above. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:191: await isolate.resume(); There's no point to waiting until the isolate is paused and then immediately resuming if you don't register any handlers in the meantime. You should access the [onExtensionEvent] future (e.g. "var eventFuture = await isolate.onExtensionEvent.first") in between these two so that you're sure to subscribe to the stream before the isolate starts running. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:193: var event = await isolate.onExtensionEvent.first; I'd like to assert that this is the only event emitted by the stream. Maybe add something like the following to the test utils and use it here (and for selectExtensionEvents): Future onlyEvent(Stream stream) { var completer = new Completer.sync(); stream.listen(expectAsync(completer.complete, count: 1), onError: registerException, onDone: () { if (completer.isCompleted) return; throw "Expected an event."; }); return completer.future; } https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:208: await isolate.resume(); You definitely want a waitUntilPaused here, and you should only resume after you've listened to the streams. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:224: var isolate = await (await client.getVM()).isolates.first.load(); If we don't wait for some signal that [registerExtension] has been run, there's no guarantee that it's actually registered. Maybe add a print afterwards and await the stdout stream. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:225: isolate.waitForExtension('ext.test').then(expectAsync((value) { You can just write expect(isolate.waitForExtension('ext.test'), completes) for assertions like this. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:240: expect(value, isNull, I don't particularly care about asserting that undefined future values are null. One thing we should do here, though, is verify that we can successfully invoke ext.two. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:505: }); I forgot to suggest this earlier, but it would probably be good to verify that an error response is processed correctly.
PTAL https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:83: transform(_onExtensionEvent, new _VMExtensionEventSelector(kind, prefix)); On 2016/02/17 21:35:41, nweiz wrote: > I'd rather this just be an anonymous function. Done. https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:361: final List<String> extensionRPCs; On 2016/02/17 21:35:41, nweiz wrote: > Nit: Following the Dart style guide, this should be "extensionRpcs" since the > acronym is longer than two letters. Done. https://codereview.chromium.org/1670283002/diff/120001/lib/src/isolate.dart#n... lib/src/isolate.dart:376: : const <String>[], // it's null when no extensions are registered On 2016/02/17 21:35:41, nweiz wrote: > Hmm, this is not what's documented. I've filed > https://github.com/dart-lang/sdk/issues/25801 about it. > > Also, I think it's fine to just write `new > UnmodifiableListView(json["extensionRPCs"] ?? [])` here. Done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:184: test("emits extension events", () async { On 2016/02/17 21:35:41, nweiz wrote: > Nit: this test and the selectExtensionEvents test should be in the group above. Done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:191: await isolate.resume(); On 2016/02/17 21:35:41, nweiz wrote: > There's no point to waiting until the isolate is paused and then immediately > resuming if you don't register any handlers in the meantime. You should access > the [onExtensionEvent] future (e.g. "var eventFuture = await > isolate.onExtensionEvent.first") in between these two so that you're sure to > subscribe to the stream before the isolate starts running. Done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:193: var event = await isolate.onExtensionEvent.first; On 2016/02/17 21:35:41, nweiz wrote: > I'd like to assert that this is the only event emitted by the stream. Maybe add > something like the following to the test utils and use it here (and for > selectExtensionEvents): > > Future onlyEvent(Stream stream) { > var completer = new Completer.sync(); > stream.listen(expectAsync(completer.complete, count: 1), > onError: registerException, > onDone: () { > if (completer.isCompleted) return; > throw "Expected an event."; > }); > return completer.future; > } Done, but used a different method. onlyEvent does not work as it completes before the second event arrives, so it succeeds regardless. Stream.single has a similar issue. Instead, I posted a second event of a different kind as a signal that we will no longer be sending events of the first kind. This signal triggers the future to complete after we have aggregated all events posted prior to the last one. This assumes that the VM posts events in order. My testing seems to show that it does. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:208: await isolate.resume(); On 2016/02/17 21:35:41, nweiz wrote: > You definitely want a waitUntilPaused here Probably not necessary when pausing on start, but sure, can't hurt. Done. >, and you should only resume after > you've listened to the streams. Done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:224: var isolate = await (await client.getVM()).isolates.first.load(); On 2016/02/17 21:35:41, nweiz wrote: > If we don't wait for some signal that [registerExtension] has been run, there's > no guarantee that it's actually registered. Maybe add a print afterwards and > await the stdout stream. Seems like an very unlikely scenario, but done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:225: isolate.waitForExtension('ext.test').then(expectAsync((value) { On 2016/02/17 21:35:41, nweiz wrote: > You can just write expect(isolate.waitForExtension('ext.test'), completes) for > assertions like this. Acknowledged. In this case I'm also checking on the returned value. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:240: expect(value, isNull, On 2016/02/17 21:35:41, nweiz wrote: > I don't particularly care about asserting that undefined future values are null. I do, primarily because we're piping futures and streams from a 3rd-party package json_rpc_2 and it's worth making sure we're not leaking implementation details too much (to pass through errors is fine, but to pass values would be weird). But if you are against having this check I'll remove it. > One thing we should do here, though, is verify that we can successfully invoke > ext.two. Done https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:505: }); On 2016/02/17 21:35:41, nweiz wrote: > I forgot to suggest this earlier, but it would probably be good to verify that > an error response is processed correctly. Good point. Done.
https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:193: var event = await isolate.onExtensionEvent.first; On 2016/02/17 23:51:43, yjbanov wrote: > On 2016/02/17 21:35:41, nweiz wrote: > > I'd like to assert that this is the only event emitted by the stream. Maybe > add > > something like the following to the test utils and use it here (and for > > selectExtensionEvents): > > > > Future onlyEvent(Stream stream) { > > var completer = new Completer.sync(); > > stream.listen(expectAsync(completer.complete, count: 1), > > onError: registerException, > > onDone: () { > > if (completer.isCompleted) return; > > throw "Expected an event."; > > }); > > return completer.future; > > } > > Done, but used a different method. onlyEvent does not work as it completes > before the second event arrives, so it succeeds regardless. Stream.single has a > similar issue. Instead, I posted a second event of a different kind as a signal > that we will no longer be sending events of the first kind. This signal triggers > the future to complete after we have aggregated all events posted prior to the > last one. This assumes that the VM posts events in order. My testing seems to > show that it does. How does your approach apply to the streams in the selectExtensionEvents test? The whole point of those streams is to only filter for specific events, so posting a follow-up event doesn't seem like it would work. I also like how onlyEvent() is independent of the stream it's testing. I don't think we need to worry too much about giving it time to emit the event; we just want to ensure that if such an event *does* arrive before the connection closes, we recognize that as an error. If the timing concerns you, though, you could also add "expect(new Future.delayed(new Duration(milliseconds: 200)), completes)" to ensure that the test stays alive for a while. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:224: var isolate = await (await client.getVM()).isolates.first.load(); On 2016/02/17 23:51:43, yjbanov wrote: > On 2016/02/17 21:35:41, nweiz wrote: > > If we don't wait for some signal that [registerExtension] has been run, > there's > > no guarantee that it's actually registered. Maybe add a print afterwards and > > await the stdout stream. > > Seems like an very unlikely scenario, but done. One thing I've learned from writing a lot of asynchronous tests is that any given race conditions will probably happen eventually, and it's better to pre-empty them than to track them down after the fact. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:240: expect(value, isNull, On 2016/02/17 23:51:43, yjbanov wrote: > On 2016/02/17 21:35:41, nweiz wrote: > > I don't particularly care about asserting that undefined future values are > null. > > I do, primarily because we're piping futures and streams from a 3rd-party > package json_rpc_2 and it's worth making sure we're not leaking implementation > details too much (to pass through errors is fine, but to pass values would be > weird). But if you are against having this check I'll remove it. I am against it. Any void future could theoretically leak internal data, but a client that obeys the method's contract shouldn't ever access that data. It's not worth the effort to test the behavior that will only be accessed by clients violating the contract.
PTAL https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart File test/isolate_test.dart (right): https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:193: var event = await isolate.onExtensionEvent.first; > How does your approach apply to the streams in the selectExtensionEvents test? > The whole point of those streams is to only filter for specific events, so > posting a follow-up event doesn't seem like it would work. Switched to onlyEvent. See below. > If the timing concerns you, though, you could > also add "expect(new Future.delayed(new Duration(milliseconds: 200)), > completes)" to ensure that the test stays alive for a while. It wasn't a concern. It plain didn't work when I deliberately posted the same event twice. But adding "expect(new Future.delayed(new Duration(milliseconds: 200)), completes)" seems to have fixed it for this case. Can't vouch for it's robustness though. Done. https://codereview.chromium.org/1670283002/diff/120001/test/isolate_test.dart... test/isolate_test.dart:240: expect(value, isNull, > I am against it. Any void future could theoretically leak internal data Not if you test that it doesn't. > client that obeys the method's contract shouldn't ever access that data. It's > not worth the effort to test the behavior that will only be accessed by clients > violating the contract. There's no Future<void> in Dart, and Future == Future<dynamic>. The best you can do is Future<Null>. To say someone is violating a contract by using a value your API returns is nonsense. Additionally, internally we have to support all clients, whether they violate the contract or not. If you don't want a contract violated, enforce it, don't leak implementation details, reduce your API surface. Anyway, removed.
LGTM! I'll merge this in. Thanks for the patch, and for all your hard work iterating on it!
On 2016/02/18 01:09:39, nweiz wrote: > LGTM! I'll merge this in. > > Thanks for the patch, and for all your hard work iterating on it! \o/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
