unittest: big cleanup, tightened test semantics

pkg/unittest/lib/unittest.dart

 // Copyright (c) 2013, 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, install the
  * [unittest package](http://pub.dartlang.org/packages/unittest) via the pub
  * package manager. See the [Getting Started](http://pub.dartlang.org/doc)
@@ skipping 195 matching lines @@
 
 /** Get the list of tests. */
 final List<TestCase> testCases = new UnmodifiableListView(_testCases);
 
 /** Setup function called before each test in a group */
 Function _testSetup;
 
 /** Teardown function called after each test in a group */
 Function _testTeardown;
 
-int _currentTestCaseIndex = 0;
+TestCase _currentTestCase = null;
 
 /** [TestCase] currently being executed. */
-TestCase get currentTestCase =>
-    (_currentTestCaseIndex >= 0 && _currentTestCaseIndex < _testCases.length)
-        ? _testCases[_currentTestCaseIndex]
-        : null;
+TestCase get currentTestCase => _currentTestCase;
 
 /** Whether the framework is in an initialized state. */
 bool _initialized = false;
 
 String _uncaughtErrorMessage = null;
 
 /** Test case result strings. */
 // TODO(gram) we should change these constants to use a different string
 // (so that writing 'FAIL' in the middle of a test doesn't
 // imply that the test fails). We can't do it without also changing
@@ skipping 54 matching lines @@
 }
 
 /** Simulates spread arguments using named arguments. */
 // TODO(sigmund): remove this class and simply use a closure with named
 // arguments (if still applicable).
 class _SpreadArgsHelper {
   final Function callback;
   final int minExpectedCalls;
   final int maxExpectedCalls;
   final Function isDone;
-  final int testNum;
+  final TestCase testCase;
   final String id;
   int actualCalls = 0;
-  TestCase testCase;
   bool complete;
   static const sentinel = const _Sentinel();
 
   _SpreadArgsHelper(Function callback, int minExpected, int maxExpected,
       Function isDone, String id)
       : this.callback = callback,
         minExpectedCalls = minExpected,
         maxExpectedCalls = (maxExpected == 0 && minExpected > 0)
             ? minExpected
             : maxExpected,
         this.isDone = isDone,
-        testNum = _currentTestCaseIndex,
+        this.testCase = currentTestCase,
         this.id = _makeCallbackId(id, callback) {
-    ensureInitialized();
-    if (!(_currentTestCaseIndex >= 0 &&
-           _currentTestCaseIndex < _testCases.length &&
-           _testCases[_currentTestCaseIndex] != null)) {
-      print("No valid test, did you forget to run your test inside a call "
-          "to test()?");
+    if(testCase == null) {
+      throw new StateError("No valid test, did you forget to run your test "
+          "inside a call to test()?");
     }
-    assert(_currentTestCaseIndex >= 0 &&
-           _currentTestCaseIndex < _testCases.length &&
-           _testCases[_currentTestCaseIndex] != null);
-    testCase = _testCases[_currentTestCaseIndex];
+
     if (isDone != null || minExpected > 0) {
       testCase._callbackFunctionsOutstanding++;
       complete = false;
     } else {
       complete = true;
     }
   }
 
   static _makeCallbackId(String id, Function callback) {
     // Try to create a reasonable id.
@@ skipping 63 matching lines @@
         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);
+    after, testCase);
   }
 
   invoke0() {
     return _guardAsync(
         () {
           if (shouldCallBack()) {
             return callback();
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke1(arg1) {
     return _guardAsync(
         () {
           if (shouldCallBack()) {
             return callback(arg1);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke2(arg1, arg2) {
     return _guardAsync(
         () {
           if (shouldCallBack()) {
             return callback(arg1, arg2);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 }
 
 /**
  * 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, 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. [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.
@@ skipping 13 matching lines @@
 
 /** 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, 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). [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, {String id}) {
-  return new _SpreadArgsHelper(callback, 0, -1, isDone, id).invoke;
-}
-
-/**
- * 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). [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, {String id}) {
@@ skipping 14 matching lines @@
 // TODO(sigmund): deprecate this API when issue 2706 is fixed.
 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). [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, {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). [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, {String id}) {
   return new _SpreadArgsHelper(callback, 0, -1, null, id).invoke0;
 }
 
 /**
@@ skipping 50 matching lines @@
 
 /**
  * Register a [setUp] function for a test [group]. This function will
  * be called before each test in the group is run. Note that if groups
  * are nested only the most locally scoped [setUpTest] function will be run.
  * [setUp] and [tearDown] should be called within the [group] before any
  * calls to [test]. The [setupTest] function can be asynchronous; in this
  * case it must return a [Future].
  */
 void setUp(Function setupTest) {
+  _requireNotRunning();

[kevmoo-old, 2013/04/06 23:10:03]

Should be a call to ensureInitialized(), but pub tests are doing setUp and then
trying to set a custom config.

Need to add a TODO here to fix that impl and then make this as strict as test()
and group()

   _testSetup = setupTest;
 }
 
 /**
  * Register a [tearDown] function for a test [group]. This function will
  * 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) {
+  _requireNotRunning();

[kevmoo-old, 2013/04/06 23:10:03]

Should be a call to ensureInitialized(), but pub tests are doing setUp and then
trying to set a custom config.

Need to add a TODO here to fix that impl and then make this as strict as test()
and group()

   _testTeardown = teardownTest;
 }
 
-/** Advance to the next test case. */
-void _nextTestCase() {
-  _defer(() {
-    _currentTestCaseIndex++;
-    _nextBatch();
-  });
-}
-
 /**
  * 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 (_currentTestCaseIndex < _testCases.length) {
-    final testCase = _testCases[_currentTestCaseIndex];
-    testCase.error(msg, trace);
+ if (currentTestCase != null) {
+   currentTestCase.error(msg, trace);
   } else {
     _uncaughtErrorMessage = "$msg: $trace";
   }
 }
 
 /**
  * Runs [callback] at the end of the event loop. Note that we don't wrap
  * the callback in guardAsync; this is for test framework functions which
  * should not be throwing unexpected exceptions that end up failing test
  * cases! Furthermore, we need the final exception to be thrown but not
  * caught by the test framework if any test cases failed. However, tests
  * that make use of a similar defer function *should* wrap the callback
  * (as we do in unitttest_test.dart).
  */
 _defer(void callback()) {
   (new Future.immediate(null)).then((_) => callback());
 }
 
 void rerunTests() {
-  _uncaughtErrorMessage = null;
-  _initialized = true; // We don't want to reset the test array.
+  assert(_uncaughtErrorMessage == null);

[kevmoo-old, 2013/04/06 23:10:03]

Need a TODO about getting rid of this. Perhaps we just need 'run' with the right
checks to make sure everything has finished.

   runTests();
 }
 
 /**
  * Filter the tests. [testFilter] can be a [RegExp], a [String] or a
  * predicate function. This is different to enabling/disabling tests
  * in that it removes the tests completely.
  */
 void filterTests(testFilter) {
-  var filterFunction;
+  _requireNotRunning();

[kevmoo-old, 2013/04/06 23:10:03]

Should be a call to ensureInitialized(), but pub tests are doing setUp and then
trying to set a custom config.

Need to add a TODO here to fix that impl and then make this as strict as test()
and group()

+  Function filterFunction;
   if (testFilter is String) {
     RegExp re = new RegExp(testFilter);
     filterFunction = (t) => re.hasMatch(t.description);
   } else if (testFilter is RegExp) {
     filterFunction = (t) => testFilter.hasMatch(t.description);
   } else if (testFilter is Function) {
     filterFunction = testFilter;
   }
   _testCases.retainWhere(filterFunction);
 }
 
 /** Runs all queued tests, one at a time. */
 void runTests() {
   _ensureInitialized(false);
-  _currentTestCaseIndex = 0;
+  assert(_currentTestCase == null);

[kevmoo-old, 2013/04/06 23:10:03]

Overly paranoid. _ensureInitialized() throws if this is not the case. Should
probably be removed.

+
   _currentGroup = '';
 
   // If we are soloing a test, remove all the others.
   if (_soloTest != null) {
     filterTests((t) => t == _soloTest);
   }
 
   _config.onStart();
 
   _defer(() {
     _nextBatch();
   });
 }
 
 /**
  * Run [tryBody] guarded in a try-catch block. If an exception is thrown, it is
  * passed to the corresponding test.
  *
  * The value returned by [tryBody] (if any) is returned by [guardAsync].
  */
 guardAsync(Function tryBody) {
-  return _guardAsync(tryBody, null, _currentTestCaseIndex);
+  return _guardAsync(tryBody, null, currentTestCase);
 }
 
-_guardAsync(Function tryBody, Function finallyBody, int testNum) {
-  assert(testNum >= 0);
+_guardAsync(Function tryBody, Function finallyBody, TestCase testCase) {
+  assert(testCase != null);
   try {
     return tryBody();
   } catch (e, trace) {
-    _registerException(testNum, e, trace);
+    _registerException(testCase, e, trace);
   } finally {
     if (finallyBody != null) finallyBody();
   }
 }
 
 /**
  * Registers that an exception was caught for the current test.
  */
 void registerException(e, [trace]) {
-  _registerException(_currentTestCaseIndex, e, trace);
+  _registerException(currentTestCase, e, trace);
 }
 
 /**
  * Registers that an exception was caught for the current test.
  */
-void _registerException(testNum, e, [trace]) {
+void _registerException(TestCase testCase, e, [trace]) {
+  assert(testCase != null);
   trace = trace == null ? '' : trace.toString();
   String message = (e is TestFailure) ? e.message : 'Caught $e';
-  if (_testCases[testNum].result == null) {
-    _testCases[testNum].fail(message, trace);
+  if (testCase.result == null) {
+    testCase.fail(message, trace);
   } else {
-    _testCases[testNum].error(message, trace);
+    testCase.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.
- */
-void _nextBatch() {
-  while (true) {
-    if (_currentTestCaseIndex >= _testCases.length) {
-      _completeTests();
-      break;
+// TODO(kevmoo) we could imagine runTests returning a future...

[Andrei Mouravski, 2013/04/07 19:15:54]

Re-add some doc comments here.

Also, this TODO doesn't specify what there is to actually do.

[kevmoo-old, 2013/04/18 16:13:55]

More doc comments are coming in an update. Should explain the flow.

+Future _nextBatch([int index = 0]) {

[kevmoo-old, 2013/04/06 23:10:03]

FYI: I looked into using Future.forEach here, but it increases the test time for
non-async tests by...a lot.

This code is inspired by Future.forEach but:

1) uses indexes, since we have a list
2) doesn't spin up unneeded Futures for cases where the test run is synchronous

[Andrei Mouravski, 2013/04/07 19:15:54]

If the Future.forEach has poor performance, please file a bug. That's worth
fixing.

Also, it's worth noting what happens if your sync tests are reaaaaally slow.
Perhaps using isolates would speed up performance in all cases.

+  for(int i = index; i < _testCases.length; i++) {

[Andrei Mouravski, 2013/04/07 19:15:54]

I'm not entirely clear on what this function is doing, and why it is doing it
this way.

Firstly, it's not entirely clear what a batch of tests means.

Secondly, it's not clear how the program is actually structured. I call
nextBatch with an index? But then it can span a future with a call to batch
again, but only after the future is done? Where is the future returned by this
method actually completed?

---

The way unittest.dart is structured makes it very difficult to understand the
program flow. What I really want is:
1) initialize test runner
2) for each test, spawn a future that completes to nothing, or error if test
failed
3) wait till futures are all done (Future.wait), then display results.

I can't really see that from what we have. :[

[gram, 2013/04/18 20:46:08]

The problem with that is you now have unpredictable ordering of execution.
Currently each test must complete before the next starts. There are, I expect,
many cases where this is necessary. Running each test in an isolate would help
but would probably break some tests. So while I agree that your proposal would
be nice, it seems likely that it would break a lot of existing tests. This
brings me back to my separate suggestion that it would be better to create a
"unit test v2" library rather than radically change this one.

+    _currentTestCase = _testCases[i];
+
+    Future f = guardAsync(_currentTestCase._run);
+
+    if(f != null) {
+      return f.then((_) => _nextBatch(i + 1));

[Andrei Mouravski, 2013/04/07 19:15:54]

Should probably be whenComplete

[kevmoo-old, 2013/04/18 16:13:55]

guardAsync should never fail. If it returns a future, it should never fail.

If it does, whenComplete would swallow the error, or complete with the error,
but after running the nested future.

Better to fail immediately in this rare case.

     }
-    final testCase = _testCases[_currentTestCaseIndex];
-    var f = _guardAsync(testCase._run, null, _currentTestCaseIndex);
-    if (f != null) {
-      f.whenComplete(() {
-        _nextTestCase(); // Schedule the next test.
-      });
-      break;
-    }
-    _currentTestCaseIndex++;
   }
+  _currentTestCase = null;
+  return new Future.of(_completeTests);
 }
 
 /** Publish results on the page and notify controller. */
 void _completeTests() {
-  if (!_initialized) return;
+  assert(_initialized);

[Andrei Mouravski, 2013/04/07 19:15:54]

Now that I think of it, it's kind of weird to have these asserts. You're
presumably running your tests in checked mode, but if unittest.dart itself fails
an assert (only in checked mode) then you get ZERO information on your tests.

I almost want a way to say that my tests run in checked mode, but the unittest
runner does not (but can, if you want it to.)

+  assert(_currentTestCase == null);
+
   int passed = 0;
   int failed = 0;
   int errors = 0;
 
   for (TestCase t in _testCases) {
     switch (t.result) {
       case PASS:  passed++; break;
       case FAIL:  failed++; break;
       case ERROR: errors++; break;
     }
   }
   _config.onSummary(passed, failed, errors, testCases, _uncaughtErrorMessage);
   _config.onDone(passed > 0 && failed == 0 && errors == 0 &&
       _uncaughtErrorMessage == null);
-  _initialized = false;
+
+  _uncaughtErrorMessage = null;
 }
 
 String _fullSpec(String spec) {
   if (spec == null) return '$_currentGroup';
   return _currentGroup != '' ? '$_currentGroup$groupSep$spec' : spec;
 }
 
 /**
  * Lazily initializes the test library if not already initialized.
  */
 void ensureInitialized() {
   _ensureInitialized(true);
 }
 
+void _requireNotRunning() {

[kevmoo-old, 2013/04/06 23:10:03]

Once everything has moved to ensureInitialized() ...meaning pub tests have been
fixed, then this can be rolled into ensureInit...

+  if(_currentTestCase != null) {
+    throw new StateError("A forbidden operation occured "

[kevmoo-old, 2013/04/06 23:10:03]

I've pondered allowing a string to be passed in here so it's clear which
forbidden operation caused this: test(), group() [others in the future].

Thoughts?

[Andrei Mouravski, 2013/04/07 19:15:54]

See my comment about asserts. I sort of want meta-unittest diagnostics to be
separate from my tests themselves.

That is to say, I would rather some of my tests are marked as invalid if the
syntax is wrong, or if the tests were incorrectly written (say if config was run
at the wrong time) as opposed to getting a big error at some weird time because
of an assert or a state error.

I definitely think that unittest.dart is something that's complicated enough and
easy to get wrong that we want to make the experience for people writing tests
to be as helpful as possible. Instead of throwing errors, I think throwing
"UnittestStateException" or something would be useful, and then the running at a
higher level can report to the user where they screwed up.

---

In short: yes, it should be noted which operation caused the problem.

+        "while tests were running");
+  }
+}
+
 void _ensureInitialized(bool configAutoStart) {
+  _requireNotRunning();
   if (_initialized) {
     return;
   }
   _initialized = true;
   // Hook our async guard into the matcher library.
   wrapAsync = (f, [id]) => expectAsync1(f, id: id);
 
   _uncaughtErrorMessage = null;
 
   if (_config == null) {
@@ skipping 34 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();