Improve test engine error feedback:

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
@@ skipping 32 matching lines @@
 // The flow of the test is as outlined below.
 // 1. Load an html file from chrome/test/data/accessibility.
 // 2. Read the expectation.
 // 3. Browse to the page and serialize the platform specific tree into a human
 //    readable string.
 // 4. Perform a comparison between actual and expected and fail if they do not
 //    exactly match.
 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_;
 };
 
 IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest,
                        PlatformTreeDifferenceTest) {
   RenderWidgetHostViewPort* host_view = static_cast<RenderWidgetHostViewPort*>(
           browser()->GetSelectedWebContents()->GetRenderWidgetHostView());
   RenderWidgetHost* host = host_view->GetRenderWidgetHost();
@@ skipping 16 matching lines @@
   // Grab all HTML files.
   file_util::FileEnumerator file_enumerator(test_path,
                                             false,
                                             file_util::FileEnumerator::FILES,
                                             FILE_PATH_LITERAL("*.html"));
 
   // TODO(dtseng): Make each of these a gtest with script.
   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);
 
     // Read the expected file.
     std::string expected_contents_raw;
     FilePath expected_file =
         FilePath(html_file.RemoveExtension().value() +
             helper_.GetExpectedFileSuffix());
     file_util::ReadFileToString(
         expected_file,
         &expected_contents_raw);
 
     // Tolerate Windows-style line endings (\r\n) in the expected file:
     // normalize by deleting all \r from the file (if any) to leave only \n.
     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,
         content::NotificationService::AllSources());
     string16 html_contents16;
     html_contents16 = UTF8ToUTF16(html_contents);
     GURL url(UTF8ToUTF16(kUrlPreamble) + html_contents16);
     browser()->OpenURL(OpenURLParams(
         url, Referrer(), CURRENT_TAB, content::PAGE_TRANSITION_TYPED, false));
 
     // Wait for the tree.
     tree_updated_observer.Wait();
 
     // Perform a diff (or write the initial baseline).
     string16 actual_contents_utf16;
     helper_.DumpAccessibilityTree(
         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)) {
       FilePath actual_file =
           FilePath(html_file.RemoveExtension().value() +
                    helper_.GetActualFileSuffix());
 
       EXPECT_TRUE(file_util::WriteFile(
           actual_file, actual_contents.c_str(), actual_contents.size()));
 
       ADD_FAILURE() << "No expectation found. Create it by doing:\n"
           << "mv " << actual_file.LossyDisplayName() << " "
           << expected_file.LossyDisplayName();
     }
   } while (!(html_file = file_enumerator.Next()).empty());
 }