Better pub integration test error reporting.

utils/tests/pub/test_pub.dart

Index: utils/tests/pub/test_pub.dart
diff --git a/utils/tests/pub/test_pub.dart b/utils/tests/pub/test_pub.dart
index 52087782a876354b9122cf577f3ac251c67d1e61..bc74e0202e8c2f61287af15222b9e815e2cfc49a 100644
--- a/utils/tests/pub/test_pub.dart
+++ b/utils/tests/pub/test_pub.dart
@@ -506,7 +506,7 @@ String get testDirectory {
   var dir = new Path.fromNative(new Options().script);
   while (dir.filename != 'pub') dir = dir.directoryPath;
 
-  return dir.toNativePath();
+  return new File(dir.toNativePath()).fullPathSync();
 }
 
 /**
@@ -541,14 +541,27 @@ void schedulePub([List<String> args, Pattern output, Pattern error,
       environment['DART_SDK'] = pathInSandbox(sdkPath);
 
       return runProcess(dartBin, dartArgs, workingDir: pathInSandbox(appPath),
-          environment: environment, pipeStdout: output == null,
-          pipeStderr: error == null);
+          environment: environment);
     }).transform((result) {
-      _validateOutput(output, result.stdout);
-      _validateOutput(error, result.stderr);
+      var failures = [];
+
+      _validateOutput(failures, 'stdout', output, result.stdout);
+      _validateOutput(failures, 'stderr', error, result.stderr);
+
+      if (result.exitCode != exitCode) {
+        failures.add(
+            'Pub returned exit code ${result.exitCode}, expected $exitCode.');
+      }
 
-      Expect.equals(result.exitCode, exitCode,
-          'Pub returned exit code ${result.exitCode}, expected $exitCode.');
+      if (failures.length > 0) {
+        if (error == null) {
+          // If we aren't validating the error, still show it on failure.
+          failures.add('Pub stderr:');
+          failures.addAll(result.stderr.map((line) => '| $line'));
+        }
+
+        throw new ExpectException(Strings.join(failures, '\n'));
+      }
 
       return null;
     });
@@ -617,16 +630,29 @@ Future _runScheduled(Directory parentDir, List<_ScheduledEvent> scheduled) {
  * report the offending difference in a nice way. For other [Pattern]s, just
  * reports whether the output contained the pattern.
  */
-void _validateOutput(Pattern expected, List<String> actual) {
+void _validateOutput(List<String> failures, String pipe, Pattern expected,
+                     List<String> actual) {
   if (expected == null) return;
 
-  if (expected is String) return _validateOutputString(expected, actual);
-  var actualText = Strings.join(actual, "\n");
-  if (actualText.contains(expected)) return;
-  Expect.fail('Expected output to match "$expected", was:\n$actualText');
+  if (expected is RegExp) {

[nweiz, 2012/10/09 00:06:39]

I'd rather short-circuit than have so much nested code.

[Bob Nystrom, 2012/10/15 20:52:15]

I reordered this to have a positive "is RegExp" test, which helped the editor's
inference.  I agree the nesting sucks, though. Re-organized into a separate
_validateOutputRegExp() function.

+    var actualText = Strings.join(actual, "\n");
+    if (!actualText.contains(expected)) {
+      if (actual.length == 0) {
+        failures.add('Expected $pipe to match "${expected.pattern}" but got none.');

[nweiz, 2012/10/09 00:06:39]

Line length.

[Bob Nystrom, 2012/10/15 20:52:15]

Done.

+      } else {
+        failures.add('Expected $pipe to match "${expected.pattern}" but got:');
+        failures.addAll(actual.map((line) => '| $line'));
+      }
+    }
+
+    return;
+  }
+
+  _validateOutputString(failures, pipe, expected, actual);
 }
 
-void _validateOutputString(String expectedText, List<String> actual) {
+void _validateOutputString(List<String> failures, String pipe,
+                           String expectedText, List<String> actual) {
   final expected = expectedText.split('\n');
 
   // Strip off the last line. This lets us have expected multiline strings
@@ -634,34 +660,41 @@ void _validateOutputString(String expectedText, List<String> actual) {
   // to expect zero lines of output, not a single empty line.
   expected.removeLast();
 
-  final length = min(expected.length, actual.length);
+  var results = [];
+  var failed = false;
+
+  // Compare them line by line to see which ones match.
+  var length = max(expected.length, actual.length);
   for (var i = 0; i < length; i++) {
-    if (expected[i].trim() != actual[i].trim()) {
-      Expect.fail(
-        'Output line ${i + 1} was: ${actual[i]}\nexpected: ${expected[i]}');
-    }
-  }
+    if (i >= actual.length) {
+      // Missing output.
+      failed = true;
+      results.add('? ${expected[i]}');
+    } else if (i >= expected.length) {
+      // Unexpected extra output.
+      failed = true;
+      results.add('X ${actual[i]}');
+    } else {
+      var expectedLine = expected[i].trim();
+      var actualLine = actual[i].trim();
 
-  if (expected.length > actual.length) {
-    final message = new StringBuffer();
-    message.add('Missing expected output:\n');
-    for (var i = actual.length; i < expected.length; i++) {
-      message.add(expected[i]);
-      message.add('\n');
+      if (expectedLine != actualLine) {
+        // Mismatched lines.
+        failed = true;
+        results.add('X ${actual[i]}');
+      } else {
+        // Output is OK, but include it in case other lines are wrong.
+        results.add('| ${actual[i]}');
+      }
     }
-
-    Expect.fail(message.toString());
   }
 
-  if (expected.length < actual.length) {
-    final message = new StringBuffer();
-    message.add('Unexpected output:\n');
-    for (var i = expected.length; i < actual.length; i++) {
-      message.add(actual[i]);
-      message.add('\n');
-    }
-
-    Expect.fail(message.toString());
+  // If any lines mismatched, show the expected and actual.
+  if (failed) {
+    failures.add('Expected $pipe:');
+    failures.addAll(expected.map((line) => '| $line'));
+    failures.add('Got:');
+    failures.addAll(results);
   }
 }