Merge pull request flutter#713 from dart-lang/add-tear-down-all

Make addTearDown() play nice with setUpAll()

Author: nex3

CHANGELOG.md

@@ -1,5 +1,9 @@
 ## 0.12.27
 
+* When `addTearDown()` is called within a call to `setUpAll()`, it runs its
+  callback after *all* tests instead of running it after the `setUpAll()`
+  callback.
+
 * When running in an interactive terminal, the test runner now prints status
   lines as wide as the terminal and no wider.
 

lib/src/backend/declarer.dart

@@ -155,11 +155,15 @@ class Declarer {
         }
       }
 
-      await Invoker.current.waitForOutstandingCallbacks(() async {
-        await _runSetUps();
-        await body();
-      });
-    }, trace: _collectTraces ? new Trace.current(2) : null));
+      await runZoned(
+          () => Invoker.current.waitForOutstandingCallbacks(() async {
+                await _runSetUps();
+                await body();
+              }),
+          // Make the declarer visible to running tests so that they'll throw
+          // useful errors when calling `test()` and `group()` within a test.
+          zoneValues: {#test.declarer: this});
+    }, trace: _collectTraces ? new Trace.current(2) : null, guarded: false));
   }
 
   /// Creates a group of tests.
@@ -224,7 +228,14 @@ class Declarer {
     _tearDownAlls.add(callback);
   }
 
+  /// Like [tearDownAll], but called from within a running [setUpAll] test to
+  /// dynamically add a [tearDownAll].
+  void addTearDownAll(callback()) => _tearDownAlls.add(callback);
+
   /// Finalizes and returns the group being declared.
+  ///
+  /// **Note**: The tests in this group must be run in a [Invoker.guard]
+  /// context; otherwise, test errors won't be captured.
   Group build() {
     _checkNotBuilt("build");
 
@@ -258,18 +269,30 @@ class Declarer {
     if (_setUpAlls.isEmpty) return null;
 
     return new LocalTest(_prefix("(setUpAll)"), _metadata, () {
-      return Future.forEach(_setUpAlls, (setUp) => setUp());
-    }, trace: _setUpAllTrace);
+      return runZoned(() => Future.forEach(_setUpAlls, (setUp) => setUp()),
+          // Make the declarer visible to running scaffolds so they can add to
+          // the declarer's `tearDownAll()` list.
+          zoneValues: {#test.declarer: this});
+    }, trace: _setUpAllTrace, guarded: false, isScaffoldAll: true);
   }
 
   /// Returns a [Test] that runs the callbacks in [_tearDownAll].
   Test get _tearDownAll {
-    if (_tearDownAlls.isEmpty) return null;
+    // We have to create a tearDownAll if there's a setUpAll, since it might
+    // dynamically add tear-down code using [addTearDownAll].
+    if (_setUpAlls.isEmpty && _tearDownAlls.isEmpty) return null;
 
     return new LocalTest(_prefix("(tearDownAll)"), _metadata, () {
-      return Invoker.current.unclosable(() {
-        return Future.forEach(_tearDownAlls.reversed, errorsDontStopTest);
-      });
-    }, trace: _tearDownAllTrace);
+      return runZoned(() {
+        return Invoker.current.unclosable(() async {
+          while (_tearDownAlls.isNotEmpty) {
+            await errorsDontStopTest(_tearDownAlls.removeLast());
+          }
+        });
+      },
+          // Make the declarer visible to running scaffolds so they can add to
+          // the declarer's `tearDownAll()` list.
+          zoneValues: {#test.declarer: this});
+    }, trace: _tearDownAllTrace, guarded: false, isScaffoldAll: true);
   }
 }

lib/src/backend/invoker.dart

@@ -10,6 +10,7 @@ import '../frontend/expect.dart';
 import '../runner/load_suite.dart';
 import '../utils.dart';
 import 'closed_exception.dart';
+import 'declarer.dart';
 import 'group.dart';
 import 'live_test.dart';
 import 'live_test_controller.dart';
@@ -28,21 +29,38 @@ class LocalTest extends Test {
   final Metadata metadata;
   final Trace trace;
 
+  /// Whether this is a test defined using `setUpAll()` or `tearDownAll()`.
+  final bool isScaffoldAll;
+
   /// The test body.
-  final AsyncFunction _body;
+  final Function() _body;
+
+  /// Whether the test is run in its own error zone.
+  final bool _guarded;
 
-  LocalTest(this.name, this.metadata, body(), {this.trace}) : _body = body;
+  /// Creates a new [LocalTest].
+  ///
+  /// If [guarded] is `true`, the test is run in its own error zone, and any
+  /// errors that escape that zone cause the test to fail. If it's `false`, it's
+  /// the caller's responsiblity to invoke [LiveTest.run] in the context of a
+  /// call to [Invoker.guard].
+  LocalTest(this.name, this.metadata, this._body,
+      {this.trace, bool guarded: true, this.isScaffoldAll: false})
+      : _guarded = guarded;
+
+  LocalTest._(this.name, this.metadata, this._body, this.trace, this._guarded,
+      this.isScaffoldAll);
 
   /// Loads a single runnable instance of this test.
   LiveTest load(Suite suite, {Iterable<Group> groups}) {
-    var invoker = new Invoker._(suite, this, groups: groups);
+    var invoker = new Invoker._(suite, this, groups: groups, guarded: _guarded);
     return invoker.liveTest;
   }
 
   Test forPlatform(TestPlatform platform, {OperatingSystem os}) {
     if (!metadata.testOn.evaluate(platform, os: os)) return null;
-    return new LocalTest(name, metadata.forPlatform(platform, os: os), _body,
-        trace: trace);
+    return new LocalTest._(name, metadata.forPlatform(platform, os: os), _body,
+        trace, _guarded, isScaffoldAll);
   }
 }
 
@@ -58,6 +76,9 @@ class Invoker {
   LiveTest get liveTest => _controller.liveTest;
   LiveTestController _controller;
 
+  /// Whether to run this test in its own error zone.
+  final bool _guarded;
+
   /// Whether the test can be closed in the current zone.
   bool get _closable => Zone.current[_closableKey];
 
@@ -118,6 +139,22 @@ class Invoker {
     return Zone.current[#test.invoker];
   }
 
+  /// Runs [callback] in a zone where unhandled errors from [LiveTest]s are
+  /// caught and dispatched to the appropriate [Invoker].
+  static T guard<T>(T callback()) =>
+      runZoned(callback, zoneSpecification: new ZoneSpecification(
+          // Use [handleUncaughtError] rather than [onError] so we can
+          // capture [zone] and with it the outstanding callback counter for
+          // the zone in which [error] was thrown.
+          handleUncaughtError: (self, _, zone, error, stackTrace) {
+        var invoker = zone[#test.invoker];
+        if (invoker != null) {
+          self.parent.run(() => invoker._handleError(zone, error, stackTrace));
+        } else {
+          self.parent.handleUncaughtError(error, stackTrace);
+        }
+      }));
+
   /// The zone that the top level of [_test.body] is running in.
   ///
   /// Tracking this ensures that [_timeoutTimer] isn't created in a
@@ -135,7 +172,9 @@ class Invoker {
   /// Messages to print if and when this test fails.
   final _printsOnFailure = <String>[];
 
-  Invoker._(Suite suite, LocalTest test, {Iterable<Group> groups}) {
+  Invoker._(Suite suite, LocalTest test,
+      {Iterable<Group> groups, bool guarded: true})
+      : _guarded = guarded {
     _controller = new LiveTestController(
         suite, test, _onRun, _onCloseCompleter.complete,
         groups: groups);
@@ -147,7 +186,12 @@ class Invoker {
   /// run in the reverse of the order they're declared.
   void addTearDown(callback()) {
     if (closed) throw new ClosedException();
-    _tearDowns.add(callback);
+
+    if (_test.isScaffoldAll) {
+      Declarer.current.addTearDownAll(callback);
+    } else {
+      _tearDowns.add(callback);
+    }
   }
 
   /// Tells the invoker that there's a callback running that it should wait for
@@ -282,7 +326,15 @@ class Invoker {
   void _handleError(Zone zone, error, [StackTrace stackTrace]) {
     // Ignore errors propagated from previous test runs
     if (_runCount != zone[#runCount]) return;
-    if (stackTrace == null) stackTrace = new Chain.current();
+
+    // Get the chain information from the zone in which the error was thrown.
+    zone.run(() {
+      if (stackTrace == null) {
+        stackTrace = new Chain.current();
+      } else {
+        stackTrace = new Chain.forTrace(stackTrace);
+      }
+    });
 
     // Store these here because they'll change when we set the state below.
     var shouldBeDone = liveTest.state.shouldBeDone;
@@ -334,60 +386,68 @@ class Invoker {
 
     _runCount++;
     Chain.capture(() {
-      runZoned(() async {
-        _invokerZone = Zone.current;
-        _outstandingCallbackZones.add(Zone.current);
-
-        // Run the test asynchronously so that the "running" state change has
-        // a chance to hit its event handler(s) before the test produces an
-        // error. If an error is emitted before the first state change is
-        // handled, we can end up with [onError] callbacks firing before the
-        // corresponding [onStateChkange], which violates the timing
-        // guarantees.
-        //
-        // Using [new Future] also avoids starving the DOM or other
-        // microtask-level events.
-        new Future(() async {
-          await _test._body();
-          await unclosable(_runTearDowns);
-          removeOutstandingCallback();
-        });
-
-        await _outstandingCallbacks.noOutstandingCallbacks;
-        if (_timeoutTimer != null) _timeoutTimer.cancel();
-
-        if (liveTest.state.result != Result.success &&
-            _runCount < liveTest.test.metadata.retry + 1) {
+      _guardIfGuarded(() {
+        runZoned(() async {
+          _invokerZone = Zone.current;
+          _outstandingCallbackZones.add(Zone.current);
+
+          // Run the test asynchronously so that the "running" state change
+          // has a chance to hit its event handler(s) before the test produces
+          // an error. If an error is emitted before the first state change is
+          // handled, we can end up with [onError] callbacks firing before the
+          // corresponding [onStateChkange], which violates the timing
+          // guarantees.
+          //
+          // Using [new Future] also avoids starving the DOM or other
+          // microtask-level events.
+          new Future(() async {
+            await _test._body();
+            await unclosable(_runTearDowns);
+            removeOutstandingCallback();
+          });
+
+          await _outstandingCallbacks.noOutstandingCallbacks;
+          if (_timeoutTimer != null) _timeoutTimer.cancel();
+
+          if (liveTest.state.result != Result.success &&
+              _runCount < liveTest.test.metadata.retry + 1) {
+            _controller
+                .message(new Message.print("Retry: ${liveTest.test.name}"));
+            _onRun();
+            return;
+          }
+
           _controller
-              .message(new Message.print("Retry: ${liveTest.test.name}"));
-          _onRun();
-          return;
-        }
+              .setState(new State(Status.complete, liveTest.state.result));
+
+          _controller.completer.complete();
+        },
+            zoneValues: {
+              #test.invoker: this,
+              // Use the invoker as a key so that multiple invokers can have
+              // different outstanding callback counters at once.
+              _counterKey: outstandingCallbacksForBody,
+              _closableKey: true,
+              #runCount: _runCount,
+            },
+            zoneSpecification: new ZoneSpecification(
+                print: (_, __, ___, line) => _print(line)));
+      });
+    }, when: liveTest.test.metadata.chainStackTraces, errorZone: false);
+  }
 
-        _controller.setState(new State(Status.complete, liveTest.state.result));
-
-        _controller.completer.complete();
-      },
-          zoneValues: {
-            #test.invoker: this,
-            // Use the invoker as a key so that multiple invokers can have different
-            // outstanding callback counters at once.
-            _counterKey: outstandingCallbacksForBody,
-            _closableKey: true,
-            #runCount: _runCount
-          },
-          zoneSpecification: new ZoneSpecification(
-              print: (self, parent, zone, line) =>
-                  _controller.message(new Message.print(line)),
-              // Use [handleUncaughtError] rather than [onError] so we can
-              // capture [zone] and with it the outstanding callback counter for
-              // the zone in which [error] was thrown.
-              handleUncaughtError: (self, _, zone, error, stackTrace) => self
-                  .parent
-                  .run(() => _handleError(zone, error, stackTrace))));
-    }, when: liveTest.test.metadata.chainStackTraces);
+  /// Runs [callback], in a [Invoker.guard] context if [_guarded] is `true`.
+  void _guardIfGuarded(void callback()) {
+    if (_guarded) {
+      Invoker.guard(callback);
+    } else {
+      callback();
+    }
   }
 
+  /// Prints [text] as a message to [_controller].
+  void _print(String text) => _controller.message(new Message.print(text));
+
   /// Run [_tearDowns] in reverse order.
   Future _runTearDowns() async {
     while (_tearDowns.isNotEmpty) {

lib/src/runner/remote_listener.dart

@@ -9,6 +9,7 @@ import 'package:term_glyph/term_glyph.dart' as glyph;
 
 import '../backend/declarer.dart';
 import '../backend/group.dart';
+import '../backend/invoker.dart';
 import '../backend/live_test.dart';
 import '../backend/metadata.dart';
 import '../backend/operating_system.dart';
@@ -97,7 +98,14 @@ class RemoteListener {
               : OperatingSystem.find(message['os']),
           path: message['path']);
 
-      new RemoteListener._(suite, printZone)._listen(channel);
+      runZoned(() {
+        Invoker.guard(
+            () => new RemoteListener._(suite, printZone)._listen(channel));
+      },
+          // Make the declarer visible to running tests so that they'll throw
+          // useful errors when calling `test()` and `group()` within a test,
+          // and so they can add to the declarer's `tearDownAll()` list.
+          zoneValues: {#test.declarer: declarer});
     }, onError: (error, stackTrace) {
       _sendError(channel, error, stackTrace, verboseChain);
     }, zoneSpecification: new ZoneSpecification(print: (_, __, ___, line) {

lib/test.dart

@@ -67,7 +67,8 @@ Declarer get _declarer {
     ExpandedReporter.watch(engine,
         color: true, printPath: false, printPlatform: false);
 
-    var success = await engine.run();
+    var success = await runZoned(() => Invoker.guard(engine.run),
+        zoneValues: {#test.declarer: _globalDeclarer});
     // TODO(nweiz): Set the exit code on the VM when issue 6943 is fixed.
     if (success) return null;
     print('');
@@ -252,6 +253,9 @@ void tearDown(callback()) => _declarer.tearDown(callback);
 ///
 /// The [callback] is run before any callbacks registered with [tearDown]. Like
 /// [tearDown], the most recently registered callback is run first.
+///
+/// If this is called from within a [setUpAll] or [tearDownAll] callback, it
+/// instead runs the function after *all* tests in the current test suite.
 void addTearDown(callback()) {
   if (Invoker.current == null) {
     throw new StateError("addTearDown() may only be called within a test.");

pubspec.yaml

@@ -1,5 +1,5 @@
 name: test
-version: 0.12.27-dev
+version: 0.12.27
 author: Dart Team <misc@dartlang.org>
 description: A library for writing dart unit tests.
 homepage: https://github.com/dart-lang/test
@@ -29,7 +29,7 @@ dependencies:
   source_map_stack_trace: '^1.1.4'
   source_maps: '^0.10.2'
   source_span: '^1.4.0'
-  stack_trace: '^1.6.0'
+  stack_trace: '^1.9.0'
   stream_channel: '^1.6.0'
   string_scanner: '>=0.1.1 <2.0.0'
   term_glyph: '^1.0.0'