Sharing metatest logic between unittest and scheduled_test

pkg/metatest/lib/metatest.dart

Index: pkg/metatest/lib/metatest.dart
diff --git a/pkg/scheduled_test/test/metatest.dart b/pkg/metatest/lib/metatest.dart
similarity index 64%
rename from pkg/scheduled_test/test/metatest.dart
rename to pkg/metatest/lib/metatest.dart
index 3c0cd9acb39f8189e56dcc284fac685d4ddd79f9..1e8b07a5c528d6fbcc06fe66b47c0bb07e2b6459 100644
--- a/pkg/scheduled_test/test/metatest.dart
+++ b/pkg/metatest/lib/metatest.dart
@@ -1,4 +1,4 @@
-// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2014, 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.
 
@@ -10,14 +10,77 @@
 library metatest;
 
 import 'dart:async';
-import 'dart:io';
 import 'dart:isolate';
 
-import 'package:path/path.dart' as path;
 import 'package:unittest/unittest.dart';
-import 'package:scheduled_test/scheduled_test.dart' as scheduled_test;
 
-import 'utils.dart';
+import 'src/util.dart';

[nweiz, 2014/09/10 21:04:42]

Nit: We use "utils", not "util".

[kevmoo, 2014/09/17 21:16:29]

Done.

+
+/// Whether or not we're running in a child isolate that's supposed to run a
+/// test.
+bool _inChildIsolate;
+
+/// The port with which the child isolate should communicate with the parent
+/// isolate.
+///
+/// `null` in the parent isolate.
+SendPort _replyTo;
+
+/// The only value of the configuration used in metatest.
+final _singleton = new _MetaConfiguration();

[nweiz, 2014/09/10 21:04:42]

"singleton" isn't a good name for this: it only communicates that there's only
one instance of this variable, not what the variable actually is.
"metaConfiguration" would be a better name.

Alternately, keep it "_singleton" and make it a static variable of
"_MetaConfiguration" that's set/accessed in a factory constructor.

[kevmoo, 2014/09/17 21:16:28]

Done.

+
+/// The function holding the tests to be run.
+Function _testBody;
+
+/// The description of the test to run in the child isolate.
+///
+/// `null` in the parent isolate.
+String _testToRun;
+
+/// Stores the optional timeout used to override the default unittest timeout.
+Duration _timeoutOverride;
+
+/// Runs [setUpFn] before every metatest.
+///
+/// Note that [setUpFn] will be overwritten if the test itself calls [setUp].
+void metaSetUp(void setUpFn()) {
+  if (_inChildIsolate) setUp(setUpFn);
+}
+
+/// Runs a set of tests defined in `body` and checks the result by comparing
+/// with values in `expectedResults`.

[nweiz, 2014/09/10 21:04:42]

This still doesn't explain what the format of [expectedResults] is.

[kevmoo, 2014/09/17 21:16:28]

Done.

+void expectTestResults(String description, void body(),
+    List<Map> expectedResults) {
+  _setUpTest(description, body, (resultsMap) {
+    var list = resultsMap['results'] as List;

[nweiz, 2014/09/10 21:04:41]

This still uses "as".

[kevmoo, 2014/09/17 21:16:29]

Done.

+    expect(list, hasLength(expectedResults.length));

[nweiz, 2014/09/10 21:04:42]

These expect calls still don't have descriptions.

[kevmoo, 2014/09/17 21:16:28]

Done.

+
+    for (var i = 0; i < list.length; i++) {
+      var expectedMap = expectedResults[i];
+      var map = list[i];
+
+      expectedMap.forEach((key, value) {
+        expect(map, containsPair(key, value));
+      });

[nweiz, 2014/09/10 21:04:42]

Can't you just do expect(map, equals(expectedMap))?

[kevmoo, 2014/09/17 21:16:29]

I want to match a subset. There may be more in 'map' than 'expectedMap' which is
fine.

expectedMap does not need to match everything in map

+    }
+  });
+}
+
+/// Runs a test defined in `body` and validates that `result` and `message`
+/// match the values generated by the finished test.

[nweiz, 2014/09/10 21:04:42]

What are the semantics of "result" and "message"?

[kevmoo, 2014/09/17 21:16:29]

Done.

+void expectSingleTest(String description, String result, String message,
+    void body()) {
+  _setUpTest(description, body, (results) {
+    var testResults = results['results'];
+    expect(testResults, hasLength(1), reason: 'Only one test should be run.');
+
+    var testResult = testResults.single;
+    expect(testResult['result'], result,
+        reason: 'The test result did not meet expectations.');

[nweiz, 2014/09/10 21:04:42]

"Unexpected test result."

[kevmoo, 2014/09/17 21:16:29]

Done.

+    expect(testResult['message'], message,
+        reason: 'The test message did not meet expectations.');

[nweiz, 2014/09/10 21:04:42]

"Unexpected test message."

[kevmoo, 2014/09/17 21:16:29]

Done.

+  });
+}
 
 /// Declares a test with the given [description] and [body]. [body] corresponds
 /// to the `main` method of a test file, and will be run in an isolate. By
@@ -26,12 +89,12 @@ import 'utils.dart';
 void expectTestsPass(String description, void body(), {List<String> passing}) {
   _setUpTest(description, body, (results) {
     if (_hasError(results)) {
-      throw 'Expected all tests to pass, but got error(s):\n'
-          '${_summarizeTests(results)}';
+      fail('Expected all tests to pass, but got error(s):\n'
+          '${_summarizeTests(results)}');
     } else if (passing == null) {
       if (results['failed'] != 0) {
-        throw 'Expected all tests to pass, but some failed:\n'
-            '${_summarizeTests(results)}';
+        fail('Expected all tests to pass, but some failed:\n'
+            '${_summarizeTests(results)}');
       }
     } else {
       var shouldPass = new Set.from(passing);
@@ -42,48 +105,15 @@ void expectTestsPass(String description, void body(), {List<String> passing}) {
       if (!shouldPass.containsAll(didPass) ||
           !didPass.containsAll(shouldPass)) {
         String stringify(Set<String> tests) =>
-          '{${tests.map((t) => '"$t"').join(', ')}}';
+            '{${tests.map((t) => '"$t"').join(', ')}}';
 
         fail('Expected exactly ${stringify(shouldPass)} to pass, but '
-            '${stringify(didPass)} passed.\n'
-            '${_summarizeTests(results)}');
+            '${stringify(didPass)} passed.\n' '${_summarizeTests(results)}');

[nweiz, 2014/09/10 21:04:41]

There's still gratuitous reformatting here.

[kevmoo, 2014/09/17 21:16:29]

Done, I think...

       }
     }
   });
 }
 
-/// Declares a test with the given [description] and [body].
-///
-/// [body] corresponds
-/// to the `main` method of a test file, and will be run in an isolate.
-///
-/// All tests must have an expected result in [expectedResults].
-void expectTests(String description, void body(),
-                 Map<String, String> expectedResults) {
-  _setUpTest(description, body, (results) {
-    expectedResults = new Map.from(expectedResults);
-
-    for (var testResult in results['results']) {
-      var description = testResult['description'];
-
-      expect(expectedResults, contains(description),
-          reason: '"$description" did not have an expected result set.\n'
-            '${_summarizeTests(results)}');
-
-      var result = testResult['result'];
-
-      expect(expectedResults, containsPair(description, result),
-          reason: 'The test "$description" not not have the expected result.\n'
-            '${_summarizeTests(results)}');
-
-      expectedResults.remove(description);
-    }
-    expect(expectedResults, isEmpty,
-        reason: 'Unexpected additional test results\n'
-            '${_summarizeTests(results)}');
-  });
-}
-
 /// Declares a test with the given [description] and [body]. [body] corresponds
 /// to the `main` method of a test file, and will be run in an isolate. Expects
 /// all tests defined by [body] to fail.
@@ -99,45 +129,27 @@ void expectTestsFail(String description, void body()) {
   });
 }
 
-/// Runs [setUpFn] before every metatest. Note that [setUpFn] will be
-/// overwritten if the test itself calls [setUp].
-void metaSetUp(void setUpFn()) {
-  if (_inChildIsolate) scheduled_test.setUp(setUpFn);
-}
-
 /// Sets up a test with the given [description] and [body]. After the test runs,
 /// calls [validate] with the result map.
-void _setUpTest(String description, void body(), void validate(Map)) {
+void _setUpTest(String description, void body(), void validate(Map map)) {
   if (_inChildIsolate) {
     _ensureInitialized();
     if (_testToRun == description) body();
   } else {
     test(description, () {
-      expect(_runInIsolate(description).then(validate), completes);
+      return _runInIsolate(description).then(validate);
     });
   }
 }
 
-/// The description of the test to run in the child isolate.
-///
-/// `null` in the parent isolate.
-String _testToRun;
-
-/// The port with which the child isolate should communicate with the parent
-/// isolate.
-///
-/// `null` in the parent isolate.
-SendPort _replyTo;
-
-/// Whether or not we're running in a child isolate that's supposed to run a
-/// test.
-bool _inChildIsolate;
-
 /// Initialize metatest.
 ///
 /// [message] should be the second argument to [main]. It's used to determine
 /// whether this test is in the parent isolate or a child isolate.
-void initMetatest(message) {
+///
+/// [timeout], when specified, overrides the default timeout for unittest.
+void initMetatest(message, {Duration timeout}) {
+  _timeoutOverride = timeout;
   if (message == null) {
     _inChildIsolate = false;
   } else {
@@ -147,22 +159,27 @@ void initMetatest(message) {
   }
 }
 
-/// Runs the test described by [description] in its own isolate. Returns a map
-/// describing the results of that test run.
+// TODO(kevmoo) We need to capture the main method to allow running in an
+// isolate. There is no mechanism to capture the current executing URI between
+// browser and vm. Issue 1145 and/or Issue 8440
+void initTests(void testBody(message)) {
+  _testBody = testBody;
+  _testBody(null);
+}
+
+/// Runs the test described by [description] in its own isolate.
+///
+/// Returns a map describing the results of that test run.
 Future<Map> _runInIsolate(String description) {
+  if (_testBody == null) {
+    throw new StateError('initTests was not called.');
+  }
+
   var replyPort = new ReceivePort();
-  // TODO(nweiz): Don't use path here once issue 8440 is fixed.
-  var uri = path.toUri(path.absolute(path.fromUri(Platform.script)));
-  return Isolate.spawnUri(uri, [], {
+  return Isolate.spawn(_testBody, {
     'testToRun': description,
     'replyTo': replyPort.sendPort
-  }).then((_) {
-    // TODO(nweiz): Remove this timeout once issue 8875 is fixed and we can
-    // capture top-level exceptions.
-    return timeout(replyPort.first, 30 * 1000, () {
-      throw 'Timed out waiting for test to complete.';
-    });
-  });
+  }).then((_) => replyPort.first);
 }
 
 /// Returns whether [results] (a test result map) describes a test run in which
@@ -186,11 +203,14 @@ String _summarizeTests(Map results) {
   buffer.writeln();
 
   var success = false;
-  if (results['passed'] == 0 && results['failed'] == 0 &&
-      results['errors'] == 0 && results['uncaughtError'] == null) {
+  if (results['passed'] == 0 &&
+      results['failed'] == 0 &&
+      results['errors'] == 0 &&
+      results['uncaughtError'] == null) {
     buffer.write('No tests found.');
     // This is considered a failure too.
-  } else if (results['failed'] == 0 && results['errors'] == 0 &&
+  } else if (results['failed'] == 0 &&
+      results['errors'] == 0 &&
       results['uncaughtError'] == null) {
     buffer.write('All ${results['passed']} tests passed.');
     success = true;
@@ -198,8 +218,9 @@ String _summarizeTests(Map results) {
     if (results['uncaughtError'] != null) {
       buffer.write('Top-level uncaught error: ${results['uncaughtError']}');
     }
-    buffer.write('${results['passed']} PASSED, ${results['failed']} FAILED, '
-        '${results['errors']} ERRORS');
+    buffer.write(
+        '${results['passed']} PASSED, ${results['failed']} FAILED, '
+            '${results['errors']} ERRORS');

[nweiz, 2014/09/10 21:04:42]

There's still reformatting all through this method.

[kevmoo, 2014/09/17 21:16:28]

Done.

   }
   return prefixLines(buffer.toString());
 }
@@ -212,10 +233,11 @@ String _indent(String str) {
 /// Ensure that the metatest configuration is loaded.
 void _ensureInitialized() {
   unittestConfiguration = _singleton;
+  if (_timeoutOverride != null) {
+    unittestConfiguration.timeout = _timeoutOverride;
+  }
 }
 
-final _singleton = new _MetaConfiguration();
-
 /// Special test configuration for use within the child isolates. This hides all
 /// output and reports data back to the parent isolate.
 class _MetaConfiguration extends Configuration {