Improve test engine error feedback:

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

Index: content/browser/accessibility/dump_accessibility_tree_browsertest.cc
diff --git a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc b/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
index 786962d50a39b128721c32ac1e24a4f4e8ebcfa6..3c53bc85f33c6e9d91cacbebf97a6743cc406bf8 100644
--- a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
+++ b/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
@@ -50,26 +50,38 @@ namespace {
 class DumpAccessibilityTreeTest : public InProcessBrowserTest {
  public:
   // Utility helper that does a comment aware equality check.
-  bool EqualsWithComments(std::string& expected, std::string& actual) {
+  // Returns array of lines from expected file which are different.
+  std::vector<int> DiffLines(std::string& expected, std::string& actual) {
+    std::vector<int> diff_lines;
     std::vector<std::string> actual_lines, expected_lines;
     int actual_lines_count = Tokenize(actual, "\n", &actual_lines);
     int expected_lines_count = Tokenize(expected, "\n", &expected_lines);
-    int i = actual_lines_count - 1, j = expected_lines_count - 1;
-    while (i >= 0 && j >= 0) {
-      if (expected_lines[j].size() > 0 &&
-          expected_lines[j][0] == kCommentToken) {
-        --j;
+    int i = 0, j = 0;
+    boolean already_inside_diff_block = false;
+    while (i < actual_lines_count || j < expected_lines_count) {

[David Tseng, 2012/04/02 23:29:38]

Shouldn't this be i < actual_lines_count && j < expected_lines_count? We never
want to access indecies beyond these. You can print the remaining j diff if
necessary after the loop?

[aaronlevbugs, 2012/04/03 02:30:50]

It was as odd way of ensuring same size files. New patch doesn't have or need
that.

+      if (j < expected_lines_count &&
+          (expected_lines[j].size() == 0 ||
+          expected_lines[j][0] == kCommentToken)) {
+        // Skip comment lines and blank lines in expected output

[David Tseng, 2012/04/02 23:29:38]

nit: end the sentence with a period.

[aaronlevbugs, 2012/04/03 02:30:50]

Done.

+        ++j;
         continue;
       }
 
-      if (actual_lines[i] != expected_lines[j])
-        return false;
-      --i;
-      --j;
+      if (i >= actual_lines_count ||
+          j >= expected_lines_count ||
+          actual_lines[i] != expected_lines[j]) {
+        if (!already_inside_diff_block)  // Avoid redundant error reporting

[David Tseng, 2012/04/02 23:29:38]

How is this redundant? This would only pick up the first line of a difference
block.

[aaronlevbugs, 2012/04/03 02:30:50]

That's the idea, so that if one file has an extra line we don't report every
line as an error until the end of the file.

[aaronlevbugs, 2012/04/03 02:30:50]

New patch doesn't need this.

+          diff_lines.push_back(j);
+        already_inside_diff_block = true;
+      }
+      else

[David Tseng, 2012/04/02 23:29:38]

Please use the following conventions for multi-lined if/else clauses:
if (...) {
...
} else {
...
}

[aaronlevbugs, 2012/04/03 02:30:50]

Done.

+        already_inside_diff_block = false;
+      ++i;
+      ++j;
     }
 
     // Actual file has been fully checked.
-    return i < 0;
+    return diff_lines;
   }
 
   DumpAccessibilityTreeHelper helper_;
@@ -106,8 +118,6 @@ IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest,
   FilePath html_file(file_enumerator.Next());
   ASSERT_FALSE(html_file.empty());
   do {
-    printf("Testing %s\n", html_file.BaseName().MaybeAsASCII().c_str());
-
     std::string html_contents;
     file_util::ReadFileToString(html_file, &html_contents);
 
@@ -125,6 +135,13 @@ IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest,
     std::string expected_contents;
     RemoveChars(expected_contents_raw, "\r", &expected_contents);
 
+    if (!expected_contents.compare(0, 6, "#<skip")) {

[David Tseng, 2012/04/02 23:29:38]

Define this string literal as a constant along with the convention of the rest
of the file above.

[aaronlevbugs, 2012/04/03 02:30:50]

Done.

+      printf("Skipping %s\n", html_file.BaseName().MaybeAsASCII().c_str());
+      continue;
+    }
+
+    printf("Testing %s\n", html_file.BaseName().MaybeAsASCII().c_str());
+
     // Load the page.
     ui_test_utils::WindowedNotificationObserver tree_updated_observer(
         content::NOTIFICATION_RENDER_VIEW_HOST_ACCESSIBILITY_TREE_UPDATED,
@@ -144,10 +161,15 @@ IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest,
         host_view->GetBrowserAccessibilityManager()->GetRoot(),
         &actual_contents_utf16);
     std::string actual_contents = UTF16ToUTF8(actual_contents_utf16);
-    EXPECT_TRUE(EqualsWithComments(expected_contents, actual_contents));
-    if (expected_contents != actual_contents) {
+    std::vector<int> diff_lines = DiffLines(expected_contents, actual_contents);
+    boolean is_different = diff_lines.size() > 0;
+    EXPECT_FALSE(is_different);
+    if (is_different) {
       printf("*** EXPECTED: ***\n%s\n", expected_contents.c_str());
-      printf("*** ACTUAL: ***\n%s\n", actual_contents.c_str());
+      printf("==> Line #s for diffs: ");
+      for (int i = 0; i < static_cast<int>(diff_lines.size()); i++)

[David Tseng, 2012/04/02 23:29:38]

nit: ++i

[aaronlevbugs, 2012/04/03 02:30:50]

Done.

+        printf(" %d", diff_lines[i]);

[David Tseng, 2012/04/02 23:29:38]

Would it be useful to print the expected lines that failed to match and their
line numbers (rather than the entire expected file above and the line numbers
here).

[aaronlevbugs, 2012/04/03 02:30:50]

New patch shows all expected lines and marks those which did not match actual
output with a *.

+      printf("\n*** ACTUAL: ***\n%s\n", actual_contents.c_str());
     }
 
     if (!file_util::PathExists(expected_file)) {