add sendRequest to VMIsolateRef

test/isolate_test.dart

Index: test/isolate_test.dart
diff --git a/test/isolate_test.dart b/test/isolate_test.dart
index b08bdd9158db4d4326739dbc8fc1e97da022cfe2..5f1d062b2c55e2f02897e5171b04a3a41af7145b 100644
--- a/test/isolate_test.dart
+++ b/test/isolate_test.dart
@@ -165,6 +165,97 @@ void main() {
         expect(other.onExit, completes);
       });
     });
+
+    test("onServiceExtensionAdded fires when an extension is added", () async {
+      client = await runAndConnect(main: """
+        registerExtension('ext.test', (_, __) {});
+      """, flags: ["--pause-isolates-on-start"]);
+
+      var isolate = await (await client.getVM()).isolates.first.loadRunnable();
+      await isolate.waitUntilPaused();
+      await isolate.resume();
+
+      String ext = await isolate.onServiceExtensionAdded.first;
+      expect(ext, 'ext.test');
+    });
+  });
+
+  group("onExtensionEvent", () {
+    test("emits extension events", () async {

[nweiz, 2016/02/17 21:35:41]

Nit: this test and the selectExtensionEvents test should be in the group above.

[yjbanov, 2016/02/17 23:51:43]

Done.

+      client = await runAndConnect(main: """
+        postEvent('foo', {'bar': 'baz'});
+      """, flags: ["--pause-isolates-on-start"]);
+
+      var isolate = await (await client.getVM()).isolates.first.load();
+      await isolate.waitUntilPaused();
+      await isolate.resume();

[nweiz, 2016/02/17 21:35:41]

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.

[yjbanov, 2016/02/17 23:51:43]

Done.

+
+      var event = await isolate.onExtensionEvent.first;

[nweiz, 2016/02/17 21:35:41]

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;
}

[yjbanov, 2016/02/17 23:51:43]

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.

[nweiz, 2016/02/18 00:24:06]

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.

[yjbanov, 2016/02/18 01:00:22]

Switched to onlyEvent. See below.
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.

+      expect(event.kind, 'foo');
+      expect(event.data, {'bar': 'baz'});
+    });
+  });
+
+  group("selectExtensionEvents", () {
+    test("chooses by extension kind", () async {
+      client = await runAndConnect(main: """
+        postEvent('foo', {'prefixed': false});
+        postEvent('bar.baz', {'prefixed': true});
+        postEvent('not.captured', {});
+      """, flags: ["--pause-isolates-on-start"]);
+
+      var isolate = await (await client.getVM()).isolates.first.load();
+      await isolate.resume();

[nweiz, 2016/02/17 21:35:41]

You definitely want a waitUntilPaused here, and you should only resume after
you've listened to the streams.

[yjbanov, 2016/02/17 23:51:44]

Probably not necessary when pausing on start, but sure, can't hurt. Done.
Done.

+
+      var unprefixedEvent = isolate.selectExtensionEvents('foo').first;
+      var prefixedEvent =
+          isolate.selectExtensionEvents('bar.', prefix: true).first;
+      expect((await unprefixedEvent).kind, 'foo');
+      expect((await prefixedEvent).kind, 'bar.baz');
+    });
+  });
+
+  group("waitForExtension", () {
+    test("notifies when the extension is already registered", () async {
+      client = await runAndConnect(main: """
+        registerExtension('ext.test', (_, __) {});
+      """);
+
+      var isolate = await (await client.getVM()).isolates.first.load();

[nweiz, 2016/02/17 21:35:41]

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.

[yjbanov, 2016/02/17 23:51:43]

Seems like an very unlikely scenario, but done.

[nweiz, 2016/02/18 00:24:06]

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.

+      isolate.waitForExtension('ext.test').then(expectAsync((value) {

[nweiz, 2016/02/17 21:35:41]

You can just write expect(isolate.waitForExtension('ext.test'), completes) for
assertions like this.

[yjbanov, 2016/02/17 23:51:44]

Acknowledged. In this case I'm also checking on the returned value.

+        expect(value, isNull,
+          reason: 'we are not expecting a value, just Future completion');
+      }));
+    });
+
+    test("notifies when the extension is registered later", () async {
+      client = await runAndConnect(main: """
+        registerExtension('ext.one', (_, __) {
+          registerExtension('ext.two', (_, __) {});
+        });
+      """);
+
+      var isolate = await (await client.getVM()).isolates.first.load();
+      isolate.waitForExtension('ext.two').then(expectAsync((value) {
+        expect(value, isNull,

[nweiz, 2016/02/17 21:35:41]

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.

[yjbanov, 2016/02/17 23:51:43]

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.
Done

[nweiz, 2016/02/18 00:24:06]

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.

[yjbanov, 2016/02/18 01:00:22]

Not if you test that it doesn't.
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.

+          reason: 'we are not expecting a value, just Future completion');
+      }));
+
+      await isolate.waitForExtension('ext.one');
+      isolate.invokeExtension('ext.one');
+    });
+  });
+
+  group("load", () {
+    test("loads extensionRPCs", () async {
+      client = await runAndConnect(main: """
+        registerExtension('ext.foo', (_, __) {});
+        registerExtension('ext.bar', (_, __) {});
+      """);
+
+      var isolate = await (await client.getVM()).isolates.first.load();
+      expect(isolate.extensionRPCs, unorderedEquals(['ext.foo', 'ext.bar']));
+    });
   });
 
   group("loadRunnable", () {
@@ -375,6 +466,44 @@ void main() {
       expect(breakpoint.number, equals(1));
     });
   });
+
+  group("invokeExtension", () {
+    test("enforces ext. prefix", () async {
+      var client = await runAndConnect();
+      var isolate = await (await client.getVM()).isolates.first.loadRunnable();
+      expect(() => isolate.invokeExtension('noprefix'), throwsArgumentError);
+    });
+
+    test("forwards to scope", () async {
+      var client = await runAndConnect(main: r"""
+        registerExtension('ext.ping', (_, __) async {
+          return new ServiceExtensionResponse.result('{"type": "pong"}');
+        });
+      """);
+
+      var isolate = await (await client.getVM()).isolates.first.loadRunnable();
+      var response = await isolate.invokeExtension('ext.ping');
+      expect(response, {'type': 'pong'});
+    });
+
+    test("passes parameters", () async {
+      var client = await runAndConnect(main: r"""
+        registerExtension('ext.params', (_, params) async {
+          return new ServiceExtensionResponse.result('''{
+            "foo": "${params['foo']}"
+          }''');
+        });
+      """);
+
+      var isolate = await (await client.getVM()).isolates.first.loadRunnable();
+      var response = await isolate.invokeExtension('ext.params', {
+        'foo': 'bar',
+      });
+      expect(response, {
+        'foo': 'bar',
+      });
+    });

[nweiz, 2016/02/17 21:35:41]

I forgot to suggest this earlier, but it would probably be good to verify that
an error response is processed correctly.

[yjbanov, 2016/02/17 23:51:43]

Good point. Done.

+  });
 }
 
 /// Starts a client with two unpaused empty isolates.