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..3236b67f344d90a45915acda5d6eb04f6e6016f6 100644
--- a/pkg/unittest/lib/unittest.dart
+++ b/pkg/unittest/lib/unittest.dart
@@ -213,12 +213,13 @@ Function _testSetup;
 /** Teardown function called after each test in a group */
 Function _testTeardown;
 
-int _currentTestCaseIndex = 0;
+int _currentTestCaseIndex = null;
 
 /** [TestCase] currently being executed. */
 TestCase get currentTestCase =>
-    (_currentTestCaseIndex >= 0 && _currentTestCaseIndex < _testCases.length)
-        ? _testCases[_currentTestCaseIndex]
+    (_currentTestCaseIndex != null && _currentTestCaseIndex >= 0 &&
+      _currentTestCaseIndex < _testCases.length)
+      ? _testCases[_currentTestCaseIndex]
         : null;
 
 /** Whether the framework is in an initialized state. */
@@ -294,10 +295,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 +309,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 +399,7 @@ class _SpreadArgsHelper {
            '');
       }
     },
-    after, testNum);
+    after, testCase);
   }
 
   invoke0() {
@@ -415,7 +409,7 @@ class _SpreadArgsHelper {
             return callback();
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke1(arg1) {
@@ -425,7 +419,7 @@ class _SpreadArgsHelper {
             return callback(arg1);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 
   invoke2(arg1, arg2) {
@@ -435,7 +429,7 @@ class _SpreadArgsHelper {
             return callback(arg1, arg2);
           }
         },
-        after, testNum);
+        after, testCase);
   }
 }
 
@@ -443,21 +437,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 +470,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 +504,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 +574,7 @@ void group(String description, void body()) {
  * case it must return a [Future].
  */
 void setUp(Function setupTest) {
+  ensureInitialized();

[kevmoo-old, 2013/04/06 17:56:34]

broke folks who set a custom config after this...which seems a bit broken, but
not going to fight that fight now

[Andrei Mouravski, 2013/04/06 22:41:53]

Whom did you break? If someone is doing something weird, it's worth just
patching that code, too.

(Perhaps place a TODO here and submit a second CL fixing everyone else's code.)

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

Since I *am* changing the semantics of unittest late in the game, I not trying
to fight every fight. I hit most issues in the scheduled_test tests.

   _testSetup = setupTest;
 }
 
@@ -634,6 +587,7 @@ void setUp(Function setupTest) {
  * case it must return a [Future].
  */
 void tearDown(Function teardownTest) {
+  ensureInitialized();

[kevmoo-old, 2013/04/06 17:56:34]

broke folks who set a custom config after this...which seems a bit broken, but
not going to fight that fight now

   _testTeardown = teardownTest;
 }
 
@@ -650,9 +604,8 @@ void _nextTestCase() {
  *  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) {

[Andrei Mouravski, 2013/04/06 22:41:53]

How about a private getter:
bool get _isCurrentTestCaseValid => _currentTestCaseIndex != null &&
_currentTestCaseIndex >= 0 && _currentTestCaseIndex < _testCases.length;

[kevmoo-old, 2013/04/06 23:00:40]

I'm confused. The latest commit eliminated the _currentTestCaseIndex entirely.
Explain...

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

_currentTestCaseIndex has been internalized into _nextBatch

This cleans up a lot of impl. Code no longer has to key off the index, instead
_currentTestCase is 'truth'. Null if a test is not running, otherwise the
instance of the running test.

+   currentTestCase.error(msg, trace);
   } else {
     _uncaughtErrorMessage = "$msg: $trace";
   }
@@ -672,8 +625,7 @@ _defer(void callback()) {
 }
 
 void rerunTests() {

[Andrei Mouravski, 2013/04/06 22:41:53]

Why do we even have this? What makes this different from runTests?

If there's a difference, please document it.

[kevmoo-old, 2013/04/06 23:00:40]

It's legacy. It's used by interactive html configuration.

I didn't want to rock the boat more in this CL.
More docs: good.
Pondering removal: good.

Just trying to keep the scope of this CL smaller. It's on my radar. :-)

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

On your radar: good.
Add a TODO: better.
File a bug to track: best.

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

It's being used. I've verified the behavior. We can discuss other work if there
is an interest. My goal here was to tighten up the logic a bit, be a bit more
strict about usage patterns, and level everything else working.

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

We could eliminate this now. Originally there was a distinction IIRC (rerunTests
was meant to not clobber an existing test result if the test was disabled) but
now it seems superfluous.

-  _uncaughtErrorMessage = null;
-  _initialized = true; // We don't want to reset the test array.
+  assert(_uncaughtErrorMessage == null);

[Andrei Mouravski, 2013/04/06 22:41:53]

Can you make a common "reset()" method somewhere? It seems as if things are set
to null/default values in many places, which is just asking for bugs.

ensureInitialized, completeTests, and even the top level. These need to all have
the same implementation or else bad things may happen. Asserts are quite, good,
though!

[kevmoo-old, 2013/04/06 23:00:40]

Agreed. Things for us to discuss on Tuesday.

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

I don't think we need to discuss. This is just something that really should be
in here. Again, a TODO and file a bug. (Reference bug # in the TODO like:
// TODO(kevmoo): Blah blah. Issue #12345)

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

I'm not going to modify the current semantic further, as discussed. The existing
behavior works fine for code that uses this method.

If we think the asserts are too much, I'm happy to remove.

   runTests();
 }
 
@@ -683,7 +635,8 @@ void rerunTests() {
  * in that it removes the tests completely.
  */
 void filterTests(testFilter) {
-  var filterFunction;
+  _ensureInitialized(false);

[kevmoo-old, 2013/04/06 17:56:34]

broke folks who set a custom config after this...which seems a bit broken, but
not going to fight that fight now

+  Function filterFunction;
   if (testFilter is String) {
     RegExp re = new RegExp(testFilter);
     filterFunction = (t) => re.hasMatch(t.description);
@@ -698,6 +651,8 @@ void filterTests(testFilter) {
 /** Runs all queued tests, one at a time. */
 void runTests() {
   _ensureInitialized(false);
+  assert(_currentTestCaseIndex == null);

[Andrei Mouravski, 2013/04/06 22:41:53]

If this assert is a post-condition on _ensureIntialized, then it should probably
go into _ensureIntialized. Otherwise, what is this assert here for?

[kevmoo-old, 2013/04/06 23:00:40]

See latest update. This is now an assert around _currentTestCase == null.

_ensureItitilaized actually throws if there is a current test case, so this
could be removed.

...just my personal paranoid default

+
   _currentTestCaseIndex = 0;
   _currentGroup = '';
 
@@ -720,15 +675,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,19 +693,20 @@ _guardAsync(Function tryBody, Function finallyBody, int testNum) {
  * Registers that an exception was caught for the current test.
  */
 void registerException(e, [trace]) {

[Andrei Mouravski, 2013/04/06 22:41:53]

Is there a good reason that this is top-level?

[kevmoo-old, 2013/04/06 23:00:40]

It was used by layout unit test random, but that's gone now.

Should consider removing it, me thinks.

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

Consider removing it? Or actually remove it. ;]

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

scheduled_test uses this

-  _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);
   }
 }
 
@@ -765,8 +721,7 @@ void _nextBatch() {
       _completeTests();
       break;
     }
-    final testCase = _testCases[_currentTestCaseIndex];
-    var f = _guardAsync(testCase._run, null, _currentTestCaseIndex);
+    var f = guardAsync(currentTestCase._run);
     if (f != null) {
       f.whenComplete(() {
         _nextTestCase(); // Schedule the next test.
@@ -794,7 +749,9 @@ void _completeTests() {
   _config.onSummary(passed, failed, errors, testCases, _uncaughtErrorMessage);
   _config.onDone(passed > 0 && failed == 0 && errors == 0 &&
       _uncaughtErrorMessage == null);
-  _initialized = false;
+
+  _currentTestCaseIndex = null;
+  _uncaughtErrorMessage = null;
 }
 
 String _fullSpec(String spec) {
@@ -810,6 +767,10 @@ void ensureInitialized() {
 }
 
 void _ensureInitialized(bool configAutoStart) {
+  if(_currentTestCaseIndex != null) {
+    throw new StateError("A forbidden operation occured "

[Andrei Mouravski, 2013/04/06 22:41:53]

I feel like this error could be useful at other places where you check nullness
of the testcase.

[kevmoo-old, 2013/04/06 23:00:40]

See latest update.

+        "while tests were running");
+  }
   if (_initialized) {
     return;
   }