tearDown is called before a test block completes when there are async errors

[jiahaog]

Consider the following:

import 'package:test/test.dart';

Future<void> makeAsyncError() async {
  Future.error('some async error');
}

Future<void> nothing() async {}

void main() {
  test('foo', () async {
    // Same behavior if this is declared with a [tearDown] outside the [test]
    // block.
    addTearDown(() => print('tearDown is called'));

    await makeAsyncError();
    await nothing();

    // If we put the [addTearDown] after the async calls instead, "tearDown is
    // called" will not be printed at all.
    // addTearDown(() => print('tearDown is called'));

    print('End of test');
  });
}

Output:

00:01 +0 -1: test/foo_test.dart: foo [E]
  some async error

00:01 +0 -1: loading test/foo_test.dart
Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'pub run test --chain-stack-traces'.
00:01 +0 -1: test/foo_test.dart: foo
teardown is called
End of test
00:01 +0 -1: Some tests failed.

It would be expected that "tearDown is called" should be printed after "End of test":

00:01 +0 -1: test/foo_test.dart: foo [E]
  some async error

00:01 +0 -1: loading test/foo_test.dart
Consider enabling the flag chain-stack-traces to receive more detailed exceptions.
For example, 'pub run test --chain-stack-traces'.
00:01 +0 -1: test/foo_test.dart: foo
End of test
teardown is called
00:01 +0 -1: Some tests failed.

await nothing(); in the example above is needed to cause this incorrect behavior.

Versions:

Dart SDK version: 2.13.0-38.0.dev (dev) (Mon Feb 15 10:21:50 2021 -0800) on "macos_x64"
package:test: 1.17.2

[jakemac53]

cc @natebosch do you think the recent changes could have modified the behavior here?

[jiahaog]

This is probably an existing problem for quite some time - I was able to reproduce this internally while synced to a release from ~1 year ago

[natebosch]

This is probably working as intended, but I would imagine we should be able to change it.

What is the motivation for wanting the test to continue? The downside is that some tests that fail quickly today could fail much more slowly (with timeouts) if we always wait for the main body of a test to complete even if we know the test won't pass.

[jakemac53]

Running tearDown before the test completes is likely to cause other errors in the test and mask the original failure? For instance it might kill processes or delete temp directories that the test is using.

I think we probably should allow the test to continue and not run tearDown until the test is completed.

[jiahaog]

Context: I found this when looking into flutter/flutter#81521. This happens in flutter_test because it makes the assumption that addTearDown is only called after the test body finishes, resulting in tripped assertions. The following is a (vastly) simplified version of how testWidgets is implemented:

void testWidgets(String description, Future<void> Function() callback) {
  test(description, () {
    bool inTest = true;

    addTearDown(() => inTest = false);

    return () async {
      assert(inTest);

      await callback();
      await nothing();

      // tearDown runs around here, setting inTest to false.

      assert(inTest); // Throws AssertionError

      // More omitted clean up...
    }();
  });
}

Future<void> nothing() async {}