When we have excess callbacks, throw instead of calling error() so that we

pkg/unittest/lib/unittest.dart

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * A library for writing dart unit tests.
  *
  * To import this library, use the pub package manager.
  * Create a pubspec.yaml file in your project and add
  * a dependency on unittest with the following lines:
@@ skipping 270 matching lines @@
 /** Sentinel value for [_SpreadArgsHelper]. */
 class _Sentinel {
   const _Sentinel();
 }
 
 /** Simulates spread arguments using named arguments. */
 // TODO(sigmund): remove this class and simply use a closure with named
 // arguments (if still applicable).
 class _SpreadArgsHelper {
   Function _callback;
-  int _expectedCalls;
+  int _minExpectedCalls;

[Siggi Cherem (dart-lang), 2013/02/28 19:29:23]

Now that we don't have _init, can we make some of these final?

I think we can do many of them

[gram, 2013/02/28 22:49:50]

Done.

+  int _maxExpectedCalls;
   int _actualCalls = 0;
   int _testNum;
   TestCase _testCase;
-  Function _shouldCallBack;
   Function _isDone;
   String _id;
+  bool _complete;

[Siggi Cherem (dart-lang), 2013/02/28 19:29:23]

since this class is private and we don't return it, we can actually remove the _
from all these fields

[gram, 2013/02/28 22:49:50]

Done.

   static const _sentinel = const _Sentinel();
 
-  _init(Function callback, Function shouldCallBack, Function isDone,
-       [expectedCalls = 0]) {
+  _SpreadArgsHelper(Function callback, int minExpected, int maxExpected,

[Siggi Cherem (dart-lang), 2013/02/28 19:29:23]

more precisely:
_SpreadArgsHelper(Function callback, this.minExpected, int maxExpected,
this._isDone, String id)
    : this.callback = callback,
      this.maxExpected = (maxExpected == 0 && minExpected > 0) ? minExpected :
maxExpected,
      this.id = _computeId(id, callback) {
    ...
}

[gram, 2013/02/28 22:49:50]

Done.

+                   Function isDone, String id) {
     ensureInitialized();
     if (!(_currentTest >= 0 &&
            _currentTest < _tests.length &&
            _tests[_currentTest] != null)) {
       print("No valid test, did you forget to run your test inside a call "
           "to test()?");
     }
     assert(_currentTest >= 0 &&
            _currentTest < _tests.length &&
            _tests[_currentTest] != null);
     _callback = callback;
-    _shouldCallBack = shouldCallBack;
+    _minExpectedCalls = minExpected;
+    if (maxExpected == 0 && minExpected > 0) {
+      _maxExpectedCalls = minExpected;
+    } else {
+      _maxExpectedCalls = maxExpected;
+    }
     _isDone = isDone;
-    _expectedCalls = expectedCalls;
     _testNum = _currentTest;
     _testCase = _tests[_currentTest];
-    if (expectedCalls > 0) {
+    if (isDone != null || minExpected > 0) {
       _testCase.callbackFunctionsOutstanding++;
+      _complete = false;
+    } else {
+      _complete = true;
     }
+
+    // Try to create a reasonable id.
     _id = '';
-    // If the callback is not an anonymous closure, try to get the
-    // name.
-    var fname = callback.toString();
-    var prefix = "Function '";
-    var pos = fname.indexOf(prefix);
-    if (pos > 0) {
-      pos += prefix.length;
-      var epos = fname.indexOf("'", pos);
-      if (epos > 0) {
-        _id = "${fname.substring(pos, epos)} ";
+    if (id != null) {
+      _id = "$id ";
+    } else {
+      // If the callback is not an anonymous closure, try to get the
+      // name.
+      var fname = callback.toString();
+      var prefix = "Function '";
+      var pos = fname.indexOf(prefix);
+      if (pos > 0) {
+        pos += prefix.length;
+        var epos = fname.indexOf("'", pos);
+        if (epos > 0) {
+          _id = "${fname.substring(pos, epos)} ";
+        }
       }
     }
   }
 
-  _SpreadArgsHelper(callback, shouldCallBack, isDone) {
-    _init(callback, shouldCallBack, isDone);
+  _shouldCallBack() {
+    ++_actualCalls;
+    if (_testCase.isComplete) {
+      // Don't run if the test is done. We don't throw here as this is not
+      // the current test, but we do mark the old test as having an error
+      // if it previously passed.
+      if (_testCase.result == PASS) {
+        _testCase.error(
+            'Callback ${_id}called after test case ${_testCase.description} '
+            'has already been marked as done.',
+            '');
+      }
+      return false;
+    } else if (_maxExpectedCalls >= 0 && _actualCalls > _maxExpectedCalls) {
+      throw new TestFailure('Callback ${_id}called more times than expected '
+                            '($_maxExpectedCalls).');
+    }
+    return true;
   }
 
-  _SpreadArgsHelper.fixedCallCount(callback, expectedCalls, id) {
-    _init(callback, _checkCallCount, _allCallsDone, expectedCalls);
-    if (id != null) {
-      _id = "$id ";
-    }
-  }
-
-  _SpreadArgsHelper.variableCallCount(callback, isDone) {
-    _init(callback, _always, isDone, 1);
-   }
-
-  _SpreadArgsHelper.optionalCalls(callback) {
-    _init(callback, _always, () => false, 0);
-   }
-
   _after() {
-    if (_isDone()) {
-      _handleCallbackFunctionComplete(_testNum, _id);
-    }
-  }
-
-  _allCallsDone() => _actualCalls == _expectedCalls;
-
-  _always() {
-    // Always run except if the test is done.
-    if (_testCase.isComplete) {
-      _testCase.error(
-          'Callback ${_id}called after already being marked '
-          'as done ($_actualCalls).',
-          '');
-      return false;
-    } else {
-      return true;
+    if (!_complete) {
+      if (_minExpectedCalls > 0 && _actualCalls < _minExpectedCalls) {
+        return;
+      }
+      if (_isDone != null && !_isDone()) {
+        return;
+      }
+      // 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();
+      }
     }
   }
 
   invoke([arg0 = _sentinel, arg1 = _sentinel, arg2 = _sentinel,
           arg3 = _sentinel, arg4 = _sentinel]) {
     return guardAsync(() {
-      ++_actualCalls;
       if (!_shouldCallBack()) {
         return;
       } else if (arg0 == _sentinel) {
         return _callback();
       } else if (arg1 == _sentinel) {
         return _callback(arg0);
       } else if (arg2 == _sentinel) {
         return _callback(arg0, arg1);
       } else if (arg3 == _sentinel) {
         return _callback(arg0, arg1, arg2);
       } else if (arg4 == _sentinel) {
         return _callback(arg0, arg1, arg2, arg3);
       } else {
         _testCase.error(
            'unittest lib does not support callbacks with more than'
               ' 4 arguments.',
            '');
       }
     },
     _after, _testNum);
   }
 
   invoke0() {
     return guardAsync(
         () {
-          ++_actualCalls;
           if (_shouldCallBack()) {
             return _callback();
           }
         },
         _after, _testNum);
   }
 
   invoke1(arg1) {
     return guardAsync(
         () {
-          ++_actualCalls;
           if (_shouldCallBack()) {
             return _callback(arg1);
           }
         },
         _after, _testNum);
   }
 
   invoke2(arg1, arg2) {
     return guardAsync(
         () {
-          ++_actualCalls;
           if (_shouldCallBack()) {
             return _callback(arg1, arg2);
           }
         },
         _after, _testNum);
   }
-
-  /** Returns false if we exceded the number of expected calls. */
-  bool _checkCallCount() {
-    if (_actualCalls > _expectedCalls) {
-      _testCase.error('Callback ${_id}called more times than expected '
-             '($_actualCalls > $_expectedCalls).', '');
-      return false;
-    }
-    return true;
-  }
 }
 
 /**
  * Indicate that [callback] is expected to be called a [count] number of times
  * (by default 1). The unittest framework will wait for the callback to run the
  * specified [count] times before it continues with the following test.  Using
  * [_expectAsync] will also ensure that errors that occur within [callback] are
  * tracked and reported. [callback] should take between 0 and 4 positional
  * arguments (named arguments are not supported here). [id] can be used
  * to provide more descriptive error messages if the callback is called more
  * often than expected.
  */
-Function _expectAsync(Function callback, {int count: 1, String id}) {
-  return new _SpreadArgsHelper.
-      fixedCallCount(callback, count, id).invoke;
+Function _expectAsync(Function callback,
+                     {int count: 1, int max: 0, String id}) {
+  return new _SpreadArgsHelper(callback, count, max, null, id).invoke;
 }
 
 /**
  * Indicate that [callback] is expected to be called a [count] number of times
  * (by default 1). The unittest framework will wait for the callback to run the
  * specified [count] times before it continues with the following test.  Using
  * [expectAsync0] will also ensure that errors that occur within [callback] are
  * tracked and reported. [callback] should take 0 positional arguments (named
  * arguments are not supported). [id] can be used to provide more
  * descriptive error messages if the callback is called more often than
- * expected.
+ * expected. [max] can be used to specify an upper bound on the number of
+ * calls; if this is exceeded the test will fail (or be marked as in error if
+ * it was already complete). A value of 0 for [max] (the default) will set
+ * the upper bound to the same value as [count]; i.e. the callback should be
+ * called exactly [count] times. A value of -1 for [max] will mean no upper
+ * bound.
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsync0(Function callback, {int count: 1, String id}) {
-  return new _SpreadArgsHelper.
-      fixedCallCount(callback, count, id).invoke0;
+Function expectAsync0(Function callback,
+                     {int count: 1, int max: 0, String id}) {
+  return new _SpreadArgsHelper(callback, count, max, null, id).invoke0;
 }
 
 /** Like [expectAsync0] but [callback] should take 1 positional argument. */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsync1(Function callback, {int count: 1, String id}) {
-  return new _SpreadArgsHelper.
-      fixedCallCount(callback, count, id).invoke1;
+Function expectAsync1(Function callback,
+                     {int count: 1, int max: 0, String id}) {
+  return new _SpreadArgsHelper(callback, count, max, null, id).invoke1;
 }
 
 /** Like [expectAsync0] but [callback] should take 2 positional arguments. */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsync2(Function callback, {int count: 1, String id}) {
-  return new _SpreadArgsHelper.
-      fixedCallCount(callback, count, id).invoke2;
+Function expectAsync2(Function callback,
+                     {int count: 1, int max: 0, String id}) {
+  return new _SpreadArgsHelper(callback, count, max, null, id).invoke2;
 }
 
 /**
  * Indicate that [callback] is expected to be called until [isDone] returns
  * true. The unittest framework checks [isDone] after each callback and only
  * when it returns true will it continue with the following test. Using
  * [expectAsyncUntil] will also ensure that errors that occur within
  * [callback] are tracked and reported. [callback] should take between 0 and
- * 4 positional arguments (named arguments are not supported).
+ * 4 positional arguments (named arguments are not supported). [id] can be
+ * used to identify the callback in error messages (for example if it is called
+ * after the test case is complete).
  */
-Function _expectAsyncUntil(Function callback, Function isDone) {
-  return new _SpreadArgsHelper.variableCallCount(callback, isDone).invoke;
+Function _expectAsyncUntil(Function callback, Function isDone, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, isDone, id).invoke;

[Siggi Cherem (dart-lang), 2013/02/28 19:29:23]

strange - I was expecting that max was always >= 0.

[gram, 2013/02/28 22:49:50]

-1 means unlimited

 }
 
 /**
  * Indicate that [callback] is expected to be called until [isDone] returns
  * true. The unittest framework check [isDone] after each callback and only
  * when it returns true will it continue with the following test. Using
  * [expectAsyncUntil0] will also ensure that errors that occur within
  * [callback] are tracked and reported. [callback] should take 0 positional
- * arguments (named arguments are not supported).
+ * arguments (named arguments are not supported). [id] can be used to
+ * identify the callback in error messages (for example if it is called 
+ * after the test case is complete).
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsyncUntil0(Function callback, Function isDone) {
-  return new _SpreadArgsHelper.variableCallCount(callback, isDone).invoke0;
+Function expectAsyncUntil0(Function callback, Function isDone, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, isDone, id).invoke0;
 }
 
 /**
  * Like [expectAsyncUntil0] but [callback] should take 1 positional argument.
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsyncUntil1(Function callback, Function isDone) {
-  return new _SpreadArgsHelper.variableCallCount(callback, isDone).invoke1;
+Function expectAsyncUntil1(Function callback, Function isDone, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, isDone, id).invoke1;
 }
 
 /**
  * Like [expectAsyncUntil0] but [callback] should take 2 positional arguments.
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function expectAsyncUntil2(Function callback, Function isDone) {
-  return new _SpreadArgsHelper.variableCallCount(callback, isDone).invoke2;
+Function expectAsyncUntil2(Function callback, Function isDone, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, isDone, id).invoke2;
 }
 
 /**
  * Wraps the [callback] in a new function and returns that function. The new
  * function will be able to handle exceptions by directing them to the correct
  * test. This is thus similar to expectAsync0. Use it to wrap any callbacks that
  * might optionally be called but may never be called during the test.
  * [callback] should take between 0 and 4 positional arguments (named arguments
- * are not supported).
+ * are not supported). [id] can be used to identify the callback in error
+ * messages (for example if it is called after the test case is complete).
  */
-Function _protectAsync(Function callback) {
-  return new _SpreadArgsHelper.optionalCalls(callback).invoke;
+Function _protectAsync(Function callback, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, null, id).invoke;
 }
 
 /**
  * Wraps the [callback] in a new function and returns that function. The new
  * function will be able to handle exceptions by directing them to the correct
  * test. This is thus similar to expectAsync0. Use it to wrap any callbacks that
  * might optionally be called but may never be called during the test.
  * [callback] should take 0 positional arguments (named arguments are not
- * supported).
+ * supported). [id] can be used to identify the callback in error
+ * messages (for example if it is called after the test case is complete).
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function protectAsync0(Function callback) {
-  return new _SpreadArgsHelper.optionalCalls(callback).invoke0;
+Function protectAsync0(Function callback, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, null, id).invoke0;
 }
 
 /**
  * Like [protectAsync0] but [callback] should take 1 positional argument.
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function protectAsync1(Function callback) {
-  return new _SpreadArgsHelper.optionalCalls(callback).invoke1;
+Function protectAsync1(Function callback, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, null, id).invoke1;
 }
 
 /**
  * Like [protectAsync0] but [callback] should take 2 positional arguments.
  */
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
-Function protectAsync2(Function callback) {
-  return new _SpreadArgsHelper.optionalCalls(callback).invoke2;
+Function protectAsync2(Function callback, {String id}) {
+  return new _SpreadArgsHelper(callback, 0, -1, null, id).invoke2;
 }
 
 /**
  * Creates a new named group of tests. Calls to group() or test() within the
  * body of the function passed to this will inherit this group's description.
  */
 void group(String description, void body()) {
   ensureInitialized();
   // Concatenate the new group.
   final parentGroup = _currentGroup;
@@ skipping 42 matching lines @@
  * be called after each test in the group is run. Note that if groups
  * are nested only the most locally scoped [teardownTest] function will be run.
  * [setUp] and [tearDown] should be called within the [group] before any
  * calls to [test]. The [teardownTest] function can be asynchronous; in this
  * case it must return a [Future].
  */
 void tearDown(Function teardownTest) {
   _testTeardown = teardownTest;
 }
 
-/**
- * Called when one of the callback functions is done with all expected
- * calls.
- */
-void _handleCallbackFunctionComplete(testNum, [id = '']) {
-  // TODO (gram): we defer this to give the nextBatch recursive
-  // stack a chance to unwind. This is a temporary hack but
-  // really a bunch of code here needs to be fixed. We have a
-  // single array that is being iterated through by nextBatch(),
-  // which is recursively invoked in the case of async tests that
-  // run synchronously. Bad things can then happen.
-  _defer(() {
-    if (_currentTest != testNum) {
-      if (_tests[testNum].result == PASS) {
-        _tests[testNum].error("${id}Unexpected extra callbacks", '');
-      }
-    } else if (_currentTest < _tests.length) {
-      final testCase = _tests[_currentTest];
-      --testCase.callbackFunctionsOutstanding;
-      if (testCase.callbackFunctionsOutstanding < 0) {
-        // TODO(gram): Check: Can this even happen?
-        testCase.error(
-            'More calls to _handleCallbackFunctionComplete() than expected.',
-             '');
-      } else if (testCase.callbackFunctionsOutstanding == 0 &&
-                 !testCase.isComplete) {
-        testCase.pass();
-      }
-    }
-  });
-}
-
 /** Advance to the next test case. */
 void _nextTestCase() {
   _defer(() {
     _currentTest++;
     _testRunner();
   });
 }
 
 /**
- * Temporary hack: expose old API.
- * TODO(gram) remove this when WebKit tests are working with new framework
- */
-void callbackDone() {
-  _handleCallbackFunctionComplete(_currentTest);
-}
-
-/**
  * Utility function that can be used to notify the test framework that an
  *  error was caught outside of this library.
  */
 void _reportTestError(String msg, String trace) {
  if (_currentTest < _tests.length) {
     final testCase = _tests[_currentTest];
     testCase.error(msg, trace);
   } else {
     _uncaughtErrorMessage = "$msg: $trace";
   }
@@ skipping 73 matching lines @@
  */
 registerException(e, [trace]) {
   _registerException(_currentTest, e, trace);
 }
 
 /**
  * Registers that an exception was caught for the current test.
  */
 _registerException(testNum, e, [trace]) {
   trace = trace == null ? '' : trace.toString();
+  String message = (e is TestFailure) ? e.message : 'Caught $e';
   if (_tests[testNum].result == null) {
-    String message = (e is TestFailure) ? e.message : 'Caught $e';
     _tests[testNum].fail(message, trace);
   } else {
-    _tests[testNum].error('Caught $e', trace);
+    _tests[testNum].error(message, trace);
   }
 }
 
 /**
  * Runs a batch of tests, yielding whenever an asynchronous test starts
  * running. Tests will resume executing when such asynchronous test calls
  * [done] or if it fails with an exception.
  */
 _nextBatch() {
   while (true) {
@@ skipping 91 matching lines @@
 }
 
 /** Enable a test by ID. */
 void enableTest(int testId) => _setTestEnabledState(testId, true);
 
 /** Disable a test by ID. */
 void disableTest(int testId) => _setTestEnabledState(testId, false);
 
 /** Signature for a test function. */
 typedef dynamic TestFunction();