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 66%
rename from pkg/scheduled_test/test/metatest.dart
rename to pkg/metatest/lib/metatest.dart
index 3c0cd9acb39f8189e56dcc284fac685d4ddd79f9..95ceeefe31c5531f1a3b986ea1cfdfdf7773345e 100644
--- a/pkg/scheduled_test/test/metatest.dart
+++ b/pkg/metatest/lib/metatest.dart
@@ -10,14 +10,49 @@
 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';
+Future defer(void fn()) {

[nweiz, 2014/09/08 22:44:40]

What is this for? Where is it used? Why not just call Future.sync manually? Why
isn't this documented? Why is it a publicly-exposed function of this package?

[kevmoo, 2014/09/10 03:17:28]

Done.

+  return new Future.sync(fn);
+}
+
+/// Runs [setUpFn] before every metatest. Note that [setUpFn] will be
+/// overwritten if the test itself calls [setUp].

[nweiz, 2014/09/08 22:44:41]

While you're in here, can you fix the comments to have the first sentence take
up its own paragraph?

[kevmoo, 2014/09/10 03:17:28]

Done.

+void metaSetUp(void setUpFn()) {
+  if (_inChildIsolate) setUp(setUpFn);
+}
+
+void expectTestResults(String description, void body(),

[nweiz, 2014/09/08 22:44:40]

Document public functions.

[kevmoo, 2014/09/10 03:17:28]

Done.

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

[nweiz, 2014/09/08 22:44:40]

Don't type annotate local variables. "as" counts.

[kevmoo, 2014/09/10 03:17:28]

Done.

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

[nweiz, 2014/09/08 22:44:41]

Why don't these [expect] calls have reasons anymore? if this fails, it's going
to be very unclear what happened.

[kevmoo, 2014/09/10 03:17:28]

Done.

+
+    for (var i = 0; i < list.length; i++) {
+      var expectedMap = expectedResults[i];
+      var map = list[i] as Map;
+
+      expectedMap.forEach((key, value) {
+        expect(map, containsPair(key, value));
+      });
+    }
+  });
+}
+
+void expectSingleTest(String description, String result, String message, void
+    body()) {

[nweiz, 2014/09/08 22:44:40]

Nit: don't split the type and variable name across lines.

[kevmoo, 2014/09/10 03:17:28]

Done.

+  _setUpTest(description, body, (results) {
+    var testResults = results['results'] as List;
+    if (testResults.length != 1) {
+      throw 'Expected only one test to run';

[nweiz, 2014/09/08 22:44:40]

Don't throw strings.

[kevmoo, 2014/09/10 03:17:28]

Done.

+    }
+    var testResult = testResults.single as Map;
+    expect(testResult['result'], result);
+    expect(testResult['message'], message);
+  });
+}
 
 /// 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
@@ -35,55 +70,24 @@ void expectTestsPass(String description, void body(), {List<String> passing}) {
       }
     } else {
       var shouldPass = new Set.from(passing);
-      var didPass = new Set.from(results['results']
-          .where((t) => t['result'] == 'pass')
-          .map((t) => t['description']));
+      var didPass =
+          new Set.from(
+              results['results'].where(
+                  (t) => t['result'] == 'pass').map((t) => t['description']));

[nweiz, 2014/09/08 22:44:41]

Why did you change the formattin ghere? This looks much worse.

[kevmoo, 2014/09/10 03:17:28]

Done.

 
       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)}');
+        fail(
+            'Expected exactly ${stringify(shouldPass)} to pass, but '

[nweiz, 2014/09/08 22:44:40]

This also doesn't seem like a good reformatting at all.

[kevmoo, 2014/09/10 03:17:28]

Done.

+                '${stringify(didPass)} passed.\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.
-///
-/// 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,21 +103,15 @@ 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);
     });
   }
 }
@@ -133,11 +131,16 @@ SendPort _replyTo;
 /// test.
 bool _inChildIsolate;
 
+Function _testBody;

[nweiz, 2014/09/08 22:44:40]

Document these fields.

[kevmoo, 2014/09/10 03:17:28]

Done.

+
+Duration _timeoutOverride;
+
 /// 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.

[nweiz, 2014/09/08 22:44:40]

Document [timeout].

[kevmoo, 2014/09/10 03:17:28]

Done.

-void initMetatest(message) {
+void initMetatest(Map message, {Duration timeout}) {

[nweiz, 2014/09/08 22:44:40]

I don't like type-annotating [message] here. The fact that it's a map is an
implementation detail. Metatest should be able to change it to e.g. a SendPort
if it wants.

[kevmoo, 2014/09/10 03:17:28]

Done.

+  _timeoutOverride = timeout;
   if (message == null) {
     _inChildIsolate = false;
   } else {
@@ -147,28 +150,35 @@ void initMetatest(message) {
   }
 }
 
+// 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)) {

[nweiz, 2014/09/08 22:44:40]

Why does the user have to call both [initTests] and [initMetatest]? They should
only have to call one initialization function. Since you have the body here, you
can automatically wrap it and have it call [initMetatest].

[kevmoo, 2014/09/10 03:17:28]

I wish I could figure out how to do this. _body is passed to Isolate.spawn. I
couldn't find a way to do this.

I cranked on this for a while and got nowhere. Would love if you could prove me
wrong.

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

Oh, I see, it's the top-level-function-only thing. Gross.

+  _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.');

[nweiz, 2014/09/08 22:44:40]

Explain how to use initTests here.

[kevmoo, 2014/09/10 03:17:28]

Done.

+  }
+
   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.';
-    });
+    return replyPort.first;

[nweiz, 2014/09/08 22:44:41]

Nit: Use "=>" here

[kevmoo, 2014/09/10 03:17:28]

Done.

   });
 }
 
 /// Returns whether [results] (a test result map) describes a test run in which
 /// an error occurred.
 bool _hasError(Map results) {
-  return results['errors'] > 0 || results['uncaughtError'] != null ||
+  return results['errors'] > 0 ||
+      results['uncaughtError'] != null ||

[nweiz, 2014/09/08 22:44:40]

Fix all these gratuitous formatting changes.

[kevmoo, 2014/09/10 03:17:28]

Done.

       (results['passed'] == 0 && results['failed'] == 0);
 }
 
@@ -186,11 +196,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,10 +211,11 @@ 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');
   }
-  return prefixLines(buffer.toString());
+  return _prefixLines(buffer.toString());
 }
 
 /// Indents each line of [str] by two spaces.
@@ -212,6 +226,9 @@ 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();
@@ -238,3 +255,17 @@ class _MetaConfiguration extends Configuration {
     });
   }
 }
+
+/// Prepends each line in [text] with [prefix]. If [firstPrefix] is passed, the
+/// first line is prefixed with that instead.
+String _prefixLines(String text, {String prefix: '| ', String firstPrefix}) {

[nweiz, 2014/09/08 22:44:40]

This should go in a utils file.

[kevmoo, 2014/09/10 03:17:28]

Done.

+  var lines = text.split('\n');
+  if (firstPrefix == null) {
+    return lines.map((line) => '$prefix$line').join('\n');
+  }
+
+  var firstLine = "$firstPrefix${lines.first}";
+  lines = lines.skip(1).map((line) => '$prefix$line').toList();
+  lines.insert(0, firstLine);
+  return lines.join('\n');
+}