Fix issue with async callbacks that get called synchronously while running test case function causing the test to be marked as complete prematurely.

Added an optional id to completion matchers that can be used in error messages (this was invaluable in tracking down the above problem).

Re-enable pub tests. https://code.google.com/p/dart/issues/detail?id=8862
Review URL: https://codereview.chromium.org//12393017

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@19339 260f80e4-7a28-3924-810f-c04153c831b5

pkg/scheduled_test/lib/scheduled_test.dart

@@ -316,7 +316,7 @@ void _setUpScheduledTest([void setUpFn()]) {
 /// initialized.
 void _ensureInitialized() {
   unittest.ensureInitialized();
-  unittest.wrapAsync = (f) {
+  unittest.wrapAsync = (f, [id = '']) {
     if (currentSchedule == null) {
       throw new StateError("Unexpected call to wrapAsync with no current "
           "schedule.");

pkg/unittest/lib/src/expect.dart

@@ -25,7 +25,7 @@ class TestFailure {
  * used safely. For example, the unittest library will set this
  * to be expectAsync1. By default this is an identity function.
  */
-Function wrapAsync = (f) => f;
+Function wrapAsync = (f, [id]) => f;
 
 /**
  * This is the main assertion function. It asserts that [actual]

pkg/unittest/lib/src/future_matchers.dart

@@ -13,7 +13,7 @@ part of matcher;
  * To test that a Future completes with an exception, you can use [throws] and
  * [throwsA].
  */
-Matcher completes = const _Completes(null);
+Matcher completes = const _Completes(null, '');
 
 /**
  * Matches a [Future] that completes succesfully with a value that matches
@@ -23,23 +23,29 @@ Matcher completes = const _Completes(null);
  *
  * To test that a Future completes with an exception, you can use [throws] and
  * [throwsA].
+ *
+ * [id] is an optional tag that can be used to identify the completion matcher
+ * in error messages.
  */
-Matcher completion(matcher) => new _Completes(wrapMatcher(matcher));
+Matcher completion(matcher, [String id = '']) =>
+    new _Completes(wrapMatcher(matcher), id);
 
 class _Completes extends BaseMatcher {
   final Matcher _matcher;
+  final String _id;
 
-  const _Completes(this._matcher);
+  const _Completes(this._matcher, String id)
+      : this._id = (id == '') ? '' : '$id ';
 
   bool matches(item, MatchState matchState) {
     if (item is! Future) return false;
-    var done = wrapAsync((fn) => fn());
+    var done = wrapAsync((fn) => fn(), _id);
 
     item.then((value) {
       done(() { if (_matcher != null) expect(value, _matcher); });
     }, onError: (e) {
-      var reason = 'Expected future to complete successfully, but it failed '
-                   'with ${e.error}';
+      var reason = 'Expected future ${_id}to complete successfully, '
+                   'but it failed with ${e.error}';
       if (e.stackTrace != null) {
         var stackTrace = e.stackTrace.toString();
         stackTrace = '  ${stackTrace.replaceAll('\n', '\n  ')}';

pkg/unittest/lib/src/test_case.dart

@@ -78,7 +78,12 @@ class TestCase {
 
   Future _runTest() {
     _prepTest();
+    // Increment/decrement callbackFunctionsOutstanding to prevent
+    // synchronous 'async' callbacks from causing the  test to be
+    // marked as complete before the body is completely executed.
+    ++callbackFunctionsOutstanding;
     var f = test();
+    --callbackFunctionsOutstanding;
     if (f is Future) {
       f.then((_) => _finishTest())
        .catchError((e) => fail("${e.error}"));
@@ -204,4 +209,10 @@ class TestCase {
   void error(String messageText, [String stack = '']) {
     _complete(ERROR, messageText, stack);
   }
+
+  void markCallbackComplete() {
+    if (--callbackFunctionsOutstanding == 0 && !isComplete) {
+      pass();
+    }
+  }
 }

pkg/unittest/lib/unittest.dart

@@ -362,8 +362,9 @@ class _SpreadArgsHelper {
       // if it previously passed.
       if (testCase.result == PASS) {
         testCase.error(
-            'Callback ${id}called after test case ${testCase.description} '
-            'has already been marked as done.', '');
+            'Callback ${id}called ($actualCalls) after test case '
+            '${testCase.description} has already been marked as '
+            '${testCase.result}.', '');
       }
       return false;
     } else if (maxExpectedCalls >= 0 && actualCalls > maxExpectedCalls) {
@@ -381,10 +382,7 @@ class _SpreadArgsHelper {
       // Mark this callback as complete and remove it from the testcase
       // oustanding callback count; if that hits zero the testcase is done.
       complete = true;
-      if (--testCase.callbackFunctionsOutstanding == 0 &&
-         !testCase.isComplete) {
-        testCase.pass();
-      }
+      testCase.markCallbackComplete();
     }
   }
 
@@ -811,7 +809,7 @@ void ensureInitialized() {
   }
   _initialized = true;
   // Hook our async guard into the matcher library.
-  wrapAsync = expectAsync1;
+  wrapAsync = (f, [id]) => expectAsync1(f, id: id);
 
   _tests = <TestCase>[];
   _testRunner = _nextBatch;

pkg/unittest/test/unittest_test.dart

@@ -386,8 +386,8 @@ main() {
     buildStatusString(1, 0, 0, tests[9], count: 10),
     buildStatusString(0, 1, 0, tests[10], message: 'Caught error!'),
     buildStatusString(1, 0, 1, 'testOne',
-        message: 'Callback called after test case testOne has already '
-                 'been marked as done.:testTwo:'),
+        message: 'Callback called (2) after test case testOne has already '
+                 'been marked as pass.:testTwo:'),
     buildStatusString(2, 1, 0,
         'testOne::testTwo:Expected: false but: was <true>.:testThree'),
     buildStatusString(2, 0, 3, 

utils/tests/pub/pub.status

@@ -9,9 +9,6 @@ pub_uploader_test: Pass,Fail
 pub_lish_test: Pass, Fail # Issue 8868
 validator_test: Pass, Fail # Issue 8868
 
-# TODO(gram): These broke with r19269; investigating.
-error_group_test: Fail
-
 # Pub only runs on the VM, so just rule out all compilers.
 [ $compiler == dart2js || $compiler == dart2dart ]
 *: Skip