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;
+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. */
 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
@@ skipping 55 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) {
+  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;
 }
 
 /**
  * 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) {
+  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;
 }
 
 /** 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) {

[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";
   }
 }
 
 /**
  * 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() {

[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();
 }
 
 /**
  * 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;
+  _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);
   } 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);
+  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 = '';
 
   // 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]) {

[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);
   }
 }
 
 /**
  * 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);
+    var f = guardAsync(currentTestCase._run);
     if (f != null) {
       f.whenComplete(() {
         _nextTestCase(); // Schedule the next test.
       });
       break;
     }
     _currentTestCaseIndex++;
   }
 }
 
 /** Publish results on the page and notify controller. */
 void _completeTests() {
   if (!_initialized) return;
   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;
+
+  _currentTestCaseIndex = null;
+  _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 _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;
   }
   _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();