Make API logic testable, and start testing some

lib/api/core.dart

@@ -18,11 +18,29 @@ class RawParameter {
   final String value;
 }
 
-class ApiConnection {
+/// All the information to talk to a Zulip server, real or fake.
+///
+/// See also:
+///  * [LiveApiConnection], which implements this for talking to a
+///    real Zulip server.
+///  * `FakeApiConnection` in the test suite, which implements this
+///    for use in tests.
+abstract class ApiConnection {
   ApiConnection({required this.auth});
 
+  // TODO move auth field to subclass, have just a realmUrl getter;
+  //   that ensures nothing assumes base class has a real API key
   final Auth auth;
 
+  Future<String> get(String route, Map<String, dynamic>? params);
+
+  Future<String> post(String route, Map<String, dynamic>? params);
+}
+
+/// An [ApiConnection] that makes real network requests to a real server.
+class LiveApiConnection extends ApiConnection {
+  LiveApiConnection({required super.auth});
+
   Map<String, String> _headers() {
     // TODO memoize
     final authBytes = utf8.encode("${auth.email}:${auth.apiKey}");
@@ -31,6 +49,7 @@ class ApiConnection {
     };
   }
 
+  @override
   Future<String> get(String route, Map<String, dynamic>? params) async {
     final baseUrl = Uri.parse(auth.realmUrl);
     final url = Uri(
@@ -48,6 +67,7 @@ class ApiConnection {
     return utf8.decode(response.bodyBytes);
   }
 
+  @override
   Future<String> post(String route, Map<String, dynamic>? params) async {
     final response = await http.post(
         Uri.parse("${auth.realmUrl}/api/v1/$route"),

lib/api/model/model.dart

@@ -127,6 +127,7 @@ abstract class Message {
   final String subject; // TODO call it "topic" internally; also similar others
   // final List<string> submessages; // TODO handle
   final int timestamp;
+  String get type;
 
   // final List<TopicLink> topic_links; // TODO handle
   // final string type; // handled by runtime type of object
@@ -166,6 +167,10 @@ abstract class Message {
 
 @JsonSerializable()
 class StreamMessage extends Message {
+  @override
+  @JsonKey(includeToJson: true)
+  String get type => 'stream';
+
   final String display_recipient;
   final int stream_id;
 
@@ -217,6 +222,10 @@ class PmRecipient {
 
 @JsonSerializable()
 class PmMessage extends Message {
+  @override
+  @JsonKey(includeToJson: true)
+  String get type => 'private';
+
   final List<PmRecipient> display_recipient;
 
   PmMessage({

lib/model/store.dart

@@ -71,7 +71,7 @@ class PerAccountStore extends ChangeNotifier {
   ///
   /// In the future this might load an old snapshot from local storage first.
   static Future<PerAccountStore> load(Account account) async {
-    final connection = ApiConnection(auth: account);
+    final connection = LiveApiConnection(auth: account);
 
     final stopwatch = Stopwatch()..start();
     final initialSnapshot = await registerQueue(connection); // TODO retry

test/api/fake_api.dart

@@ -0,0 +1,35 @@
+import 'package:zulip/api/core.dart';
+import 'package:zulip/model/store.dart';
+
+/// An [ApiConnection] that accepts and replays canned responses, for testing.
+class FakeApiConnection extends ApiConnection {
+  FakeApiConnection({required String realmUrl, required String email})
+      : super(auth: Account(
+                realmUrl: realmUrl, email: email, apiKey: _fakeApiKey));
+
+  String? _nextResponse;
+
+  // TODO: This mocking API will need to get richer to support all the tests we need.
+
+  void prepare(String response) {
+    assert(_nextResponse == null,
+        'FakeApiConnection.prepare was called while already expecting a request');
+    _nextResponse = response;
+  }
+
+  @override
+  Future<String> get(String route, Map<String, dynamic>? params) async {
+    final response = _nextResponse;
+    _nextResponse = null;
+    return response!;
+  }
+
+  @override
+  Future<String> post(String route, Map<String, dynamic>? params) async {
+    final response = _nextResponse;
+    _nextResponse = null;
+    return response!;
+  }
+}
+
+const String _fakeApiKey = 'fake-api-key';

test/api/model/events_checks.dart

@@ -0,0 +1,29 @@
+// ignore_for_file: non_constant_identifier_names
+
+import 'package:checks/checks.dart';
+import 'package:zulip/api/model/events.dart';
+import 'package:zulip/api/model/model.dart';
+
+extension EventChecks on Subject<Event> {
+  Subject<int> get id => has((e) => e.id, 'id');
+  Subject<String> get type => has((e) => e.type, 'type');
+}
+
+extension UnexpectedEventChecks on Subject<UnexpectedEvent> {
+  Subject<Map<String, dynamic>> get json => has((e) => e.json, 'json');
+}
+
+extension AlertWordsEventChecks on Subject<AlertWordsEvent> {
+  Subject<List<String>> get alert_words => has((e) => e.alert_words, 'alert_words');
+}
+
+extension MessageEventChecks on Subject<MessageEvent> {
+  Subject<Message> get message => has((e) => e.message, 'message');
+}
+
+extension HeartbeatEventChecks on Subject<HeartbeatEvent> {
+  // No properties not covered by Event.
+}
+
+// Add more extensions here for more event types as needed.
+// Keep them in the same order as the event types' own definitions.

test/api/model/events_test.dart

@@ -0,0 +1,22 @@
+import 'package:checks/checks.dart';
+import 'package:test/scaffolding.dart';
+import 'package:zulip/api/model/events.dart';
+
+import '../../example_data.dart' as eg;
+import 'events_checks.dart';
+import 'model_checks.dart';
+
+void main() {
+  test('message: move flags into message object', () {
+    final message = eg.streamMessage();
+    MessageEvent mkEvent(List<String> flags) => Event.fromJson({
+        'type': 'message',
+        'id': 1,
+        'message': message.toJson()..remove('flags'),
+        'flags': flags,
+      }) as MessageEvent;
+    check(mkEvent(message.flags)).message.jsonEquals(message);
+    check(mkEvent([])).message.flags.deepEquals([]);
+    check(mkEvent(['read'])).message.flags.deepEquals(['read']);
+  });
+}

test/api/model/model_checks.dart

@@ -0,0 +1,16 @@
+import 'package:checks/checks.dart';
+import 'package:zulip/api/model/model.dart';
+
+extension MessageChecks on Subject<Message> {
+  Subject<Map<String, dynamic>> get toJson => has((e) => e.toJson(), 'toJson');
+
+  void jsonEquals(Message expected) {
+    toJson.deepEquals(expected.toJson());
+  }
+
+  Subject<List<String>> get flags => has((e) => e.flags, 'flags');
+
+  // TODO accessors for other fields
+}
+
+// TODO similar extensions for other types in model

test/api/route/messages_test.dart

@@ -0,0 +1,26 @@
+import 'dart:convert';
+
+import 'package:checks/checks.dart';
+import 'package:test/scaffolding.dart';
+import 'package:zulip/api/route/messages.dart';
+
+import '../fake_api.dart';
+import 'route_checks.dart';
+
+void main() {
+  test('sendMessage accepts fixture realm', () async {
+    final connection = FakeApiConnection(
+        realmUrl: 'https://chat.zulip.org/', email: '[email protected]');
+    connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson()));
+    check(sendMessage(connection, content: 'hello', topic: 'world'))
+        .completes(it()..id.equals(42));
+  });
+
+  test('sendMessage rejects unexpected realm', () async {
+    final connection = FakeApiConnection(
+        realmUrl: 'https://chat.example/', email: '[email protected]');
+    connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson()));
+    check(sendMessage(connection, content: 'hello', topic: 'world'))
+        .throws();
+  });
+}

test/api/route/route_checks.dart

@@ -0,0 +1,11 @@
+// ignore_for_file: non_constant_identifier_names
+
+import 'package:checks/checks.dart';
+import 'package:zulip/api/route/messages.dart';
+
+extension SendMessageResultChecks on Subject<SendMessageResult> {
+  Subject<int> get id => has((e) => e.id, 'id');
+  Subject<String?> get deliver_at => has((e) => e.deliver_at, 'deliver_at');
+}
+
+// TODO add similar extensions for other classes in api/route/*.dart

test/example_data.dart

@@ -0,0 +1,45 @@
+import 'package:zulip/api/model/model.dart';
+
+final _messagePropertiesBase = {
+  'is_me_message': false,
+  'last_edit_timestamp': null,
+  'recipient_id': 32, // obsolescent in API, and ignored
+};
+
+// When we have a User object, this can take that as an argument.
+Map<String, dynamic> _messagePropertiesFromSender() {
+  return {
+    'client': 'ExampleClient',
+    'sender_email': 'a-person@example',
+    'sender_full_name': 'A Person',
+    'sender_id': 12345, // TODO generate example IDs
+    'sender_realm_str': 'zulip',
+  };
+}
+
+// When we have a Stream object, this can take that as an argument.
+// Also it can default explicitly to an example stream.
+StreamMessage streamMessage(
+    {String? streamName, int? streamId}) {
+  // The use of JSON here is convenient in order to delegate parts of the data
+  // to helper functions.  The main downside is that it loses static typing
+  // of the properties as we're constructing the data.  That's probably OK
+  // because (a) this is only for tests; (b) the types do get checked
+  // dynamically in the constructor, so any ill-typing won't propagate further.
+  return StreamMessage.fromJson({
+    ..._messagePropertiesBase,
+    ..._messagePropertiesFromSender(),
+    'display_recipient': streamName ?? 'a stream',
+    'stream_id': streamId ?? 123, // TODO generate example IDs
+
+    'content': '<p>This is an example stream message.</p>',
+    'content_type': 'text/html',
+    'flags': [],
+    'id': 1234567, // TODO generate example IDs
+    'subject': 'example topic',
+    'timestamp': 1678139636,
+    'type': 'stream',
+  });
+}
+
+// TODO example data for many more types