Enable unawaited_futures

[PIG208]

Fixes part of #731

[PIG208]

TODO: TestGlobalStore's doInsertAccount implementation has a race condition for checking the invariants. Concurrently adding accounts allows you to bypass the check.

[chrisbobbe]

This sounds helpful!! It looks like we have an issue for it: #731 🙂

[PIG208]

This is stacked on top of #937.

[gnprice]

I just wrote down some thoughts on showErrorDialog's API on a related PR:
#937 (comment)

so adding a link to that here.

I think the many unawaited calls aroung showErrorDialog here get fairly noisy. That makes the diff bigger, which is undesirable but could be OK… but then I think it also leaves enough noise in the tree that it might start to undermine the effect of this lint. If we find ourselves scattering unawaited calls all over because there are some functions that just need it most times you call them, then the other more interesting unawaited calls don't stand out so much.

Probably a good fix for that is to have our own pair of showFooDialog wrappers provide an API that's more like the one I describe in that comment.

That would also be a better API for showDialog upstream to have. But it's a tough change to make, because the change in return type would break tons of downstream callers; I don't see a clear way to get to a great API given that constraint. So I wouldn't invest in trying to make the change upstream.

[PIG208]

Yeah, I like the showSnackBar API. Future is more primitive in the sense that it doesn't carry much contextual information. The wrapper class is a step up from simply using typedef, and offers good documentation discoverability.

[PIG208]

This is stacked on top of #970 (dialog [nfc]: Wrap showErrorDialog's return value).

The PR is mostly small commits for refactors in a limited scope.

[gnprice]

Are these needed? If I munge the expected value here, the test duly fails, even without await:

00:05 +0 -1: ApiConnection.get [E]                                                            
  Expected: a BaseRequest that:
    is a Request
    has method that:
      equals 'GET'
    has url that:
      has toString that:
        equals 'https://chat.example/api/v1/example/rote'
  Actual: a BaseRequest that:
    is a Request
    has method that:
      equals 'GET'
    has url that:
      has toString that:
      Actual: 'https://chat.example/api/v1/example/route'
      Which: differs at offset 38:
      ... example/rote
      ... example/route
                    ^
  package:checks/src/checks.dart 85:9               check.<fn>
  package:checks/src/checks.dart 708:12             _TestContext.expect
  package:checks/src/extensions/string.dart 108:13  StringChecks.equals
  test/api/core_test.dart 28:26                     main.<fn>.checkRequest.<fn>
  ===== asynchronous gap ===========================
  dart:async                                        _CustomZone.registerBinaryCallback
  test/api/fake_api.dart 189:14                     FakeApiConnection.with_
  test/api/core_test.dart 23:32                     main.<fn>.checkRequest
  test/api/core_test.dart 37:5                      main.<fn>
  
  This test failed after it had already completed.
  Make sure to use a matching library which informs the test runner
  of pending async work.
  package:checks/src/checks.dart 85:9               check.<fn>
  package:checks/src/checks.dart 708:12             _TestContext.expect
  package:checks/src/extensions/string.dart 108:13  StringChecks.equals
  test/api/core_test.dart 28:26                     main.<fn>.checkRequest.<fn>
  ===== asynchronous gap ===========================
  dart:async                                        _CustomZone.registerBinaryCallback
  test/api/fake_api.dart 189:14                     FakeApiConnection.with_
  test/api/core_test.dart 23:32                     main.<fn>.checkRequest
  test/api/core_test.dart 37:5                      main.<fn>
  
00:05 +17 -1: Some tests failed.                                                              

There is that warning message. But it points at a different solution: just make sure the test runner knows there's some pending async work.

That in turn can I think be done with check(…).completes(). For example:

   test('ApiConnection.get', () async {
-    Future<void> checkRequest(Map<String, dynamic>? params, String expectedRelativeUrl) {
-      return FakeApiConnection.with_(account: eg.selfAccount, (connection) async {
+    void checkRequest(Map<String, dynamic>? params, String expectedRelativeUrl) {
+      check(FakeApiConnection.with_(account: eg.selfAccount, (connection) async {
         connection.prepare(json: {});
         await connection.get(kExampleRouteName, (json) => json, 'example/route', params);
         check(connection.lastRequest!).isA<http.Request>()
           ..method.equals('GET')
           ..url.asString.equals('${eg.realmUrl.origin}$expectedRelativeUrl')
           ..headers.deepEquals({
             ...authHeader(email: eg.selfAccount.email, apiKey: eg.selfAccount.apiKey),
             ...kFallbackUserAgentHeader,
           })
           ..body.equals('');
-      });
+      })).completes();
     }

… In fact, looking at the implementation of completes, I think it can be a bit cleaner than that. I'll send a PR.

[PIG208]

Skimming on mobile, this should also apply to c516066, right? I recall that check(SomethingFuture) has some special handling.

[gnprice]

Yeah, the new finish helper in #985 can probably apply to some other places as well. No need to go sweep through using it in all the existing tests, but worth using to simplify new tests or tests changed in this PR.

[gnprice]

Thanks @PIG208 for cleaning these up, and thanks @chrisbobbe for the previous review! Generally this all looks good; small comments below.

[gnprice]

nit in commit message: duplicate text

The `Future` that `Navigator.push` returns resolves when a
corresponding `Navigator.pop` is called.

The future Navgiator.push returns completes when the there is a
corresponding Navigator.pop call.  We don't use that as of now, so we
use unawaited to explicitly discard it.

[gnprice]

Huh, odd. I wonder why this test is using tester.pump — is that just a substitute for awaiting the future?

What if we replaced both these lines with this?

      await markNarrowAsRead(context, narrow);

(Similarly for the other markNarrowAsRead calls in this file.)

[PIG208]

It's stuck forever, right before the connection.post call to "messages/flags/narrow". It's puzzling that it happens to a FakeApiConnection though. If I remove the connection.prepare call, the test would complete just fine, because updateMessageFlagsStartingFromAnchor catches all errors.

[PIG208]

I guess this is because we call it inside testWidgets. This issue is not specific to markNarrowAsRead. For example, a store.sendMessage call stucks as well. The FakeApiClient returns a Future.delayed(response.delay, computation); that does not complete in such tests.

[gnprice]

Ah, I see. So basically it's running into the problem that awaitFakeAsync solves.

It feels odd for these tests to not be awaiting that future — it means they're not ensuring that the requests have actually completed, and that the function has done the work it's going to do when it gets the responses, before they complete.

I'll send a quick separate PR that fixes these.

[gnprice]

#993

[gnprice]

For throws, I think the normal thing is to await the future:

Suggested change

	finish(check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion(
	id: Value(1234)))).throws());
	await check(globalStore.updateAccount(eg.selfAccount.id, const AccountsCompanion(
	id: Value(1234)))).throws();

From the docs linked in the commit message:

Expectation extension methods checking asynchronous behavior return a Future. The future should typically be awaited within the test body, […]

[gnprice]

The commit message steps into a classic GitHub pitfall 🙂 :

Partially fixes: #731

GitHub doesn't understand the "partially", so that will close the issue.

If you don't mean "fixes", then the trick is to put some other word between it and the issue number. For example:

Fixes-partly: #731

[gnprice]

nit in commit message: the convention for email-header-style lines at the end of a commit message is that the words before the colon are joined by hyphens -, not spaces

[PIG208]

Thanks for the review! Updated the PR and fixed some more errors that appeared after rebasing (and applied #731)

[gnprice]

Thanks for the revision! All looks good; merging, with one NFC commit on top:

1aef868 api test [nfc]: Simplify some unawaited checks