Add an accessibility audit test for WebUI pages

chrome/test/base/js2gtest.js

Index: chrome/test/base/js2gtest.js
diff --git a/chrome/test/base/js2gtest.js b/chrome/test/base/js2gtest.js
index 9ec40a811cb06ccd5a2eca0be60cb1094c4bb111..b697b1e5b1c2cf53dccc69b9d4be54d0ceda6d88 100644
--- a/chrome/test/base/js2gtest.js
+++ b/chrome/test/base/js2gtest.js
@@ -187,6 +187,9 @@ function TEST_F(testFixture, testFunction, testBody) {
   print('  ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam +
         '"' + testFixture + '", ' +
         '"' + testFunction + '"));');
+  print('  ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam +

[Sheridan Rawlins, 2012/11/29 17:43:48]

Is there any mechanism for sheriffs to disable only the a11y portion of
flaky/broken tests?  I'm wondering about reducing generator changes by providing
an easy way to do A11Y testing - maybe:
TEST_F('MyFixture', ACCESSIBILITY_AUDIT)
and some sort of presubmit to ensure this test is added everywhere - what do you
think of that tack?  Presubmit may need to allow 'DISABLED_' +
ACCESSIBILITY_AUDIT or 'FLAKY_' + ACCESSIBILITY_AUDIT as well (for sheriffs
e.g.)

[dmazzoni, 2012/11/29 18:44:33]

I like this idea, but I think there should be a way to run the audit for every
test, even if that's not the default - see next comment for rationale.

[aboxhall, 2012/11/30 00:56:27]

I agree - perhaps it could be more along the lines of either TEST_F('MyFixture',
NO_ACCESSIBILITY_AUDIT) or a disableAccessibilityCheck() method at the start of
the test?

[dmazzoni, 2012/11/30 19:22:31]

If we use TEST_F it should be as a way to add a new test - like a fixture-wide
accessibility audit. It doesn't make sense to use TEST_F as a way to disable a
feature. A method like disableAccessibilityCheck() sounds more reasonable,
especially if it'd be possible to call it from a setUp() method for a whole
fixture rather than once for every test.

+        '"' + testFixture + '", ' +
+        '"accessibilityAudit"));');

[Sheridan Rawlins, 2012/11/29 17:43:48]

I'm confused - is this adding one call to accessibilityAudit to every test?  If
you have a fixture with 100 tests, e.g. would it perform _the same_
accessibility test 100 times (instead of just once?)  I think this may need to
be once per fixture; not once per test.  The previous suggestion would also make
(and make explicitly) only one per fixture.

[dmazzoni, 2012/11/29 18:44:33]

I think there may be many cases when running once per test might catch a lot
more; I'm thinking of cases like this:

TEST_F(DialogWithThreeTabs, TestFirstTab) {
  first_tab.show();
  ... run some tests
}

TEST_F(DialogWithThreeTabs, TestSecondTab) {
  second_tab.show();
  ... run some tests
}

If the accessibility audit ran only on the fixture, it might not ever test all
of the configurations the UI might be in.

What would be ideal would be to be able to specify this for each test.

Why aren't there JavaScript SetUpFixture and TearDownFixture methods that a
WebUI test can override? Should we add those?

I'm imagining allowing something like this:

DialogWithThreeTabs.SetUpTextFixture = function() {
    AccessibilityAudit.RunOnceAfterAllTests();
  or
    AccessibilityAudit.RunOnceAfterEachTest();
  or
    AccessibilityAudit.Disable();
};

When the audit fails, the error message could print out these instructions.

[Sheridan Rawlins, 2012/11/29 18:58:54]

This js test infra generates C++ gtest tests - I'm pretty sure gtest or gtest
running in browser_tests runs each test completely independently, so there's no
state saved in the fixture beyond setUp/tearDown that is available
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/test/data/webui/tes...)
and is shared & run for each test in a fixture.

So… I think that per-fixture should be solved by adding an explicit TEST_F, and
per-test would be either in the setUp/tearDown (or some other phase - maybe
accessibilityCheck and called from testDone within a separate try or maybe
refactoring the try/catch into a private method since it's similar for both
a11yCheck and tearDown - neither would want to throw all the way out or stop
execution of the next, and both would want errors reported back to the C++
counterpart)

It may also be needed to call explicitly - I can also vaguely remember some
tests that walked all the way through sign in flow - going to one page, clicking
on something, entering password, clicking something else - so there may be
multiple surfaces in one test that would all need to call out to the a11y
checker…

I can see the benefit of adding this for every test automatically, but I think
that there is need to have finer control for some tests, so the extra
overridable phase/method makes sense - it could default to doing the a11y check,
but be overridden if the check needs to be called at specific or multiple
places.

On 2012/11/29 18:44:33, Dominic Mazzoni wrote:

[aboxhall, 2012/11/30 00:56:27]

Where would the explicit TEST_F be added? Would it need to be added by the
developer for each fixture?

I am not sure about putting the accessibility check into teardown, as we'd have
to do some work to make sure the test results were reported nicely. That said,
perhaps calling the accessibility check at the start of testDone, with a bit of
work to combine the results of the explicit test and the accessibility check,
could work.
Good point; we could document that developers can call
this.runAccessibilityAudit() during a test to check the current state.
I agree; I like Dominic's idea of being able to set the configuration on a
per-fixture bases as part of setUp(); it may also make sense to be able to have
a method like disableAccessibilityCheck() that can be added as part of an
individual TEST_F() which would suppress the accessibility check for that test
only.

   if (testGenPostamble)
     testGenPostamble(testFixture, testFunction);
   print('}');