Better pub integration test error reporting.

utils/tests/pub/test_pub.dart

 // Copyright (c) 2012, 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.
 
 /**
  * Test infrastructure for testing pub. Unlike typical unit tests, most pub
  * tests are integration tests that stage some stuff on the file system, run
  * pub, and then validate the results. This library provides an API to build
  * tests like that.
  */
@@ skipping 488 matching lines @@
   future.chain((_) => cleanup()).then((_) {
     asyncDone();
   });
 }
 
 /// Get the path to the root "util/test/pub" directory containing the pub tests.
 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();
 }
 
 /**
  * Schedules a call to the Pub command-line utility. Runs Pub with [args] and
  * validates that its results match [output], [error], and [exitCode].
  */
 void schedulePub([List<String> args, Pattern output, Pattern error,
     int exitCode = 0]) {
   _schedule((sandboxDir) {
     String pathInSandbox(path) => join(getFullPath(sandboxDir), path);
@@ skipping 14 matching lines @@
 
       var dartArgs =
           ['--enable-type-checks', '--enable-asserts', pubPath, '--trace'];
       dartArgs.addAll(args);
 
       var environment = new Map.from(Platform.environment);
       environment['PUB_CACHE'] = pathInSandbox(cachePath);
       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 = [];
 
-      Expect.equals(result.exitCode, exitCode,
-          'Pub returned exit code ${result.exitCode}, expected $exitCode.');
+      _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.');
+      }
+
+      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;
     });
   });
 }
 
 /**
  * A shorthand for [schedulePub] and [run] when no validation needs to be done
  * after Pub has been run.
  */
@@ skipping 48 matching lines @@
 
   return runNextEvent(null);
 }
 
 /**
  * Compares the [actual] output from running pub with [expected]. For [String]
  * patterns, ignores leading and trailing whitespace differences and tries to
  * 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
   // where the closing ''' is on its own line. It also fixes '' expected output
   // 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 (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]}');
+      }
     }
   }
 
-  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');
-    }
-
-    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);
   }
 }
 
 /**
  * Base class for [FileDescriptor] and [DirectoryDescriptor] so that a
  * directory can contain a heterogeneous collection of files and
  * subdirectories.
  */
 abstract class Descriptor {
   /**
@@ skipping 491 matching lines @@
 }
 
 /**
  * Schedules a callback to be called after Pub is run with [runPub], even if it
  * fails.
  */
 void _scheduleCleanup(_ScheduledEvent event) {
   if (_scheduledCleanup == null) _scheduledCleanup = [];
   _scheduledCleanup.add(event);
 }