Adds ability to comment expected files in DumpAccessibilityTests.

Adds ability to comment expected files in DumpAccessibilityTests.


BUG=none
TEST=this suite of tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126436

[David Tseng, 2012-03-08 18:56:35]

Ready for a look (will wait for your indentation patch).
Primary: dmazzoni
FYI: aaronlev

[dmazzoni, 2012-03-09 19:55:37]

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:54: int
actual_line_length = Tokenize(actual, line_ending, &actual_lines);
How about line_count instead of line_length? line_length makes me think of the
number of characters in a line, but here it means the number of lines.

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:64:
EXPECT_EQ(actual_lines[i], expected_lines[j]);
This could spew a whole bunch of errors if there's a difference. It also might
be confusing to have them appear in reverse order like this.

How about just setting a flag and then expect just one at the end?

Alternatively maybe it'd be more clear to have this function return a boolean
and do the EXPECT from the calling function? That might work better with my
merge.

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:151: } //
namespace
I think tests aren't supposed to be in an anonymous namespace - it prevents them
from being selectable via gtest_filter. Let me know if you found examples to the
contrary for this same type of test, or if there's another good reason.

[David Tseng, 2012-03-12 18:51:58]

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:54: int
actual_line_length = Tokenize(actual, line_ending, &actual_lines);
substituted with  *lines_count.

On 2012/03/09 19:55:37, Dominic Mazzoni wrote:
> How about line_count instead of line_length? line_length makes me think of the
> number of characters in a line, but here it means the number of lines.

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:64:
EXPECT_EQ(actual_lines[i], expected_lines[j]);
Returns result of comparison now.
On 2012/03/09 19:55:37, Dominic Mazzoni wrote:
> This could spew a whole bunch of errors if there's a difference. It also might
> be confusing to have them appear in reverse order like this.
> 
> How about just setting a flag and then expect just one at the end?
> 
> Alternatively maybe it'd be more clear to have this function return a boolean
> and do the EXPECT from the calling function? That might work better with my
> merge.
>

http://codereview.chromium.org/9639016/diff/2001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:151: } //
namespace
gtest_filter works fine for me with the tests wrapped in a namespace. Saw this
pattern being used elsewhere, but don't have a good reason beyond that.

On 2012/03/09 19:55:37, Dominic Mazzoni wrote:
> I think tests aren't supposed to be in an anonymous namespace - it prevents
them
> from being selectable via gtest_filter. Let me know if you found examples to
the
> contrary for this same type of test, or if there's another good reason.

[dmazzoni, 2012-03-12 19:03:28]

lgtm

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:52: bool
ExpectEqualsWithComments(std::string& expected, std::string& actual) {
Rename to EqualsWithComments

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:71:
EXPECT_LT(i, 0);
If it fails, you'll get two EXPECT failures - how about just return i < 0 here?

[commit-bot: I haz the power, 2012-03-12 20:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/9639016/13001

[David Tseng, 2012-03-12 20:44:22]

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:52: bool
ExpectEqualsWithComments(std::string& expected, std::string& actual) {
On 2012/03/12 19:03:28, Dominic Mazzoni wrote:
> Rename to EqualsWithComments

Done.

http://codereview.chromium.org/9639016/diff/7001/content/browser/accessibilit...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:71:
EXPECT_LT(i, 0);
On 2012/03/12 19:03:28, Dominic Mazzoni wrote:
> If it fails, you'll get two EXPECT failures - how about just return i < 0
here?

Done.

[commit-bot: I haz the power, 2012-03-12 22:20:17]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 126245, job name
was 9639016-13001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2012-03-13 16:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/9639016/13001

[commit-bot: I haz the power, 2012-03-13 19:36:17]

Change committed as 126436