From 615eac8c6847cebf51e7d6ba12849683f5969f09 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 13:01:27 -0700 Subject: [PATCH 01/11] test [nfc]: Add "checks" extension for Uri This includes a "TODO(checks)" comment, indicating a question that should go upstream for the `package:checks` beta. --- test/stdlib_checks.dart | 26 ++++++++++++++++++++++++++ test/widgets/login_test.dart | 6 +++--- 2 files changed, 29 insertions(+), 3 deletions(-) create mode 100644 test/stdlib_checks.dart diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart new file mode 100644 index 0000000000..68d6370eb8 --- /dev/null +++ b/test/stdlib_checks.dart @@ -0,0 +1,26 @@ +/// `package:checks`-related extensions for the Dart standard library. +/// +/// Use this file for types in the Dart SDK, as well as in other +/// packages published by the Dart team that function as +/// part of the Dart standard library. + +import 'package:checks/checks.dart'; + +extension UriChecks on Subject { + Subject get asString => has((u) => u.toString(), 'toString'); // TODO(checks): what's a good convention for this? + + Subject get scheme => has((u) => u.scheme, 'scheme'); + Subject get authority => has((u) => u.authority, 'authority'); + Subject get userInfo => has((u) => u.userInfo, 'userInfo'); + Subject get host => has((u) => u.host, 'host'); + Subject get port => has((u) => u.port, 'port'); + Subject get path => has((u) => u.path, 'path'); + Subject get query => has((u) => u.query, 'query'); + Subject get fragment => has((u) => u.fragment, 'fragment'); + Subject> get pathSegments => has((u) => u.pathSegments, 'pathSegments'); + Subject> get queryParameters => has((u) => u.queryParameters, 'queryParameters'); + Subject>> get queryParametersAll => has((u) => u.queryParametersAll, 'queryParametersAll'); + Subject get isAbsolute => has((u) => u.isAbsolute, 'isAbsolute'); + Subject get origin => has((u) => u.origin, 'origin'); + // TODO hasScheme, other has*, data +} diff --git a/test/widgets/login_test.dart b/test/widgets/login_test.dart index c12a213a84..081dbc6c5b 100644 --- a/test/widgets/login_test.dart +++ b/test/widgets/login_test.dart @@ -2,6 +2,8 @@ import 'package:checks/checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/login.dart'; +import '../stdlib_checks.dart'; + void main() { group('ServerUrlTextEditingController.tryParse', () { final controller = ServerUrlTextEditingController(); @@ -11,9 +13,7 @@ void main() { controller.text = text; final result = controller.tryParse(); check(result.error).isNull(); - check(result.url) - .isNotNull() // catch `null` here instead of by its .toString() - .has((url) => url.toString(), 'toString()').equals(expectedUrl); + check(result.url).isNotNull().asString.equals(expectedUrl); }); } From 4e0279447ef225c3b42e423b2990fd9c75bc0841 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 8 May 2023 20:36:11 -0700 Subject: [PATCH 02/11] fake_api [nfc]: Make realmUrl optional on FakeApiConnection constructor --- test/api/fake_api.dart | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 38d810fe30..0bb50271e4 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -4,6 +4,8 @@ import 'package:http/http.dart' as http; import 'package:zulip/api/core.dart'; import 'package:zulip/model/store.dart'; +import '../example_data.dart' as eg; + /// An [http.Client] that accepts and replays canned responses, for testing. class FakeHttpClient extends http.BaseClient { @@ -28,14 +30,14 @@ class FakeHttpClient extends http.BaseClient { /// An [ApiConnection] that accepts and replays canned responses, for testing. class FakeApiConnection extends ApiConnection { - FakeApiConnection({required Uri realmUrl}) - : this._(realmUrl: realmUrl, client: FakeHttpClient()); + FakeApiConnection({Uri? realmUrl}) + : this._(realmUrl: realmUrl ?? eg.realmUrl, client: FakeHttpClient()); FakeApiConnection.fromAccount(Account account) : this(realmUrl: account.realmUrl); - FakeApiConnection._({required Uri realmUrl, required this.client}) - : super(client: client, realmUrl: realmUrl); + FakeApiConnection._({required super.realmUrl, required this.client}) + : super(client: client); final FakeHttpClient client; From af53861699fe9bdf90b1fb06634069fd28fd0f5d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 9 May 2023 20:49:37 -0700 Subject: [PATCH 03/11] fake_api [nfc]: Take body as named parameter on prepare This will make things a bit cleaner later, when we accept more parameters. --- test/api/fake_api.dart | 10 +++++----- test/api/route/messages_test.dart | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 0bb50271e4..e93e4d8755 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -13,10 +13,10 @@ class FakeHttpClient extends http.BaseClient { // TODO: This mocking API will need to get richer to support all the tests we need. - void prepare(String response) { + void prepare({String? body}) { assert(_nextResponseBytes == null, - 'FakeApiConnection.prepare was called while already expecting a request'); - _nextResponseBytes = utf8.encode(response); + 'FakeApiConnection.prepare was called while already expecting a request'); + _nextResponseBytes = utf8.encode(body ?? ''); } @override @@ -41,7 +41,7 @@ class FakeApiConnection extends ApiConnection { final FakeHttpClient client; - void prepare(String response) { - client.prepare(response); + void prepare({String? body}) { + client.prepare(body: body); } } diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index f15fda887e..48db2382c9 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -11,7 +11,7 @@ void main() { test('sendMessage accepts fixture realm', () async { final connection = FakeApiConnection( realmUrl: Uri.parse('https://chat.zulip.org/')); - connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson())); + connection.prepare(body: jsonEncode(SendMessageResult(id: 42).toJson())); check(sendMessage(connection, content: 'hello', topic: 'world')) .completes(it()..id.equals(42)); }); @@ -19,7 +19,7 @@ void main() { test('sendMessage rejects unexpected realm', () async { final connection = FakeApiConnection( realmUrl: Uri.parse('https://chat.example/')); - connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson())); + connection.prepare(body: jsonEncode(SendMessageResult(id: 42).toJson())); check(sendMessage(connection, content: 'hello', topic: 'world')) .throws(); }); From 852df58b9f6b631120fb2aa8cddb41f85952d6dc Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 13:01:44 -0700 Subject: [PATCH 04/11] fake_api: Log last request made Also unpack a TODO comment into a softer invitation to expand the API, and a list of suggestions. (This wasn't really an actionable TODO in the first place; its real function was always more to just disclaim any belief that the API was sufficient, as an invitation to go ahead and expand it when needed.) --- test/api/fake_api.dart | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index e93e4d8755..837ccad1b8 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -9,9 +9,14 @@ import '../example_data.dart' as eg; /// An [http.Client] that accepts and replays canned responses, for testing. class FakeHttpClient extends http.BaseClient { + http.BaseRequest? lastRequest; + List? _nextResponseBytes; - // TODO: This mocking API will need to get richer to support all the tests we need. + // Please add more features to this mocking API as needed. For example: + // * preparing an HTTP status other than 200 + // * preparing an exception instead of an [http.StreamedResponse] + // * preparing more than one request, and logging more than one request void prepare({String? body}) { assert(_nextResponseBytes == null, @@ -23,6 +28,7 @@ class FakeHttpClient extends http.BaseClient { Future send(http.BaseRequest request) { final responseBytes = _nextResponseBytes!; _nextResponseBytes = null; + lastRequest = request; final byteStream = http.ByteStream.fromBytes(responseBytes); return Future.value(http.StreamedResponse(byteStream, 200, request: request)); } @@ -41,6 +47,8 @@ class FakeApiConnection extends ApiConnection { final FakeHttpClient client; + http.BaseRequest? get lastRequest => client.lastRequest; + void prepare({String? body}) { client.prepare(body: body); } From a5f7d66038c180df38eceb5471ea622930a83c31 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 13:14:04 -0700 Subject: [PATCH 05/11] fake_api: Use auth in FakeApiConnection.fromAccount constructor Seems most logical, given that the account contains the auth. And this will enable us to test that the auth information gets appropriately included in the HTTP request. (The API key should of course be a fake one; the tests shouldn't have any other kind of API key in the first place. And the fake connection won't generate any real network requests in any case, because it uses a FakeHttpClient.) --- test/api/fake_api.dart | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 837ccad1b8..70be4e3f1c 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -40,10 +40,18 @@ class FakeApiConnection extends ApiConnection { : this._(realmUrl: realmUrl ?? eg.realmUrl, client: FakeHttpClient()); FakeApiConnection.fromAccount(Account account) - : this(realmUrl: account.realmUrl); - - FakeApiConnection._({required super.realmUrl, required this.client}) - : super(client: client); + : this._( + realmUrl: account.realmUrl, + email: account.email, + apiKey: account.apiKey, + client: FakeHttpClient()); + + FakeApiConnection._({ + required super.realmUrl, + super.email, + super.apiKey, + required this.client, + }) : super(client: client); final FakeHttpClient client; From 300b230ac665e63be458660662e81a0e754da56e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 12:14:53 -0700 Subject: [PATCH 06/11] fake_api [nfc]: Add FakeApiConnection.with_ helper --- test/api/fake_api.dart | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 70be4e3f1c..d71185d188 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -55,6 +55,24 @@ class FakeApiConnection extends ApiConnection { final FakeHttpClient client; + /// Run the given callback on a fresh [FakeApiConnection], then close it, + /// using try/finally. + static Future with_( + Future Function(FakeApiConnection connection) fn, { + Uri? realmUrl, + Account? account, + }) async { + assert((account == null) || (realmUrl == null)); + final connection = (account != null) + ? FakeApiConnection.fromAccount(account) + : FakeApiConnection(realmUrl: realmUrl); + try { + return fn(connection); + } finally { + connection.close(); + } + } + http.BaseRequest? get lastRequest => client.lastRequest; void prepare({String? body}) { From f0defb6da611a0c1ada9d2d666fbcae506a1cec5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 13:01:27 -0700 Subject: [PATCH 07/11] test: Add "checks" extensions for http.Request --- test/stdlib_checks.dart | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart index 68d6370eb8..95756bd70e 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -4,7 +4,10 @@ /// packages published by the Dart team that function as /// part of the Dart standard library. +import 'dart:convert'; + import 'package:checks/checks.dart'; +import 'package:http/http.dart' as http; extension UriChecks on Subject { Subject get asString => has((u) => u.toString(), 'toString'); // TODO(checks): what's a good convention for this? @@ -24,3 +27,19 @@ extension UriChecks on Subject { Subject get origin => has((u) => u.origin, 'origin'); // TODO hasScheme, other has*, data } + +extension HttpBaseRequestChecks on Subject { + Subject get method => has((r) => r.method, 'method'); + Subject get url => has((r) => r.url, 'url'); + Subject get contentLength => has((r) => r.contentLength, 'contentLength'); + Subject> get headers => has((r) => r.headers, 'headers'); + // TODO persistentConnection, followRedirects, maxRedirects, finalized +} + +extension HttpRequestChecks on Subject { + Subject get contentLength => has((r) => r.contentLength, 'contentLength'); + Subject get encoding => has((r) => r.encoding, 'encoding'); + Subject> get bodyBytes => has((r) => r.bodyBytes, 'bodyBytes'); // TODO or Uint8List? + Subject get body => has((r) => r.body, 'body'); + Subject> get bodyFields => has((r) => r.bodyFields, 'bodyFields'); +} From b5eb79bd892d4f06805d38879612a4e39b8c0504 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 13:25:46 -0700 Subject: [PATCH 08/11] api tests: Add tests for ApiConnection.get --- test/api/core_test.dart | 49 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 test/api/core_test.dart diff --git a/test/api/core_test.dart b/test/api/core_test.dart new file mode 100644 index 0000000000..4c2bd88cbb --- /dev/null +++ b/test/api/core_test.dart @@ -0,0 +1,49 @@ +import 'dart:convert'; + +import 'package:checks/checks.dart'; +import 'package:http/http.dart' as http; +import 'package:test/scaffolding.dart'; +import 'package:zulip/api/core.dart'; + +import '../stdlib_checks.dart'; +import 'fake_api.dart'; +import '../example_data.dart' as eg; + +void main() { + test('ApiConnection.get', () async { + Future checkRequest(Map? params, String expectedRelativeUrl) { + return FakeApiConnection.with_(account: eg.selfAccount, (connection) async { + connection.prepare(body: jsonEncode({})); + await connection.get('example/route', params); + check(connection.lastRequest!).isA() + ..method.equals('GET') + ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl') + ..headers.deepEquals(authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey)) + ..body.equals(''); + }); + } + + checkRequest(null, '/api/v1/example/route'); + checkRequest({}, '/api/v1/example/route?'); + checkRequest({'x': 3}, '/api/v1/example/route?x=3'); + checkRequest({'x': 3, 'y': 4}, '/api/v1/example/route?x=3&y=4'); + checkRequest({'x': null}, '/api/v1/example/route?x=null'); + checkRequest({'x': true}, '/api/v1/example/route?x=true'); + checkRequest({'x': 'foo'}, '/api/v1/example/route?x=%22foo%22'); + checkRequest({'x': [1, 2]}, '/api/v1/example/route?x=%5B1%2C2%5D'); + checkRequest({'x': {'y': 1}}, '/api/v1/example/route?x=%7B%22y%22%3A1%7D'); + checkRequest({'x': RawParameter('foo')}, + '/api/v1/example/route?x=foo'); + checkRequest({'x': RawParameter('foo'), 'y': 'bar'}, + '/api/v1/example/route?x=foo&y=%22bar%22'); + }); + + test('API success result', () async { + await FakeApiConnection.with_(account: eg.selfAccount, (connection) async { + connection.prepare(body: jsonEncode({'result': 'success', 'x': 3})); + final result = await connection.get( + 'example/route', {'y': 'z'}); + check(result).deepEquals({'result': 'success', 'x': 3}); + }); + }); +} From 2ca6917d9aa9379dcc8547d3f7351230f4fa048c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 14:16:35 -0700 Subject: [PATCH 09/11] api tests: Cover ApiConnection.post --- test/api/core_test.dart | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 4c2bd88cbb..d0154716e6 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -38,6 +38,36 @@ void main() { '/api/v1/example/route?x=foo&y=%22bar%22'); }); + test('ApiConnection.post', () async { + Future checkRequest(Map? params, String expectedBody, {bool expectContentType = true}) { + return FakeApiConnection.with_(account: eg.selfAccount, (connection) async { + connection.prepare(body: jsonEncode({})); + await connection.post('example/route', params); + check(connection.lastRequest!).isA() + ..method.equals('POST') + ..url.asString.equals('${eg.realmUrl.origin}/api/v1/example/route') + ..headers.deepEquals({ + ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey), + if (expectContentType) + 'content-type': 'application/x-www-form-urlencoded; charset=utf-8', + }) + ..body.equals(expectedBody); + }); + } + + checkRequest(null, '', expectContentType: false); + checkRequest({}, ''); + checkRequest({'x': 3}, 'x=3'); + checkRequest({'x': 3, 'y': 4}, 'x=3&y=4'); + checkRequest({'x': null}, 'x=null'); + checkRequest({'x': true}, 'x=true'); + checkRequest({'x': 'foo'}, 'x=%22foo%22'); + checkRequest({'x': [1, 2]}, 'x=%5B1%2C2%5D'); + checkRequest({'x': {'y': 1}}, 'x=%7B%22y%22%3A1%7D'); + checkRequest({'x': RawParameter('foo')}, 'x=foo'); + checkRequest({'x': RawParameter('foo'), 'y': 'bar'}, 'x=foo&y=%22bar%22'); + }); + test('API success result', () async { await FakeApiConnection.with_(account: eg.selfAccount, (connection) async { connection.prepare(body: jsonEncode({'result': 'success', 'x': 3})); From f2d34396598f452b748e876ff09d1c06e54e94c4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 23 May 2023 16:13:08 -0700 Subject: [PATCH 10/11] api: Fix double slash in postFileFromStream request Caught this when I wrote tests for this method and its peers. Those tests are coming in the next commit. --- lib/api/core.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/api/core.dart b/lib/api/core.dart index 814f0eac93..b6910c41f0 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -85,7 +85,8 @@ class ApiConnection { } Future> postFileFromStream(String route, Stream> content, int length, { String? filename }) async { - http.MultipartRequest request = http.MultipartRequest('POST', Uri.parse("$realmUrl/api/v1/$route")) + final url = realmUrl.replace(path: "/api/v1/$route"); + final request = http.MultipartRequest('POST', url) ..files.add(http.MultipartFile('file', content, length, filename: filename)); return send(request); } From a910ebe51a95399c08533d413f90af9cae04618a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 10 May 2023 14:16:35 -0700 Subject: [PATCH 11/11] api tests: Cover ApiConnection.postFileFromStream --- test/api/core_test.dart | 32 ++++++++++++++++++++++++++++++++ test/stdlib_checks.dart | 14 ++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/test/api/core_test.dart b/test/api/core_test.dart index d0154716e6..e5a1287b4d 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -68,6 +68,38 @@ void main() { checkRequest({'x': RawParameter('foo'), 'y': 'bar'}, 'x=foo&y=%22bar%22'); }); + test('ApiConnection.postFileFromStream', () async { + Future checkRequest(List> content, int length, String? filename) { + return FakeApiConnection.with_(account: eg.selfAccount, (connection) async { + connection.prepare(body: jsonEncode({})); + await connection.postFileFromStream( + 'example/route', + Stream.fromIterable(content), length, filename: filename); + check(connection.lastRequest!).isA() + ..method.equals('POST') + ..url.asString.equals('${eg.realmUrl.origin}/api/v1/example/route') + ..headers.deepEquals(authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey)) + ..fields.deepEquals({}) + ..files.single.which(it() + ..field.equals('file') + ..length.equals(length) + ..filename.equals(filename) + ..has>>((f) => f.finalize().toBytes(), 'contents') + .completes(it()..deepEquals(content.expand((l) => l))) + ); + }); + } + + checkRequest([], 0, null); + checkRequest(['asdf'.codeUnits], 4, null); + checkRequest(['asd'.codeUnits, 'f'.codeUnits], 4, null); + + checkRequest(['asdf'.codeUnits], 4, 'info.txt'); + + checkRequest(['asdf'.codeUnits], 1, null); // nothing on client side catches a wrong length + checkRequest(['asdf'.codeUnits], 100, null); + }); + test('API success result', () async { await FakeApiConnection.with_(account: eg.selfAccount, (connection) async { connection.prepare(body: jsonEncode({'result': 'success', 'x': 3})); diff --git a/test/stdlib_checks.dart b/test/stdlib_checks.dart index 95756bd70e..5aeab0163c 100644 --- a/test/stdlib_checks.dart +++ b/test/stdlib_checks.dart @@ -43,3 +43,17 @@ extension HttpRequestChecks on Subject { Subject get body => has((r) => r.body, 'body'); Subject> get bodyFields => has((r) => r.bodyFields, 'bodyFields'); } + +extension HttpMultipartRequestChecks on Subject { + Subject> get fields => has((r) => r.fields, 'fields'); + Subject> get files => has((r) => r.files, 'files'); + Subject get contentLength => has((r) => r.contentLength, 'contentLength'); +} + +extension HttpMultipartFileChecks on Subject { + Subject get field => has((f) => f.field, 'field'); + Subject get length => has((f) => f.length, 'length'); + Subject get filename => has((f) => f.filename, 'filename'); + // TODO Subject get contentType => has((f) => f.contentType, 'contentType'); + Subject get isFinalized => has((f) => f.isFinalized, 'isFinalized'); +}