Skip to content

GitLab

T
tangled
Commit eed7d97c authored by Dan H's avatar Dan H Committed by Commit Bot
Browse files
Options

Revert "Fix accessibility dump tree tests to not need an end-of-file sentinel." 

This reverts commit c7d0001d.

Reason for revert: May be causing test failures: https://bugs.chromium.org/p/chromium/issues/detail?id=1063167

Original change's description:
> Fix accessibility dump tree tests to not need an end-of-file sentinel.
> 
> The DumpAccessibilityTree tests generate an output text file and diff
> the results against an expected file. Previously, the way that the
> algorithm handled files of different lengths was by adding an
> end-of-file sentinel to the end. This resulted in that sentinel
> getting checked in, which was just confusing.
> 
> Fix this by modifying the DiffLines helper function to properly compare
> files that don't have the same number of lines. Add full unit tests for
> DiffLines so we can be confident it works.
> 
> Continue to LOG the end-of-file sentinel, because it's needed by
> running rebase_dump_accessibility_tree_test.py to parse actual output
> files from remote logs.
> 
> Bug: none
> Change-Id: If1bcef063822fd5786acaeff9133c05d5fc5b065
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2109458
> Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Avi Drissman <avi@chromium.org>
> Auto-Submit: Dominic Mazzoni <dmazzoni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#751791}

TBR=avi@chromium.org,dmazzoni@chromium.org,aleventhal@chromium.org

Change-Id: Ic369d42c9bd201792b03bcf995cb57ad3383b93a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: none
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2111771Reviewed-by: default avatarDan H <harringtond@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751854}
parent 2c71d6ee
Show whitespace changes
Inline Side-by-side
Showing with 11 additions and 98 deletions
content/browser/accessibility/dump_accessibility_browsertest_base.cc
View file @ eed7d97c
......@@ -367,6 +367,7 @@ void DumpAccessibilityTestBase::RunTestForPlatform(
}
// Validate against the expectation file.
actual_lines.push_back(kMarkEndOfFile);
bool matches_expectation = test_helper.ValidateAgainstExpectation(
file_path, expected_file, actual_lines, *expected_lines);
EXPECT_TRUE(matches_expectation);
......
content/public/test/dump_accessibility_test_helper.cc
View file @ eed7d97c
......@@ -19,9 +19,10 @@ namespace {
const char kCommentToken = '#';
const char kMarkSkipFile[] = "#<skip";
const char kSignalDiff[] = "*";
const char kMarkEndOfFile[] = "<-- End-of-file -->";
} // namespace
const char kMarkEndOfFile[] = "<-- End-of-file -->";
DumpAccessibilityTestHelper::DumpAccessibilityTestHelper(
AccessibilityTestExpectationsLocator* test_locator)
: test_locator_(test_locator) {}
......@@ -79,6 +80,10 @@ DumpAccessibilityTestHelper::LoadExpectationFile(
base::SplitString(expected_contents, "\n", base::KEEP_WHITESPACE,
base::SPLIT_WANT_NONEMPTY);
// Marking the end of the file with a line of text ensures that
// file length differences are found.
expected_lines.push_back(kMarkEndOfFile);
return expected_lines;
}
......@@ -118,11 +123,6 @@ bool DumpAccessibilityTestHelper::ValidateAgainstExpectation(
diff += "------\n";
diff += base::JoinString(actual_lines, "\n");
diff += "\n";
// This is used by rebase_dump_accessibility_tree_test.py to signify
// the end of the file when parsing the actual output from remote logs.
diff += kMarkEndOfFile;
diff += "\n";
LOG(ERROR) << "Diff:\n" << diff;
} else {
LOG(INFO) << "Test output matches expectations.";
......@@ -163,20 +163,6 @@ std::vector<int> DumpAccessibilityTestHelper::DiffLines(
++j;
}
// Report a failure if there are additional expected lines or
// actual lines.
if (i < actual_lines_count) {
diff_lines.push_back(j);
} else {
while (j < expected_lines_count) {
if (expected_lines[j].size() > 0 &&
expected_lines[j][0] != kCommentToken) {
diff_lines.push_back(j);
}
j++;
}
}
// Actual file has been fully checked.
return diff_lines;
}
......
content/public/test/dump_accessibility_test_helper.h
View file @ eed7d97c
......@@ -5,7 +5,6 @@
#ifndef CONTENT_PUBLIC_TEST_DUMP_ACCESSIBILITY_TEST_HELPER_H_
#define CONTENT_PUBLIC_TEST_DUMP_ACCESSIBILITY_TEST_HELPER_H_
#include "base/gtest_prod_util.h"
#include "base/optional.h"
namespace base {
......@@ -14,6 +13,9 @@ class FilePath;
namespace content {
// Sentinal value to mark end of actual/expected results.
extern const char kMarkEndOfFile[];
class AccessibilityTestExpectationsLocator;
// A helper class for writing accessibility tree dump tests.
......@@ -44,8 +46,6 @@ class DumpAccessibilityTestHelper {
const std::vector<std::string>& expected_lines);
private:
FRIEND_TEST_ALL_PREFIXES(DumpAccessibilityTestHelperTest, TestDiffLines);
// Utility helper that does a comment-aware equality check.
// Returns array of lines from expected file which are different.
static std::vector<int> DiffLines(
......
content/public/test/dump_accessibility_test_helper_unittest.cc deleted 100644 → 0
View file @ 2c71d6ee
// Copyright 2020 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 "content/public/test/dump_accessibility_test_helper.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using testing::ElementsAre;
namespace content {
TEST(DumpAccessibilityTestHelperTest, TestDiffLines) {
// Files with the same lines should have an empty diff.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines({"broccoli", "turnip"},
{"broccoli", "turnip"}),
ElementsAre());
// Empty files should have an empty diff.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines({}, {}), ElementsAre());
// If one line differs, that line number should be returned as the diff.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines({"broccoli", "turnip"},
{"watermelon", "turnip"}),
ElementsAre(0));
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines({"broccoli", "turnip"},
{"broccoli", "rutabaga"}),
ElementsAre(1));
// Multiple lines can differ.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines(
{"broccoli", "turnip", "carrot", "squash"},
{"broccoli", "aspartame", "carrot", "toothbrush"}),
ElementsAre(1, 3));
// Blank lines in the expected file are allowed.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines(
{"", "broccoli", "turnip", "carrot", "", "squash"},
{"broccoli", "turnip", "carrot", "squash"}),
ElementsAre());
// Comments in the expected file are allowed.
EXPECT_THAT(
DumpAccessibilityTestHelper::DiffLines(
{"#Vegetables:", "broccoli", "turnip", "carrot", "", "squash"},
{"broccoli", "turnip", "carrot", "squash"}),
ElementsAre());
// Comments or blank lines in the actual file are NOT allowed, that's a diff.
EXPECT_THAT(
DumpAccessibilityTestHelper::DiffLines(
{"broccoli", "turnip", "carrot", "squash"},
{"#vegetables", "broccoli", "turnip", "carrot", "", "squash"}),
ElementsAre(0, 1, 2, 3, 4));
// If the expected file has an extra line, that's a diff.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines(
{"broccoli", "turnip", "cow"}, {"broccoli", "turnip"}),
ElementsAre(2));
// If the actual file has an extra line, that's a diff.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines(
{"broccoli", "turnip"}, {"broccoli", "turnip", "horse"}),
ElementsAre(2));
// If the expected file has an extra blank line, that's okay.
EXPECT_THAT(DumpAccessibilityTestHelper::DiffLines({"broccoli", "turnip", ""},
{"broccoli", "turnip"}),
ElementsAre());
}
} // namespace content
content/test/BUILD.gn
View file @ eed7d97c
......@@ -1916,7 +1916,6 @@ test("content_unittests") {
"../public/common/url_utils_unittest.cc",
"../public/test/browser_task_environment_unittest.cc",
"../public/test/devtools_permission_overrides_unittest.cc",
"../public/test/dump_accessibility_test_helper_unittest.cc",
"../public/test/no_renderer_crashes_assertion_unittest.cc",
"../public/test/permission_type_unittest.cc",
"../public/test/referrer_unittest.cc",
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment