restructure client api to use streams

pkg/analysis_server/test/channel_test.dart

Index: pkg/analysis_server/test/channel_test.dart
diff --git a/pkg/analysis_server/test/channel_test.dart b/pkg/analysis_server/test/channel_test.dart
index 0c639ad64a3259b0b3aa50e7120c05d5a776d96c..18e95ac73c6be9a170b0b804d2c3a65b496a5005 100644
--- a/pkg/analysis_server/test/channel_test.dart
+++ b/pkg/analysis_server/test/channel_test.dart
@@ -4,102 +4,144 @@
 
 library test.channel;
 
+import 'dart:async';
+
+import 'mocks.dart';

[Bob Nystrom, 2014/03/04 01:03:45]

Nit, but we tend to put local imports after "package:" ones.

[danrubel, 2014/03/05 15:32:04]

Done.

+
+import 'package:analysis_server/src/channel.dart';
 import 'package:analysis_server/src/protocol.dart';
 import 'package:unittest/matcher.dart';
 import 'package:unittest/unittest.dart';
-import 'package:analysis_server/src/channel.dart';
-import 'mocks.dart';
 
 main() {
   group('Channel', () {
+    setUp(ChannelTest.setUp);
     test('invalidJsonToClient', ChannelTest.invalidJsonToClient);
     test('invalidJsonToServer', ChannelTest.invalidJsonToServer);
     test('notification', ChannelTest.notification);
+    test('notificationAndResponse', ChannelTest.notificationAndResponse);
     test('request', ChannelTest.request);
+    test('requestResponse', ChannelTest.requestResponse);
     test('response', ChannelTest.response);
   });
 }
 
 class ChannelTest {

[Bob Nystrom, 2014/03/04 01:03:45]

Why make a class for this?

[danrubel, 2014/03/05 15:32:04]

It groups the tests. We will have more tests for this library but focused on
other classes.

[nweiz, 2014/03/05 20:16:39]

You're already using [group] for this. If you need coarser-grained grouping, I
find it's cleaner to use multiple test libraries.

[danrubel, 2014/03/11 19:06:09]

Good point.

+  static MockSocket socket;
+  static WebSocketClientChannel client;
+  static WebSocketServerChannel server;
 
-  static void invalidJsonToClient() {
-    InvalidJsonMockSocket mockSocket = new InvalidJsonMockSocket();
-    WebSocketClientChannel client = new WebSocketClientChannel(mockSocket);
-    var responsesReceived = new List();
-    var notificationsReceived = new List();
-    client.listen((Response response) => responsesReceived.add(response),
-        (Notification notification) => notificationsReceived.add(notification));
+  static List requestsReceived;
+  static List responsesReceived;
+  static List notificationsReceived;
 
-    mockSocket.addInvalid('"blat"');
-    mockSocket.addInvalid('{foo:bar}');
+  static void setUp() {
+    socket = new MockSocket.pair();
+    client = new WebSocketClientChannel(socket);
+    server = new WebSocketServerChannel(socket.twin);
 
-    expect(responsesReceived.length, equals(0));
-    expect(notificationsReceived.length, equals(0));
-    expect(mockSocket.responseCount, equals(0));
-  }
+    requestsReceived = new List();
+    responsesReceived = new List();
+    notificationsReceived = new List();

[Bob Nystrom, 2014/03/04 01:03:45]

new List() => []

[danrubel, 2014/03/05 15:32:04]

Done.

 
-  static void invalidJsonToServer() {
-    InvalidJsonMockSocket mockSocket = new InvalidJsonMockSocket();
-    WebSocketServerChannel server = new WebSocketServerChannel(mockSocket);
-    var received = new List();
-    server.listen((Request request) => received.add(request));
+    // Allow multiple listeners on server side for testing

[Bob Nystrom, 2014/03/04 01:03:45]

".".

[danrubel, 2014/03/05 15:32:04]

Done.

+    socket.twin.allowMultipleListeners();
 
-    mockSocket.addInvalid('"blat"');
-    mockSocket.addInvalid('{foo:bar}');
+    server.listen((data) => requestsReceived.add(data));

[Bob Nystrom, 2014/03/04 01:03:45]

server.listen(requestsReceived.add)

Ditto below. Yay for implicit closurization!

[danrubel, 2014/03/05 15:32:04]

Nice! Done.

+    client.responseStream.listen((data) => responsesReceived.add(data));
+    client.notificationStream.listen((data) => notificationsReceived.add(data));
+  }
 
-    expect(received.length, equals(0));
-    expect(mockSocket.responseCount, equals(2));
+  static Future invalidJsonToClient() {
+    socket.twin.add('{"foo":"bar"}');
+    server.sendResponse(new Response('myId'));
+    return client.responseStream
+        .first
+        .timeout(new Duration(seconds: 1))
+        .then((Response response) {
+          expect(response.id, equals('myId'));
+          expectMsgCount(0, 1, 0);
+        });

[Bob Nystrom, 2014/03/04 01:03:45]

This might do the right thing because responseStream is broadcast, but I've run
into problems using .first on a stream in tests. Nathan would know more here.

I would style it like:

  static Future invalidJsonToClient() {
    socket.twin.add('{"foo":"bar"}');
    server.sendResponse(new Response('myId'));
    return client.responseStream
        .first
        .timeout(new Duration(seconds: 1))
        .then((response) {
      expect(response.id, equals('myId'));
      expectMsgCount(0, 1, 0);
    });
  }

[danrubel, 2014/03/05 15:32:04]

The modified indentation makes it less obvious that the 2 statements are inside
the then block.

[nweiz, 2014/03/05 20:16:39]

With broadcast streams, it's important that you subscribe to them synchronously
as soon as they're created, since otherwise they can drop events on the floor.
In this case you do so, so you should be fine.

[danrubel, 2014/03/11 19:06:09]

Good point. https://codereview.chromium.org/195463004

   }
 
-  static void notification() {
-    MockSocket mockSocket = new MockSocket();
-    WebSocketClientChannel client = new WebSocketClientChannel(mockSocket);
-    WebSocketServerChannel server = new WebSocketServerChannel(mockSocket);
-    var responsesReceived = new List();
-    var notificationsReceived = new List();
-    client.listen((Response response) => responsesReceived.add(response),
-        (Notification notification) => notificationsReceived.add(notification));
+  static Future invalidJsonToServer() {
+    socket.add('"blat"');
+    return client.responseStream
+        .first
+        .timeout(new Duration(seconds: 1))
+        .then((Response response) {
+          expect(response.id, equals(''));
+          expect(response.error, isNotNull);
+          expectMsgCount(0, 1, 0);
+        });
+  }
 
+  static Future notification() {
     server.sendNotification(new Notification('myEvent'));
+    return client.notificationStream
+        .first
+        .timeout(new Duration(seconds: 1))
+        .then((Notification notification) {
+          expect(notification.event, equals('myEvent'));
+          expectMsgCount(0, 0, 1);
+
+          expect(notificationsReceived.first, equals(notification));
+        });
+  }
 
-    expect(responsesReceived.length, equals(0));
-    expect(notificationsReceived.length, equals(1));
-    expect(notificationsReceived.first.runtimeType, equals(Notification));
-    Notification actual = notificationsReceived.first;
-    expect(actual.event, equals('myEvent'));
+  static Future notificationAndResponse() {
+    server
+        ..sendNotification(new Notification('myEvent'))
+        ..sendResponse(new Response('myId'));
+    return Future
+        .wait([
+          client.notificationStream.first,
+          client.responseStream.first])
+        .timeout(new Duration(seconds: 1))
+        .then((_) {expectMsgCount(0, 1, 1);});

[Bob Nystrom, 2014/03/04 01:03:45]

Use => here or make it a multi-line method if you want to use {}.

[danrubel, 2014/03/05 15:32:04]

Good point. Done.

   }
 
   static void request() {
-    MockSocket mockSocket = new MockSocket();
-    WebSocketClientChannel client = new WebSocketClientChannel(mockSocket);
-    WebSocketServerChannel server = new WebSocketServerChannel(mockSocket);
-    var requestsReceived = new List();
-    server.listen((Request request) => requestsReceived.add(request));
-
-    client.sendRequest(new Request('myId', 'aMethod'));
-
-    expect(requestsReceived.length, equals(1));
-    expect(requestsReceived.first.runtimeType, equals(Request));
-    Request actual = requestsReceived.first;
-    expect(actual.id, equals('myId'));
-    expect(actual.method, equals('aMethod'));
+    client.sendRequest(new Request('myId', 'myMth'));
+    server.listen((Request request) {
+      expect(request.id, equals('myId'));
+      expect(request.method, equals('myMth'));
+      expectMsgCount(1, 0, 0);
+    });
   }
 
-  static void response() {
-    MockSocket mockSocket = new MockSocket();
-    WebSocketClientChannel client = new WebSocketClientChannel(mockSocket);
-    WebSocketServerChannel server = new WebSocketServerChannel(mockSocket);
-    var responsesReceived = new List();
-    var notificationsReceived = new List();
-    client.listen((Response response) => responsesReceived.add(response),
-        (Notification notification) => notificationsReceived.add(notification));
+  static Future requestResponse() {
+    // Simulate server sending a response by echoing the request
+    server.listen((Request request) =>
+        server.sendResponse(new Response(request.id)));
+    return client.sendRequest(new Request('myId', 'myMth'))
+        .timeout(new Duration(seconds: 1))
+        .then((Response response) {
+          expect(response.id, equals('myId'));
+          expectMsgCount(1, 1, 0);
+
+          expect(requestsReceived.first.runtimeType, equals(Request));

[Bob Nystrom, 2014/03/04 01:03:45]

Do:

requestsReceived.first, new InstanceOf<Request>()

or just:

requestsReceived.first is Request, isTrue

[danrubel, 2014/03/05 15:32:04]

Good point. Done.

Sure would be nice if the matcher defaulted to isTrue so that you could write 

expect(requestsReceived.first is Request)

[Bob Nystrom, 2014/03/05 21:01:03]

It used to, but expect has optional named parameters and Dart doesn't let you
mix optional positional and option named parameters. :(

+          Request request = requestsReceived.first;
+          expect(request.id, equals('myId'));
+          expect(request.method, equals('myMth'));
+          expect(responsesReceived.first, equals(response));
+        });
+  }
 
+  static Future response() {
     server.sendResponse(new Response('myId'));
+    return client.responseStream
+        .first
+        .timeout(new Duration(seconds: 1))
+        .then((Response response) {
+          expect(response.id, equals('myId'));
+          expectMsgCount(0, 1, 0);
+        });
+  }
 
-    expect(responsesReceived.length, equals(1));
-    expect(notificationsReceived.length, equals(0));
-    expect(responsesReceived.first.runtimeType, equals(Response));
-    Response actual = responsesReceived.first;
-    expect(actual.id, equals('myId'));
+  static void expectMsgCount(requestCount, responseCount, notificationCount) {

[Bob Nystrom, 2014/03/04 01:03:45]

I think the calls to this would be easier to read if you made these named
parameters.

[danrubel, 2014/03/05 15:32:04]

I like it. Done.

+    expect(requestsReceived, hasLength(requestCount));
+    expect(responsesReceived, hasLength(responseCount));
+    expect(notificationsReceived, hasLength(notificationCount));
   }
 }