Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11363170:
Add an accessibility audit test for WebUI pages (Closed)
|
Created:
8 years, 1 month ago by aboxhall Modified:
7 years, 11 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, yoshiki+watch_chromium.org, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org, zork+watch_chromium.org, dmazzoni, Sheridan Rawlins, Dan Beam, rkc, Yaron, Jay Civelli Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd an accessibility audit for WebUI pages.
This change adds an accessibility audit which runs just before tearDown() on any WebUI browser test that uses the chrome/test/data/webui/test_api.js framework.
As of this change, the test will run by default for each test case, but not cause any errors in the case of accessibility issues. As we fix accessibility issues, we can set the a11yIssuesAreErrors flag on a per-test and per-fixture basis, to catch regressions. Once the majority of WebUI pages are passing the accessibility audit, and we have a mechanism in place for ignoring false positives, we will transition to treating accessibility issues as errors by default.
http://www.chromium.org/developers/accessibility/webui-accessibility-audit has more information.
BUG=162740
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175866
Patch Set 1 #Patch Set 2 : Working test (but doesn't conform with style rules etc.) #Patch Set 3 : Pull audit code in from third_party #Patch Set 4 : Actually add the third_party library, and remove the old accessibility_audit.js" #Patch Set 5 : Run accessibility audit on all WebUI pages #Patch Set 6 : Rebase #Patch Set 7 : Moving accessibility-developer-tools to src/third_party #
Total comments: 2
Patch Set 8 : Remove reference to obsolete a11y_audit.js #
Total comments: 14
Patch Set 9 : First, very incomplete pass at doing accessibility check in Javascript only #
Total comments: 3
Patch Set 10 : Doing accessibility check in javascript, with flags for whether to run a11y checks and whether to t… #
Total comments: 8
Patch Set 11 : Just addressing comments #Patch Set 12 : Added some test cases for the test test #
Total comments: 3
Patch Set 13 : Working test for a11y audit #
Total comments: 26
Patch Set 14 : Addressing review comments and adding documentation. #Patch Set 15 : Change a11y to accessibility in public API #
Total comments: 51
Patch Set 16 : Addressed scr's comments (mostly) #Patch Set 17 : Rebase #
Total comments: 1
Patch Set 18 : Next round of review comments (including Dominic's) #Patch Set 19 : Rebase #
Total comments: 11
Patch Set 20 : Addressing review comments, and ensuring that expects* during tearDown generates errors #Patch Set 21 : Sync DEPS to latest accessibility-developer-tools (fixing one bug in a test) #
Total comments: 2
Patch Set 22 : Sheridan's patch and order change for imports in web_ui_browsertest #
Total comments: 2
Patch Set 23 : Add accessibility-developer-tools to v8_unit_test.cc, and check for undefined |document| before run… #Patch Set 24 : Fix comments on a11yErrors_ and a11yWarnings_ #Patch Set 25 : Add axs_testing to test_files in build/android/pylib/single_test_runner.py #Patch Set 26 : Rebase #
Total comments: 19
Patch Set 27 : Sort test_files in single_test_runner.py and remove unused constant in web_ui_browsertest.cc #Patch Set 28 : Nits #
Total comments: 4
Patch Set 29 : Addressed remaining comments. #
Total comments: 6
Patch Set 30 : Remove unnecessary {} #
Total comments: 4
Patch Set 31 : chrome/test/data/webui/test_api.js #Patch Set 32 : Remove copyright boilerplate, s/Test that/Tests that/g #Patch Set 33 : Rebase to master #Patch Set 34 : Reinstate copyright header #Messages
Total messages: 61 (0 generated)
+scr, dbeam Hi Sheridan, Dan; This is my work-in-progress to add an accessibility audit which runs on all WebUI pages, as per the associated bug. I'm pulling in accessibility-developer-tools separately (https://codereview.chromium.org/11348234/ - this has been rolled out due to a problem with the SHA-1 hash in the DEPS line that I added, but is otherwise approved to go in). This code refers to a single file version of just those parts of the code which are needed to run an audit from javascript. The changes I've made here: - Pull in the third party javascript code in web_ui_browsertest.cc, analogous to how mock4js is pulled in - Add a 'runAccessibilityAudit' method to the testing.Test prototype in test_api.js, and add an 'accessibilityAudit' test to each test fixture created via the TEST_F function which calls that method - Add a generated call to RunJavascriptTestF() which runs the 'accessibilityAudit' test for each call to TEST_F during the js2gtest code generation. This necessitates an ugly hack in test_api.js where I call resetTestState() instead of errors.splice(0, errors.length) during testDone(), to ensure that the second (accessibilityAudit) test can run successfully. Without this, the testDone() method doesn't do the teardown and causes a console warning to be emitted. This code successfully runs the accessibility audit on any browser test which uses test_api.js (and catches quite a few potential accessibility issues). Do you have any ideas for how to more cleanly integrate this audit into browser tests?
https://codereview.chromium.org/11363170/diff/12002/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/12002/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:50: const FilePath::CharType kMockJS[] = FILE_PATH_LITERAL("mock4js.js"); unused right? if so, remove https://codereview.chromium.org/11363170/diff/12002/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:81: void WebUIBrowserTest::AddLibraryFromUTFPath(std::string path) { WebUIBrowserTest::AddLibraryFromUTFPath(const std::string& path) https://codereview.chromium.org/11363170/diff/12002/chrome/js_unittest_vars.gypi File chrome/js_unittest_vars.gypi (right): https://codereview.chromium.org/11363170/diff/12002/chrome/js_unittest_vars.g... chrome/js_unittest_vars.gypi:11: 'accessibility_audit_js': '<(DEPTH)/third_party/accessibility-developer-tools/gen/axs_testing.js', I'm a little fuzzy on the mechanism, but there was a need to ensure that all test files are available for ChromeOS testing by copying into test_data dir (or adding new dirs to the cros "ebuild" script - see https://codereview.chromium.org/8333013 e.g). I'll add rkc@ for awareness/comments. https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:190: print(' ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam + 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.) https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); 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. https://codereview.chromium.org/11363170/diff/12002/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:642: resetTestState(); I think that explicitly having TEST_F would also reduce the need to do this as the A11Y test would be its own test and behave the same as others rather than needing to glob onto/hack into existing tests.
https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:190: print(' ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam + On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > 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.) 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. https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > 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. 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.
https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); 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: > On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > > 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. > > 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.
https://codereview.chromium.org/11363170/diff/12002/chrome/js_unittest_vars.gypi File chrome/js_unittest_vars.gypi (right): https://codereview.chromium.org/11363170/diff/12002/chrome/js_unittest_vars.g... chrome/js_unittest_vars.gypi:11: 'accessibility_audit_js': '<(DEPTH)/third_party/accessibility-developer-tools/gen/axs_testing.js', On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > I'm a little fuzzy on the mechanism, but there was a need to ensure that all > test files are available for ChromeOS testing by copying into test_data dir (or > adding new dirs to the cros "ebuild" script - see > https://codereview.chromium.org/8333013 e.g). I'll add rkc@ for > awareness/comments. Ok; I don't exactly understand this either, I just copied the way mock_js is used. Looking forward to a better understanding. https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:190: print(' ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam + On 2012/11/29 18:44:33, Dominic Mazzoni wrote: > On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > > 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.) > > 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. 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? https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); On 2012/11/29 18:58:54, Sheridan Rawlins wrote: > 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) 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. > 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… Good point; we could document that developers can call this.runAccessibilityAudit() during a test to check the current state. > 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. 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. > On 2012/11/29 18:44:33, Dominic Mazzoni wrote: > > On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > > > 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. > > > > 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. > https://codereview.chromium.org/11363170/diff/12002/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:642: resetTestState(); On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > I think that explicitly having TEST_F would also reduce the need to do this as > the A11Y test would be its own test and behave the same as others rather than > needing to glob onto/hack into existing tests. As per my other comment, I don't completely understand the TEST_F proposal here. Definitely would be nice to be able to avoid this hack, though.
Based on our discussions, I think here should be the plan for this initial change: 1. Add the new logic to test_api.js rather than the generated C++ code. Have an accessibility audit method that's called automatically when a test is done. 2. Make it easy to override the accessibility audit if you don't want it just by overriding the method - no "magic". It should be just as easy to call it explicitly if you want to call it more often. 3. Add some info to the error messages so that if it breaks, a sheriff knows how to quickly disable the audit. https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/12002/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:190: print(' ' + testPredicate + '(RunJavascriptTestF(' + isAsyncParam + On 2012/11/30 00:56:27, aboxhall wrote: > On 2012/11/29 18:44:33, Dominic Mazzoni wrote: > > On 2012/11/29 17:43:48, Sheridan Rawlins wrote: > > > 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.) > > > > 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. > > 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? 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.
Sheridan: this is a very incomplete first cut of doing what we chatted about yesterday. I'm mostly looking at the changes to testDone() in test_api.js, and to web_ui_test_handler.cc - you will notice that it doesn't allow any per-test overriding or anything yet, but I wanted to see whether this mechanism for running the audit per-test and reporting the results matched what you had in mind.
Obviously still has a way to go (including testing the test), but this is closer to what we've been talking about. I included history_browsertest.js as an example of how to use the enable/disable flags and the a11y expectation in a given test.
Some comments… not complete on review either, but you said you had more to do too :D https://codereview.chromium.org/11363170/diff/6002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/6002/chrome/test/base/js2gtest.... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); We don't need this anymore do we? https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:49: const std::string kMockJSPath = "chrome/third_party/mock4js/mock4js.js"; Would rather see FILE_PATH_LITERAL here and below. https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:81: void WebUIBrowserTest::AddLibraryFromUTFPath(const std::string path) { const FilePath& path? https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:349: // EXPECT_TRUE(RunJavascriptTest("runAccessibilityAudit")); Still needed? https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:57: void AddLibraryFromUTFPath(const std::string path); Can this be a FilePath as well?
https://codereview.chromium.org/11363170/diff/6002/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/6002/chrome/test/base/js2gtest.... chrome/test/base/js2gtest.js:192: '"accessibilityAudit"));'); On 2012/12/07 06:11:35, scr wrote: > We don't need this anymore do we? No, this has been reverted. https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:49: const std::string kMockJSPath = "chrome/third_party/mock4js/mock4js.js"; On 2012/12/07 06:11:35, scr wrote: > Would rather see FILE_PATH_LITERAL here and below. Done. https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:81: void WebUIBrowserTest::AddLibraryFromUTFPath(const std::string path) { On 2012/12/07 06:11:35, scr wrote: > const FilePath& path? Done. https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:349: // EXPECT_TRUE(RunJavascriptTest("runAccessibilityAudit")); On 2012/12/07 06:11:35, scr wrote: > Still needed? Nope! https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/24001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:57: void AddLibraryFromUTFPath(const std::string path); On 2012/12/07 06:11:35, scr wrote: > Can this be a FilePath as well? Done.
scr: Obviously this still needs some polish, but it's complete and tested now. Could you PTAL and tell me if there are any huge problems you see?
Please update the change title and description now that this is basically finished. Please summarize the important points for anyone who drives by, like that this runs on dozens of tests, but it's just informational for now. https://codereview.chromium.org/11363170/diff/36001/.gitmodules File .gitmodules (right): https://codereview.chromium.org/11363170/diff/36001/.gitmodules#newcode25 .gitmodules:25: [submodule "chrome/test/data/perf/third_party/octane"] I'm assuming most of these changes are not really part of this change? https://codereview.chromium.org/11363170/diff/36001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); indentation? https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.cc:15: AddLibrary(FilePath(FILE_PATH_LITERAL( Can you move most of these repeated lines to a common SetUp method? https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.h (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.h:10: class WebUIA11yAuditTest : public WebUIBrowserTest { I'm thinking we should probably not use A11y in filenames or class names, it's not widely known enough. Feel free to use it in comments or variable names within an implementation class if you define it once at the top - but let's avoid it anywhere that non-accessibility developers are likely to run across frequently. Let me know if you see exceptions to this elsewhere in the codebase! https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.js:15: expectedA11yWarnings: null, It should be fine to use A11y here, for variables local to this file - maybe define it once at the top. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.js:44: function mockAudit() { How about createMockAudit? https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:612: var a11y_warnings = []; Use camelCase but make it a private variable with a trailing underscore - accessibilityWarnings_ https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:615: function getA11yWarnings() { Should you use js getters? http://stackoverflow.com/questions/812961/javascript-getters-and-setters-for-... They seem to be used elsewhere within Chrome's JS codebase https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:866: if (result.result == axs.constants.AuditResult.FAIL) { result.result is confusing - i wonder if it should be something like "status" or "resultCode" upstream https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:903: function a11yIssuesToMessage(a11y_errors, a11y_warnings, message) { It might be cleaner here to just have this function return the message, then create a tiny helper function to append a string with initial newlines because they're multi-line log messages. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:923: 'https://sites.google.com/a/chromium.org/dev/webui-accessibility-audit'; This url is shorter and should be the same: http://chromium.org/webui-accessibility-audit Although, I think you should probably move it from the top level first - most other accessibility documentation has a url more like this: http://chromium.org/developers/accessibility/windows-accessibility https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:950: for (var i = 0; i < Math.min(result.elements.length, 5); i++) Do the Math.min just once https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:960: var a11y_errors = []; var a11yErrors (no underscores in js variables normally)
Sheridan: I've fixed up the FIXMEs and addressed Dominic's comments; just waiting on your review now, whenever you can take a look. https://codereview.chromium.org/11363170/diff/36001/.gitmodules File .gitmodules (right): https://codereview.chromium.org/11363170/diff/36001/.gitmodules#newcode25 .gitmodules:25: [submodule "chrome/test/data/perf/third_party/octane"] On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > I'm assuming most of these changes are not really part of this change? In fact none of them are, but I can't figure out why it changed or how to revert it. https://codereview.chromium.org/11363170/diff/36001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > indentation? Ugh. Don't know how that happened. Done. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.cc:15: AddLibrary(FilePath(FILE_PATH_LITERAL( On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > Can you move most of these repeated lines to a common SetUp method? Actually, this file is no longer used. Removed. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.h (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.h:10: class WebUIA11yAuditTest : public WebUIBrowserTest { On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > I'm thinking we should probably not use A11y in filenames or class names, it's > not widely known enough. > > Feel free to use it in comments or variable names within an implementation class > if you define it once at the top - but let's avoid it anywhere that > non-accessibility developers are likely to run across frequently. > > Let me know if you see exceptions to this elsewhere in the codebase! I'm happy to change the file name, but changing it in the class name makes it eye-wateringly long. I've changed it for now, we can see what they look like side by side. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... File chrome/test/data/webui/a11y_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.js:15: expectedA11yWarnings: null, On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > It should be fine to use A11y here, for variables local to this file - maybe > define it once at the top. SGTM. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/a1... chrome/test/data/webui/a11y_audit_browsertest.js:44: function mockAudit() { On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > How about createMockAudit? Done. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:612: var a11y_warnings = []; On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > Use camelCase but make it a private variable with a trailing underscore - > accessibilityWarnings_ Done. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:615: function getA11yWarnings() { On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > Should you use js getters? > > http://stackoverflow.com/questions/812961/javascript-getters-and-setters-for-... > > They seem to be used elsewhere within Chrome's JS codebase It won't work in this case as it's not a property of an object. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:866: if (result.result == axs.constants.AuditResult.FAIL) { On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > result.result is confusing - i wonder if it should be something like "status" or > "resultCode" upstream Or I could change result to auditResult? Changing it upstream is probably a good idea at some point, I agree. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:903: function a11yIssuesToMessage(a11y_errors, a11y_warnings, message) { On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > It might be cleaner here to just have this function return the message, then > create a tiny helper function to append a string with initial newlines because > they're multi-line log messages. I collapsed it into one line; hopefully that's a bit less obtrusive. I couldn't see anywhere else where the method might be used. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:923: 'https://sites.google.com/a/chromium.org/dev/webui-accessibility-audit'; On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > This url is shorter and should be the same: > > http://chromium.org/webui-accessibility-audit > > Although, I think you should probably move it from the top level first - most > other accessibility documentation has a url more like this: > > http://chromium.org/developers/accessibility/windows-accessibility Done. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:950: for (var i = 0; i < Math.min(result.elements.length, 5); i++) On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > Do the Math.min just once Done. https://codereview.chromium.org/11363170/diff/36001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:960: var a11y_errors = []; On 2012/12/14 18:45:41, Dominic Mazzoni wrote: > var a11yErrors (no underscores in js variables normally) Done.
https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:49: const std::string kMockJSPath = "chrome/third_party/mock4js/mock4js.js"; Why aren't these also FILE_PATH_LITERALs? https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:57: void AddLibraryFromUTFPath(const std::string path); Can this take a FilePath? Why a string? https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): https://codereview.chromium.org/11363170/diff/20001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_test_handler.cc:79: if (test_result->GetSize() > 2) { This won't be needed if you do the console.warn stuff we talked about, otherwise let errors be errors if a11yIssuesAreErrors. https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:49: const FilePath::CharType kMockJSPath[] = Any reason we can't keep these alphabetical? https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:82: void WebUIBrowserTest::AddLibraryFromPath(const FilePath path) { const FilePath& path https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath path); const FilePath& path https://codereview.chromium.org/11363170/diff/38014/chrome/js_unittest_vars.gypi File chrome/js_unittest_vars.gypi (right): https://codereview.chromium.org/11363170/diff/38014/chrome/js_unittest_vars.g... chrome/js_unittest_vars.gypi:11: 'accessibility_audit_js': '<(DEPTH)/third_party/accessibility-developer-tools/gen/axs_testing.js', Is this needed/used? https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:5: function WebUIAccessibilityAuditBrowserTest() {} JSDoc this class/constructor https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:16: expectedWarnings: null, Should these be true/false? https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:23: assertEquals(this.expectedWarnings, getAccessibilityWarnings().length); expect? Otherwise, it'll skip the call to superclass's tearDown (and fail to report mock4js errors, if any). https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:28: function addAuditFailures() { jsdoc https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:45: function createMockAudit() { jsdoc https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:51: function expectAuditWillNotRun() { jsdoc https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:57: function expectAuditWillRun(times) { jsdoc https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:64: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); I'm a bit confused by this… couldn't you just do something like the following in the for loop? expectedInvocation.will(callFunction(realAudit.run)); https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:68: TEST_F('WebUIAccessibilityAuditBrowserTest', 'testWithAuditFailures_shouldFail', regular comment ( // ) what the test intention is. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:74: TEST_F('WebUIAccessibilityAuditBrowserTest', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:82: function WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture() {} jsdoc this class/constructor https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:91: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:97: }); For kicks, could we add a version of this that does expectAuditWillRun(2); ... expectAccessibilityOk(); So we can see it run once with our expect, and once with the tearDown? https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:99: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:106: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:113: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:121: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:128: function WebUIAccessibilityAuditBrowserTest_IssuesAreWarnings() {} jsdoc class/constructor https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:136: TEST_F('WebUIAccessibilityAuditBrowserTest_IssuesAreWarnings', // regular comment https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:145: Any way to test for warnings being logged? Maybe in the C++? Might have to adjust the C++ of WebUIBrowserTest's LogHandler to also save the logging::LOG_WARNING (or whatever the log level is for console.warn) so it could be checked by a test in C++. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/we...
https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:49: const FilePath::CharType kMockJSPath[] = On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > Any reason we can't keep these alphabetical? Nope. Done. https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:82: void WebUIBrowserTest::AddLibraryFromPath(const FilePath path) { On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > const FilePath& path Done. https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/38014/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath path); On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > const FilePath& path Done. https://codereview.chromium.org/11363170/diff/38014/chrome/js_unittest_vars.gypi File chrome/js_unittest_vars.gypi (right): https://codereview.chromium.org/11363170/diff/38014/chrome/js_unittest_vars.g... chrome/js_unittest_vars.gypi:11: 'accessibility_audit_js': '<(DEPTH)/third_party/accessibility-developer-tools/gen/axs_testing.js', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > Is this needed/used? No. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:5: function WebUIAccessibilityAuditBrowserTest() {} On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > JSDoc this class/constructor Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:16: expectedWarnings: null, On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > Should these be true/false? They are numerical values. Added JSDoc. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:23: assertEquals(this.expectedWarnings, getAccessibilityWarnings().length); On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > expect? Otherwise, it'll skip the call to superclass's tearDown (and fail to > report mock4js errors, if any). Ah yes, I was having trouble with expect initially and forgot to change it back. Thanks. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:28: function addAuditFailures() { On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:45: function createMockAudit() { On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:51: function expectAuditWillNotRun() { On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:57: function expectAuditWillRun(times) { On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:64: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > I'm a bit confused by this… couldn't you just do something like the following in > the for loop? > > expectedInvocation.will(callFunction(realAudit.run)); Unfortunately, no: will() can only be called once; it destroys all previous expectations each time it's run. See https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... and https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:68: TEST_F('WebUIAccessibilityAuditBrowserTest', 'testWithAuditFailures_shouldFail', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > regular comment ( // ) what the test intention is. Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:74: TEST_F('WebUIAccessibilityAuditBrowserTest', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:82: function WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture() {} On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc this class/constructor Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:97: }); On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > For kicks, could we add a version of this that does > > expectAuditWillRun(2); > ... > expectAccessibilityOk(); > > So we can see it run once with our expect, and once with the tearDown? Added at the end, in the fixture with |accessibilityIssuesAreErrors| set to false, so that it won't cause spurious test passes when the test fails in an unexpected (due to mock expectations) rather than expected (due to audit failure) way. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:99: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:106: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:113: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:121: TEST_F('WebUIAccessibilityAuditBrowserTest_TestsDisabledInFixture', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:128: function WebUIAccessibilityAuditBrowserTest_IssuesAreWarnings() {} On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > jsdoc class/constructor Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:136: TEST_F('WebUIAccessibilityAuditBrowserTest_IssuesAreWarnings', On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > // regular comment Done. https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:145: On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > Any way to test for warnings being logged? Maybe in the C++? > > Might have to adjust the C++ of WebUIBrowserTest's LogHandler to also save the > logging::LOG_WARNING (or whatever the log level is for console.warn) so it could > be checked by a test in C++. > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/we... Would it be sufficient to mock out console.warn similarly to axs.Audit.run?
https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath path); const FilePath& path // (the reference is missing). https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:64: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); Ok, so how 'bout this after the for loop instead of needing the cumbersome "apply" syntax? for(…) … audit.expects(exactly(times)).run().will(willArgs) On 2012/12/18 19:21:35, aboxhall wrote: > On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > > I'm a bit confused by this… couldn't you just do something like the following > in > > the for loop? > > > > expectedInvocation.will(callFunction(realAudit.run)); > > Unfortunately, no: will() can only be called once; it destroys all previous > expectations each time it's run. See > https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... > and > https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:145: Sure, that seems reasonable. On 2012/12/18 19:21:35, aboxhall wrote: > On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > > Any way to test for warnings being logged? Maybe in the C++? > > > > Might have to adjust the C++ of WebUIBrowserTest's LogHandler to also save the > > logging::LOG_WARNING (or whatever the log level is for console.warn) so it > could > > be checked by a test in C++. > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/we... > > Would it be sufficient to mock out console.warn similarly to axs.Audit.run? https://codereview.chromium.org/11363170/diff/53001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/53001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:9: * to enable/disable it work as expected. Indent 4 spaces http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath path); Weird - I don't know why this comment didn't go out with the previous batch… Hasn't this already been fixed? Very weird. On 2012/12/18 23:13:44, scr wrote: > const FilePath& path // (the reference is missing).
One tiny request: I want to really set apart the accessibility audit from other errors, so there's no confusion as to where these errors are coming from. Could you add some bookends like this? *** Begin accessibility audit results *** An accessibility audit found 1 error on this page. For more information, please see http://chromium.org/developers/accessibility/webui-accessibility-audit Error: controlsWithoutLabel failed on the following element: #search-field", source: (1311) *** End accessibility audit results *** You could skip the bookends when it passes the audit.
https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/33001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath path); On 2012/12/18 23:15:09, scr wrote: > Weird - I don't know why this comment didn't go out with the previous batch… > Hasn't this already been fixed? Very weird. > > On 2012/12/18 23:13:44, scr wrote: > > const FilePath& path // (the reference is missing). > Yeah, fixed. No idea what's going on there, but eh! https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:64: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); On 2012/12/18 23:13:44, scr wrote: > Ok, so how 'bout this after the for loop instead of needing the cumbersome > "apply" syntax? > > for(…) … > audit.expects(exactly(times)).run().will(willArgs) > > On 2012/12/18 19:21:35, aboxhall wrote: > > On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > > > I'm a bit confused by this… couldn't you just do something like the > following > > in > > > the for loop? > > > > > > expectedInvocation.will(callFunction(realAudit.run)); > > > > Unfortunately, no: will() can only be called once; it destroys all previous > > expectations each time it's run. See > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... > > and > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/third_party/mock4j... > will() only takes varargs, not an array - hence the use of apply(). https://codereview.chromium.org/11363170/diff/38014/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:145: On 2012/12/18 23:13:44, scr wrote: > Sure, that seems reasonable. > > On 2012/12/18 19:21:35, aboxhall wrote: > > On 2012/12/18 01:43:35, Sheridan Rawlins wrote: > > > Any way to test for warnings being logged? Maybe in the C++? > > > > > > Might have to adjust the C++ of WebUIBrowserTest's LogHandler to also save > the > > > logging::LOG_WARNING (or whatever the log level is for console.warn) so it > > could > > > be checked by a test in C++. > > > > > > > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/we... > > > > Would it be sufficient to mock out console.warn similarly to axs.Audit.run? > Done.
scr: ping?
LGTM with nits (fine to commit after Dominic's LG and addressing nits). https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:5: function WebUIAccessibilityAuditBrowserTest() {} nit: jsdoc this, with @constructor @extends {testing.Test} https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:7: /** nit: Move @fileoverview below copyright & above first code. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:117: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); nit: expectedInvocation.will.apply(expectedInvocation, willArgs); Seems like that would be more compact & easier to read than sending a user's mind over to the class' prototype to realize it's the same thing as expectedInvocation. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:155: * Test fixture with |runAccessibilityChecks| set to false. nit: @constructor & @extends https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:214: * Test fixture with |accessibilityIssuesAreErrors| set to false. nit: @constructor & @extends
Hey, I don't see a try run on this CL - please run with git try
https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:5: function WebUIAccessibilityAuditBrowserTest() {} On 2012/12/20 20:27:49, scr wrote: > nit: jsdoc this, with > @constructor > @extends {testing.Test} Done. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:7: /** On 2012/12/20 20:27:49, scr wrote: > nit: Move @fileoverview below copyright & above first code. Done. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:117: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); On 2012/12/20 20:27:49, scr wrote: > nit: expectedInvocation.will.apply(expectedInvocation, willArgs); > > Seems like that would be more compact & easier to read than sending a user's > mind over to the class' prototype to realize it's the same thing as > expectedInvocation. Done. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:117: ExpectedInvocation.prototype.will.apply(expectedInvocation, willArgs); On 2012/12/20 20:27:49, scr wrote: > nit: expectedInvocation.will.apply(expectedInvocation, willArgs); > > Seems like that would be more compact & easier to read than sending a user's > mind over to the class' prototype to realize it's the same thing as > expectedInvocation. Done. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:155: * Test fixture with |runAccessibilityChecks| set to false. On 2012/12/20 20:27:49, scr wrote: > nit: @constructor & @extends Done. https://codereview.chromium.org/11363170/diff/57003/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:214: * Test fixture with |accessibilityIssuesAreErrors| set to false. On 2012/12/20 20:27:49, scr wrote: > nit: @constructor & @extends Done.
lgtm https://codereview.chromium.org/11363170/diff/60003/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/60003/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1604: exports.assertAccessibilityOk = assertAccessibilityOk; Nit: sort alphabetically
https://codereview.chromium.org/11363170/diff/60003/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/60003/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:1604: exports.assertAccessibilityOk = assertAccessibilityOk; On 2012/12/20 22:05:06, Dominic Mazzoni wrote: > Nit: sort alphabetically I agree in principle, but they're actually not in alpha order currently, so I'm not sure where it (and the analogous expect method) would go best.
Sheridan, is this good to go now?
Just a commenting change AFAICT; still LGTM especially if tests pass on trybots :D https://codereview.chromium.org/11363170/diff/52015/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/52015/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:142: * Like |errors|, cleared when results are reported. Hunh, these aren't cleared now. Do they need to be? I thought they weren't nuked because they were needed to be checked. Can you adjust the comments if these' aren't cleared?
https://codereview.chromium.org/11363170/diff/52015/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/52015/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:142: * Like |errors|, cleared when results are reported. On 2012/12/21 02:13:39, scr wrote: > Hunh, these aren't cleared now. Do they need to be? I thought they weren't > nuked because they were needed to be checked. Can you adjust the comments if > these' aren't cleared? Right - they'll be recreated for each new test now, so I will update this comment.
FYI: There are lots of test failures in the try jobs that have to do this with change (they all have runAccessibilityAudit in the stack).
On 2012/12/21 03:21:03, Dan Beam wrote: > FYI: There are lots of test failures in the try jobs that have to do this with > change (they all have runAccessibilityAudit in the stack). They all seem to be ChromeOS/Aura bots. Rahul, is there something I'm supposed to do to ensure that the new file is in the right place?
On 2012/12/21 03:31:22, aboxhall wrote: > On 2012/12/21 03:21:03, Dan Beam wrote: > > FYI: There are lots of test failures in the try jobs that have to do this with > > change (they all have runAccessibilityAudit in the stack). > > They all seem to be ChromeOS/Aura bots. > > Rahul, is there something I'm supposed to do to ensure that the new file is in > the right place? Oops, I spoke too soon; I see it just failed on mac_rel. Looking into it now.
On 2012/12/21 16:59:08, aboxhall wrote:
> On 2012/12/21 03:31:22, aboxhall wrote:
> > On 2012/12/21 03:21:03, Dan Beam wrote:
> > > FYI: There are lots of test failures in the try jobs that have to do this
> with
> > > change (they all have runAccessibilityAudit in the stack).
> >
> > They all seem to be ChromeOS/Aura bots.
> >
> > Rahul, is there something I'm supposed to do to ensure that the new file is
in
> > the right place?
>
> Oops, I spoke too soon; I see it just failed on mac_rel. Looking into it now.
The latest change fails on Triggerable(android_dbg) with this message:
obj/chrome/unit_tests.gen/chrome/test/data/unit/framework_unittest-gen.cc:12:
Failure
Value of: RunJavascriptTestF("FrameworkUnitTest", "ExpectTrueOk")
Actual: false
Expected: true
[ FAILED ] FrameworkUnitTest.ExpectTrueOk (6 ms)
[ RUN ] FrameworkUnitTest.AssertTrueOk
../../chrome/test/base/v8_unit_test.cc:72: Failure
Failed
/mnt/sdcard/third_party/accessibility-developer-tools/gen/axs_testing.js
obj/chrome/unit_tests.gen/chrome/test/data/unit/framework_unittest-gen.cc:17:
Failure
Value of: RunJavascriptTestF("FrameworkUnitTest", "AssertTrueOk")
Actual: false
Expected: true
[ FAILED ] FrameworkUnitTest.AssertTrueOk (6 ms)
It looks like it's not finding the library. Does anyone know who I should ask
about this?
Adding some Chrome/Android folks: yfriedman, jcivelli, dtrainor Can any of you help figure out why this change would be failing on Triggerable(android_dbg)?
On 2012/12/21 22:52:30, Dominic Mazzoni wrote: > Adding some Chrome/Android folks: yfriedman, jcivelli, dtrainor > > Can any of you help figure out why this change would be failing on > Triggerable(android_dbg)? Not 100% sure. I don't know too much about the test framework. But a quick git grep on mock4js (another lib that is added) showed an entry in build/android/pylib/single_test_runner.py that is responsible for pushing all test data to the device. I would add something like 'third_party/accessibility-developer-tools/gen/axs_testing.js' to that file (looks like the list is around line 121). Let me know if that works! :)
On 2012/12/21 23:23:03, dtrainor wrote: > On 2012/12/21 22:52:30, Dominic Mazzoni wrote: > > Adding some Chrome/Android folks: yfriedman, jcivelli, dtrainor > > > > Can any of you help figure out why this change would be failing on > > Triggerable(android_dbg)? > > Not 100% sure. I don't know too much about the test framework. But a quick git > grep on mock4js (another lib that is added) showed an entry in > build/android/pylib/single_test_runner.py that is responsible for pushing all > test data to the device. > > I would add something like > 'third_party/accessibility-developer-tools/gen/axs_testing.js' to that file > (looks like the list is around line 121). > > Let me know if that works! :) Looks like it worked! Thanks!
phajdan.jr, jhawkins: would you mind reviewing for OWNERS approval in chrome/test and chrome/browser/ui/webui?
https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); How is this different from AddLibrary, which also is 'FromPath' essentially? https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:45: expectEquals(this.expectedWarnings, nit: Braces required for mult-line blocks. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:253: } else { nit: No braces for single-line blocks. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:966: else nit: Braces required for multi-line blocks.
https://codereview.chromium.org/11363170/diff/79001/build/android/pylib/singl... File build/android/pylib/single_test_runner.py (right): https://codereview.chromium.org/11363170/diff/79001/build/android/pylib/singl... build/android/pylib/single_test_runner.py:162: 'third_party/accessibility-developer-tools/gen/axs_testing.js', nit: This should probably be sorted. https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); On 2013/01/03 21:23:39, James Hawkins wrote: > How is this different from AddLibrary, which also is 'FromPath' essentially? +1 . Please don't create superfluous public interfaces. Either standardize on relative paths or absolute paths, but providing both options is a recipe for mess. https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); Is this really needed? Again, standardize on _one_ way to indicate expected failures and stick to it, do not invent multiple ways.
https://codereview.chromium.org/11363170/diff/79001/build/android/pylib/singl... File build/android/pylib/single_test_runner.py (right): https://codereview.chromium.org/11363170/diff/79001/build/android/pylib/singl... build/android/pylib/single_test_runner.py:162: 'third_party/accessibility-developer-tools/gen/axs_testing.js', On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > nit: This should probably be sorted. Done. https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > On 2013/01/03 21:23:39, James Hawkins wrote: > > How is this different from AddLibrary, which also is 'FromPath' essentially? > > +1 . Please don't create superfluous public interfaces. Either standardize on > relative paths or absolute paths, but providing both options is a recipe for > mess. I don't think it's superfluous. Standardising on relative paths wouldn't work, since there are some paths that are relative to the test data dir and some that are relative to base::DIR_SOURCE_ROOT. Standardising on absolute paths would mean writing code similar to the implementation of this method wherever AddLibrary() is called. I agree the name is not terribly helpful - how about AddLibraryFromSourceRoot or similar? https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > Is this really needed? Again, standardize on _one_ way to indicate expected > failures and stick to it, do not invent multiple ways. Again, I'm not sure how this is redundant. What should I use instead?
https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); Either subclass and set testShouldFail to true or just put this.testShouldFail = true; as the first statement in the test. Then you can remove changes to this generator script. On 2013/01/03 22:11:22, aboxhall wrote: > On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > > Is this really needed? Again, standardize on _one_ way to indicate expected > > failures and stick to it, do not invent multiple ways. > > Again, I'm not sure how this is redundant. What should I use instead?
Oops, missed the nits last time. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:45: expectEquals(this.expectedWarnings, On 2013/01/03 21:23:39, James Hawkins wrote: > nit: Braces required for mult-line blocks. Done. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:45: expectEquals(this.expectedWarnings, On 2013/01/03 21:23:39, James Hawkins wrote: > nit: Braces required for mult-line blocks. Done. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:253: } else { On 2013/01/03 21:23:39, James Hawkins wrote: > nit: No braces for single-line blocks. Done. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:966: else On 2013/01/03 21:23:39, James Hawkins wrote: > nit: Braces required for multi-line blocks. Done. https://codereview.chromium.org/11363170/diff/79001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:966: else On 2013/01/03 21:23:39, James Hawkins wrote: > nit: Braces required for multi-line blocks. Done.
phajdan.jr, jhawkins: Would you be able to take another look at this?
https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); On 2013/01/03 22:11:22, aboxhall wrote: > On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > > On 2013/01/03 21:23:39, James Hawkins wrote: > > > How is this different from AddLibrary, which also is 'FromPath' essentially? > > > > +1 . Please don't create superfluous public interfaces. Either standardize on > > relative paths or absolute paths, but providing both options is a recipe for > > mess. > > I don't think it's superfluous. Standardising on relative paths wouldn't work, > since there are some paths that are relative to the test data dir and some that > are relative to base::DIR_SOURCE_ROOT. Standardising on absolute paths would > mean writing code similar to the implementation of this method wherever > AddLibrary() is called. > > I agree the name is not terribly helpful - how about AddLibraryFromSourceRoot or > similar? That's fine, but this method does not need to be on this object. Move it to unnamed namespace in the implementation file.
https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/79001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); On 2013/01/03 22:11:22, aboxhall wrote: > I don't think it's superfluous. Standardising on relative paths wouldn't work, > since there are some paths that are relative to the test data dir and some that > are relative to base::DIR_SOURCE_ROOT. Standardising on absolute paths would > mean writing code similar to the implementation of this method wherever > AddLibrary() is called. This effectively says to me, that standardizing wouldn't work since you've not standardized on a consistent solution. I think this is yet another point that it would be useful to be consistent. > I agree the name is not terribly helpful - how about AddLibraryFromSourceRoot or > similar? I don't think playing with the name of this method would be particularly helpful. One alternative I though about would be a helper somewhere else to build the DIR_SOURCE_ROOT or other-based paths for you. Just please leave only _one_ method here in the public interface.
Repeated some of the comments/tasks on latest patch. https://codereview.chromium.org/11363170/diff/91001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/91001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); As James said, change from member to anonymous namespaced function in the .cc file, possibly taking another param for the WebUIBrowserTest object so that it can call the public AddLibrary. https://codereview.chromium.org/11363170/diff/91001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/91001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); As suggested before, remove this change in favor of setting in your test file. https://codereview.chromium.org/11363170/diff/91001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/91001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:133: TEST_F('WebUIAccessibilityAuditBrowserTest', 'testWithAuditFailures_shouldFail', s/_shouldFail// and include this.testShouldFail = true in test body here and below.
https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest.js File chrome/test/base/js2gtest.js (right): https://codereview.chromium.org/11363170/diff/79001/chrome/test/base/js2gtest... chrome/test/base/js2gtest.js:156: testFunction.match(/_shouldFail$/); On 2013/01/03 22:18:53, scr wrote: > Either subclass and set testShouldFail to true or just put this.testShouldFail = > true; as the first statement in the test. > > Then you can remove changes to this generator script. > > On 2013/01/03 22:11:22, aboxhall wrote: > > On 2013/01/03 21:40:56, Paweł Hajdan Jr. wrote: > > > Is this really needed? Again, standardize on _one_ way to indicate expected > > > failures and stick to it, do not invent multiple ways. > > > > Again, I'm not sure how this is redundant. What should I use instead? > Putting this.testShouldFail = true as the first statement in the test doesn't work, since this looks at the prototype of the fixture and not the fixture itself. I have subclassed each fixture to make a version with testShouldFail = true. https://codereview.chromium.org/11363170/diff/91001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.h (right): https://codereview.chromium.org/11363170/diff/91001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.h:56: void AddLibraryFromPath(const FilePath& path); On 2013/01/04 19:02:55, scr wrote: > As James said, change from member to anonymous namespaced function in the .cc > file, possibly taking another param for the WebUIBrowserTest object so that it > can call the public AddLibrary. Done.
https://codereview.chromium.org/11363170/diff/94001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/94001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:86: ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &filePath)); nit: I'd prefer to make this helper return bool, have WARN_UNUSED_RESULT, and make the caller use ASSERT_TRUE. This should help with better error messages. Up to you, but I recommend it. https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:144: a11yErrors_: [], nit: In some places this uses a11y, and in some accessibility. Please standardize on one of these. https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:254: console.warn(report); nit: When you use {} above, shouldn't you also use them here? Or maybe drop from the one above?
https://codereview.chromium.org/11363170/diff/94001/chrome/browser/ui/webui/w... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): https://codereview.chromium.org/11363170/diff/94001/chrome/browser/ui/webui/w... chrome/browser/ui/webui/web_ui_browsertest.cc:86: ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &filePath)); On 2013/01/07 17:33:51, Paweł Hajdan Jr. wrote: > nit: I'd prefer to make this helper return bool, have WARN_UNUSED_RESULT, and > make the caller use ASSERT_TRUE. This should help with better error messages. Up > to you, but I recommend it. I think I'll leave it as-is for now; this function is used fairly seldom, so it shouldn't be too much of a problem to track down issues if they occur, and I don't think it's worth the extra verbosity at this stage. https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:144: a11yErrors_: [], On 2013/01/07 17:33:51, Paweł Hajdan Jr. wrote: > nit: In some places this uses a11y, and in some accessibility. Please > standardize on one of these. Internal-only names use "a11y" as "accessibility" is very long, and potentially causes things to spill over onto extra lines unnecessarily. "Accessibility" is used for external names as not everyone knows the abbreviation "a11y". https://codereview.chromium.org/11363170/diff/94001/chrome/test/data/webui/te... chrome/test/data/webui/test_api.js:254: console.warn(report); On 2013/01/07 17:33:51, Paweł Hajdan Jr. wrote: > nit: When you use {} above, shouldn't you also use them here? Or maybe drop from > the one above? Dropping {} above. Thanks for catching.
Is this looking ok?
LGTM with nits, but please wait for everyone else to LG. https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Don't add copyrights. https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:257: * Test fixture with |accessibilityIssuesAreErrors| set to false. nit: You use 'Tests' elsewhere, which is what should be used. Fix this up here and elsewhere.
https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... File chrome/test/data/webui/accessibility_audit_browsertest.js (right): https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/08 18:38:07, James Hawkins wrote: > Don't add copyrights. Done. https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/ac... chrome/test/data/webui/accessibility_audit_browsertest.js:257: * Test fixture with |accessibilityIssuesAreErrors| set to false. On 2013/01/08 18:38:07, James Hawkins wrote: > nit: You use 'Tests' elsewhere, which is what should be used. Fix this up here > and elsewhere. Done (not here though, because this is a test fixture).
phajdan.jr: ping?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/11363170/110001
Presubmit check for 11363170-110001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/test/data/webui/accessibility_audit_browsertest.js Presubmit checks took 3.6s to calculate.
@jhawkins Is this an out of date presubmit? On 2013/01/09 17:22:58, I haz the power (commit-bot) wrote: > Presubmit check for 11363170-110001 failed and returned exit status 1. > > > Running presubmit commit checks ... > > ** Presubmit Warnings ** > License must match: > .*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The > Chromium Authors\. All rights reserved\.\n.*? Use of this source code is > governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: > \*/)?\n > Found a bad license header in these files: > chrome/test/data/webui/accessibility_audit_browsertest.js > > Presubmit checks took 3.6s to calculate.
On Tue, Jan 8, 2013 at 10:38 AM, <jhawkins@chromium.org> wrote: > https://codereview.chromium.**org/11363170/diff/99001/** > chrome/test/data/webui/**accessibility_audit_**browsertest.js#newcode1<https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/accessibility_audit_browsertest.js#newcode1> > chrome/test/data/webui/**accessibility_audit_**browsertest.js:1: // > Copyright (c) 2012 The Chromium Authors. All rights reserved. > Don't add copyrights. > No, the latest email from mal@ seems to indicate that we should still add copyrights to all new files, we just shouldn't update the year every time we make a change to a file. The policy has changed so many times it's hard to keep track.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aboxhall@chromium.org/11363170/115002
On 2013/01/09 17:41:02, Dominic Mazzoni wrote: > On Tue, Jan 8, 2013 at 10:38 AM, <mailto:jhawkins@chromium.org> wrote: > > > https://codereview.chromium.**org/11363170/diff/99001/** > > > chrome/test/data/webui/**accessibility_audit_**browsertest.js#newcode1<https://codereview.chromium.org/11363170/diff/99001/chrome/test/data/webui/accessibility_audit_browsertest.js#newcode1> > > chrome/test/data/webui/**accessibility_audit_**browsertest.js:1: // > > Copyright (c) 2012 The Chromium Authors. All rights reserved. > > Don't add copyrights. > > > > No, the latest email from mal@ seems to indicate that we should still add > copyrights to all new files, we just shouldn't update the year every time > we make a change to a file. The policy has changed so many times it's hard > to keep track. Oh, my bad. I guess I did get Chromium's non-google3 policy mixed up.
Message was sent while issue was closed.
Change committed as 175866 |
