unittest: big cleanup, tightened test semantics

pkg/unittest/lib/unittest.dart

Index: pkg/unittest/lib/unittest.dart
diff --git a/pkg/unittest/lib/unittest.dart b/pkg/unittest/lib/unittest.dart
index efe6f8d40f7535b34e12fe33d791b47535a7ab53..82aa4d92756e9c22f0d8f034ce415b07f1e306b3 100644
--- a/pkg/unittest/lib/unittest.dart
+++ b/pkg/unittest/lib/unittest.dart
@@ -213,13 +213,10 @@ 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;
@@ -294,10 +291,9 @@ class _SpreadArgsHelper {
   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();
 
@@ -309,19 +305,13 @@ class _SpreadArgsHelper {
             ? 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;
@@ -405,7 +395,7 @@ class _SpreadArgsHelper {
            '');
       }
     },
-    after, testNum);
+    after, testCase);
   }
 
   invoke0() {
@@ -415,7 +405,7 @@ class _SpreadArgsHelper {
             return callback();
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke1(arg1) {
@@ -425,7 +415,7 @@ class _SpreadArgsHelper {
             return callback(arg1);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke2(arg1, arg2) {
@@ -435,7 +425,7 @@ class _SpreadArgsHelper {
             return callback(arg1, arg2);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 }
 
@@ -443,21 +433,6 @@ class _SpreadArgsHelper {
  * 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
@@ -491,20 +466,6 @@ Function expectAsync2(Function callback,
 
 /**
  * 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
@@ -539,19 +500,6 @@ Function expectAsyncUntil2(Function callback, Function isDone, {String id}) {
  * 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).
@@ -622,6 +570,7 @@ void group(String description, void body()) {
  * 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;
 }
 
@@ -634,25 +583,17 @@ void setUp(Function setupTest) {
  * 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";
   }
@@ -672,8 +613,7 @@ _defer(void 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();
 }
 
@@ -683,7 +623,8 @@ void rerunTests() {
  * 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);
@@ -698,7 +639,8 @@ void filterTests(testFilter) {
 /** 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.
@@ -720,15 +662,15 @@ void runTests() {
  * 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();
   }
@@ -738,48 +680,43 @@ _guardAsync(Function tryBody, Function finallyBody, int testNum) {
  * 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;
-    }
-    final testCase = _testCases[_currentTestCaseIndex];
-    var f = _guardAsync(testCase._run, null, _currentTestCaseIndex);
-    if (f != null) {
-      f.whenComplete(() {
-        _nextTestCase(); // Schedule the next test.
-      });
-      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.

     }
-    _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;
@@ -794,7 +731,8 @@ void _completeTests() {
   _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) {
@@ -809,7 +747,15 @@ 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;
   }