Allow filters in accessibility tests to specify which attributes to check.

Allow filters in accessibility tests to specify which attributes to check.

With this change, a DumpAccessibilityTree test can specify filters to
control which attributes are printed. This keeps each test expectation file
small and readable, while making it easy to test for all sorts of attributes,
including obscure ones.

Each platform will have a few attributes that print by default, like the
role and name. HTML files add rules of this form to override the defaults:

  @MAC-DENY:subrole*
  @MAC-DENY:value*
  @MAC-ALLOW:description*

You can also specify @MAC-ALLOW:* or @WIN-ALLOW:* if you want to dump everything - helpful
during development sometimes.

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

[dmazzoni, 2012-06-22 22:45:19]

Not ready for actual review, just looking for feedback.

Drive-bys welcome.

David, I'd like to know what you think of this approach before I do the Windows
impl and then clean up some of the test files based on this idea.

I have three design goals:
1. Test expectations should be more readable. If the test is just checking
roles, there's no point in dumping anything other than roles.
2. Making new attributes testable should never require rebasing existing tests.
Only the new tests would dump that attribute.
3. Fixing some obscure attribute for one particular type of object shouldn't
require rebasing other unrelated tests as often.

Right now the filters apply to all platforms. I haven't decided how to make them
platform-specific - should the filters go in the -expected file instead? Or
should the filters specify the platform, like @ALLOW-MAC:subrole?

Thanks in advance for your thoughts.

[David Tseng, 2012-06-26 00:01:00]

Looks like a good approach.

First question, do we want global filters or do we want per file filters (or
both)? Starting off with per file filters is good, but we might end up repeating
a lot of the same @* filters.

Also, perhaps use a set to hold the allow/deny filters.

[dmazzoni, 2012-07-27 21:47:51]

PTAL, I think this is finally complete.

On 2012/06/26 00:01:00, David Tseng wrote:
> Looks like a good approach.
> 
> First question, do we want global filters or do we want per file filters (or
> both)? Starting off with per file filters is good, but we might end up
repeating
> a lot of the same @* filters.

I added some global filters.

> Also, perhaps use a set to hold the allow/deny filters.

Done.

[David Tseng, 2012-07-31 17:04:16]

lgtm

http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
File content/browser/accessibility/dump_accessibility_tree_browsertest.cc
(right):

http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
content/browser/accessibility/dump_accessibility_tree_browsertest.cc:222:
IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityA) {
nit: Do we really want to add a test per file? Gtest can handle starting new
tests.

http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
(right):

http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:55: const
std::string DumpAccessibilityTreeHelper::GetAllowString() const {
nit: For simple/trivial accessors, do we want to name these as
"allow_filters()"? I realize it's not in the header, but perhaps more fitting
within the style guidelines.

http://codereview.chromium.org/10662003/diff/6008/content/public/test/accessi...
File content/public/test/accessibility_test_utils_win.h (right):

http://codereview.chromium.org/10662003/diff/6008/content/public/test/accessi...
content/public/test/accessibility_test_utils_win.h:8: #include <vector>
nit: new line above.

http://codereview.chromium.org/10662003/diff/6008/content/test/accessibility_...
File content/test/accessibility_test_utils_win.cc (right):

http://codereview.chromium.org/10662003/diff/6008/content/test/accessibility_...
content/test/accessibility_test_utils_win.cc:257: result->push_back(it->second);
nit: This seems repetitive with above (seems like the map could be passed as a
param).

[dmazzoni, 2012-07-31 17:14:04]

Thanks! Two inline response below:

On Tue, Jul 31, 2012 at 10:04 AM, <dtseng@chromium.org> wrote:

> http://codereview.chromium.**org/10662003/diff/6008/**
>
content/browser/accessibility/**dump_accessibility_tree_**browsertest.cc<http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibility/dump_accessibility_tree_browsertest.cc>
> File
> content/browser/accessibility/**dump_accessibility_tree_**browsertest.cc
> (right):
>
> http://codereview.chromium.**org/10662003/diff/6008/**
> content/browser/accessibility/**dump_accessibility_tree_**
>
browsertest.cc#newcode222<http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibility/dump_accessibility_tree_browsertest.cc#newcode222>
> content/browser/accessibility/**dump_accessibility_tree_**
> browsertest.cc:222:
> IN_PROC_BROWSER_TEST_F(**DumpAccessibilityTreeTest, AccessibilityA) {
> nit: Do we really want to add a test per file? Gtest can handle starting
> new tests.
>

My thought is that I want sheriffs to be able to easily disable an
individual test if it starts failing, maybe as the result of a WebKit roll
- rather than disable the whole suite of tests.

What do you think?

I'd be happy with doing it more automatically if we could provide an easy
and clear way for sheriffs to enable/disable tests.


> http://codereview.chromium.**org/10662003/diff/6008/**
>
content/browser/accessibility/**dump_accessibility_tree_**helper_mac.mm<http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibility/dump_accessibility_tree_helper_mac.mm>
> File content/browser/accessibility/**dump_accessibility_tree_**
> helper_mac.mm <http://dump_accessibility_tree_helper_mac.mm>
> (right):
>
> http://codereview.chromium.**org/10662003/diff/6008/**
> content/browser/accessibility/**dump_accessibility_tree_**
>
helper_mac.mm#newcode55<http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibility/dump_accessibility_tree_helper_mac.mm#newcode55>
>
content/browser/accessibility/**dump_accessibility_tree_**helper_mac.mm:55<http://dump_accessibility_tree_helper_mac.mm:55>
> :
> const std::string DumpAccessibilityTreeHelper::**GetAllowString() const {
> nit: For simple/trivial accessors, do we want to name these as
> "allow_filters()"? I realize it's not in the header, but perhaps more
> fitting within the style guidelines.
>

I thought it was maybe nontrivial because there are multiple
implementations of the same method in different files. That make sense or
do you think it's trivial?

[David Tseng, 2012-07-31 18:51:39]

On Tue, Jul 31, 2012 at 10:14 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:
> Thanks! Two inline response below:
>
> On Tue, Jul 31, 2012 at 10:04 AM, <dtseng@chromium.org> wrote:
>>
>>
>>
http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
>> File
>> content/browser/accessibility/dump_accessibility_tree_browsertest.cc
>> (right):
>>
>>
>>
http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
>> content/browser/accessibility/dump_accessibility_tree_browsertest.cc:222:
>> IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityA) {
>> nit: Do we really want to add a test per file? Gtest can handle starting
>> new tests.
>
>
> My thought is that I want sheriffs to be able to easily disable an
> individual test if it starts failing, maybe as the result of a WebKit roll -
> rather than disable the whole suite of tests.
>
> What do you think?
>

This is fine for this patch.
 I do think we'll want to come up with a way to exclude/disable files
(maybe a skip file).

> I'd be happy with doing it more automatically if we could provide an easy
> and clear way for sheriffs to enable/disable tests.
>
>>
>>
>>
http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
>> File content/browser/accessibility/dump_accessibility_tree_helper_mac.mm
>> (right):
>>
>>
>>
http://codereview.chromium.org/10662003/diff/6008/content/browser/accessibili...
>> content/browser/accessibility/dump_accessibility_tree_helper_mac.mm:55:
>> const std::string DumpAccessibilityTreeHelper::GetAllowString() const {
>> nit: For simple/trivial accessors, do we want to name these as
>> "allow_filters()"? I realize it's not in the header, but perhaps more
>> fitting within the style guidelines.
>
>
> I thought it was maybe nontrivial because there are multiple implementations
> of the same method in different files. That make sense or do you think it's
> trivial?

Could go either way.
>

[dmazzoni, 2012-07-31 18:56:03]

On Tue, Jul 31, 2012 at 11:51 AM, David Tseng <dtseng@chromium.org> wrote:

> This is fine for this patch.
>  I do think we'll want to come up with a way to exclude/disable files
> (maybe a skip file).
>

Yeah, or maybe the default could be to run all files but there could be an
additional way to skip files from within this source file - to make things
easiest for sheriffs or anyone else trying to quickly disable a single
broken test.

- Dominic

[dmazzoni, 2012-08-01 07:44:09]

+jam for OWNERS review

[jam, 2012-08-01 20:26:32]

lgtm for files outside of accessibility dirs

[commit-bot: I haz the power, 2012-08-01 20:33:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/10662003/17025

[commit-bot: I haz the power, 2012-08-01 22:07:10]

Change committed as 149510