Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
582493002:
Enable accessibility testing for the bookmark browser test. (Closed)
|
Created:
6 years, 3 months ago by hcarmona Modified:
5 years, 10 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, yuzo+watch_chromium.org, plundblad+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable accessibility testing for the bookmark browser test.
Updated the InProcessBrowserTest so that it can run accessibility tests.
Any browser test can enable the audit for a test by calling
EnableAccessibilityChecksForTestCase.
Added a test for the accessibility audit. This will ensure that the
InProcessBrowserTest's ability to do an accessibility audit doesn't
break. Added sample HTML to validate the accessibility test helper. This
HTML is used only in the browser test.
Enable accessibility testing for the Bookmarks browser test. No
additional changes were made to either the bookmark page or the bookmark
browser tests because the accessibility tests passed.
BUG=406152
Committed: https://crrev.com/67cce527f37cb11e351333f65b18d0e25b0d2304
Cr-Commit-Position: refs/heads/master@{#315487}
Patch Set 1 : Initial Upload #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 #Patch Set 8 : Update for Review #
Total comments: 20
Patch Set 9 : Apply Feedback #
Total comments: 25
Patch Set 10 : Applied Feedback #
Total comments: 12
Patch Set 11 : Applied Feedback #Patch Set 12 : Fix windows compile #Patch Set 13 : Forgotten file for win compile #Patch Set 14 : fix empty string check #
Total comments: 14
Patch Set 15 : Address nits #
Messages
Total messages: 26 (8 generated)
hcarmona@chromium.org changed reviewers: + aboxhall@chromium.org, jhawkins@chromium.org
Adding aboxhall@ to review changes related to a11y audit. This change allows the a11y audit to be enabled in tests that are written in c++ instead of js. jhawkins@, would you also be a good reviewer for this change? Thanks!
LGTM for accessibility audit additions. Looks great!
hcarmona@chromium.org changed reviewers: + phajdan.jr@chromium.org - jhawkins@chromium.org
Hi phajdan, Would you be the right person to review the changes in chrome/test? -Hector
https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper.h (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:30: bool LoadLibrary(content::WebContents* web_contents); nit: Please use WARN_UNUSED_RESULT, applies to other bool-returning methods in this class. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:40: std::string accessibility_message(); nit: If this is not inline (why?) it should not be using hacker_style name. Consider just inlining the accessor's body. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:44: std::string accessibility_message_; Is this member variable the only reason for making this a class? If so, please rather convert it to a function, I think that would be a better API. We can return bool and also an error string via out parameter. Instead of splitting LoadLibrary and RunAccessibilityTest, why not just return a different string error message? https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper_test.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper_test.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 Also since this file is a browser test, please use the standard _browsertest file name suffix. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:266: // A failure here means it is not possible to test for accessibility and it nit: All the comments in this method seem to just repeat what the code indicates. How about removing them? https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:151: // Disables running of accessibility tests for a particular test case. This is Do we really need this somewhat elaborate from API POV way to disable checks either for all test cases or just for a specific one? This doesn't seem used. Actually if one wants to disable checks for all test cases DisableAccessibilityChecks could be called from SetUp() method. https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... File chrome/test/data/accessibility_fail.html (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... chrome/test/data/accessibility_fail.html:5: Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... File chrome/test/data/accessibility_pass.html (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... chrome/test/data/accessibility_pass.html:5: Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: 2015
Applied feedback https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/29 13:01:18, Paweł Hajdan Jr. wrote: > nit: 2015 Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper.h (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: 2015 Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:30: bool LoadLibrary(content::WebContents* web_contents); On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: Please use WARN_UNUSED_RESULT, applies to other bool-returning methods in > this class. Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:40: std::string accessibility_message(); On 2015/01/29 13:01:18, Paweł Hajdan Jr. wrote: > nit: If this is not inline (why?) it should not be using hacker_style name. > > Consider just inlining the accessor's body. Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper.h:44: std::string accessibility_message_; On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > Is this member variable the only reason for making this a class? If so, please > rather convert it to a function, I think that would be a better API. > > We can return bool and also an error string via out parameter. Instead of > splitting LoadLibrary and RunAccessibilityTest, why not just return a different > string error message? That makes sense. I've moved these functions into the InProcessBrowserTest class and got rid of these files. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... File chrome/test/base/accessibility_test_helper_test.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/access... chrome/test/base/accessibility_test_helper_test.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: 2015 > > Also since this file is a browser test, please use the standard _browsertest > file name suffix. Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:266: // A failure here means it is not possible to test for accessibility and it On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: All the comments in this method seem to just repeat what the code > indicates. How about removing them? Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:151: // Disables running of accessibility tests for a particular test case. This is On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > Do we really need this somewhat elaborate from API POV way to disable checks > either for all test cases or just for a specific one? This doesn't seem used. > > Actually if one wants to disable checks for all test cases > DisableAccessibilityChecks could be called from SetUp() method. This is based on the a11y audit from js: src/chrome/test/data/webui/test_api.js I added the ability to set a class level ON/OFF and test level ON/OFF in order to mirror that API. I am not opposed to simplifying the interface, but I feel that having these methods makes the user's intentions more explicit. If we want to simplify the API, we can get rid of set_run_accessibility_checks but I think it makes sense to keep both EnableA11yChecks and DisableA11yChecks so that all checks can be enabled in SetUp but individual tests can still be disabled. I have seen cases where all tests in a test suite will pass the a11y audit except for one, but it can't be fixed immediately for some reason (3rd party lib). In those cases it is necessary to disable just one test. https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... File chrome/test/data/accessibility_fail.html (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... chrome/test/data/accessibility_fail.html:5: Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: 2015 Done. https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... File chrome/test/data/accessibility_pass.html (right): https://codereview.chromium.org/582493002/diff/130001/chrome/test/data/access... chrome/test/data/accessibility_pass.html:5: Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/01/29 13:01:19, Paweł Hajdan Jr. wrote: > nit: 2015 Done.
hcarmona@chromium.org changed reviewers: + jcivelli@chromium.org
jcivelli, would you be able to review this CL since phajdan is ooo? Thanks, Hector
Your CL description shows up weird. Are lines 72 char max? Here are the comments/questions from my first pass. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:116: const base::FilePath kAXSTesting( Style guide recommends to use const char[] as constants. Same for kExpectedAccessibilityResults and kAccessibilityTestString. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:148: " pass = false;" The code would be shorter if you simply called domAutomationController.send(axs.Audit.createReport(result)); and return here. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:308: void InProcessBrowserTest::RunAccessibilityChecks(std::string *test_result) { std::string* https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:109: static const std::string kExpectedAccessibilityResults; Does it need to be part of the class? https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:158: void DisableAccessibilityChecks(); How about having only 1 method? SetAccessibilityCheckEnabled(bool enabled) https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:197: void set_run_accessibility_checks(bool run_accessibility_checks) { I am a bit confused by this. You can either enable it always (in which case RunAccessibilityChecks is called by the test framework for you for all test cases) or on a test case bases by calling RunAccessibilityChecks from your test. Did I get it right? https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:202: void RunAccessibilityChecks(std::string *test_result); std::string* https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:217: // Returns false on loading error. Accessibility audit will fail if library is Should it just assert if it fails to load? https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:94: * InProcessBrowserTest. These test do NOT validate the accessibility audit, Typo: These tests https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:100: base::FilePath BuildURLToFile(const base::FilePath file_path) { const base::FilePath& https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:107: bool NavigateToURL(base::FilePath address) { const base::FilePath& https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:126: RunAccessibilityChecks(&test_result); Would it make sense for RunAccessibilityChecks to also return false when it fails? (and it would accept nullptr as a param, so if you are not interested in the test result, the check becomes a one-liner. And then you don't need to expose kExpectedAccessibilityResults).
Applied feedback and updated the CL description. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:116: const base::FilePath kAXSTesting( On 2015/02/05 23:29:09, Jay Civelli wrote: > Style guide recommends to use const char[] as constants. > Same for kExpectedAccessibilityResults and kAccessibilityTestString. Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:148: " pass = false;" On 2015/02/05 23:29:09, Jay Civelli wrote: > The code would be shorter if you simply called > domAutomationController.send(axs.Audit.createReport(result)); and return here. Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:308: void InProcessBrowserTest::RunAccessibilityChecks(std::string *test_result) { On 2015/02/05 23:29:09, Jay Civelli wrote: > std::string* Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:109: static const std::string kExpectedAccessibilityResults; On 2015/02/05 23:29:09, Jay Civelli wrote: > Does it need to be part of the class? With the other change to make the parameter for the RunAccessibilityChecks into an error message it's no longer necessary to have this constant at all. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:158: void DisableAccessibilityChecks(); On 2015/02/05 23:29:09, Jay Civelli wrote: > How about having only 1 method? > SetAccessibilityCheckEnabled(bool enabled) I think it makes sense. Collapsed both of these into 1 method. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:197: void set_run_accessibility_checks(bool run_accessibility_checks) { On 2015/02/05 23:29:09, Jay Civelli wrote: > I am a bit confused by this. > You can either enable it always (in which case RunAccessibilityChecks is called > by the test framework for you for all test cases) or on a test case bases by > calling RunAccessibilityChecks from your test. > Did I get it right? > I think you're right and this is too confusing. I've simplified this a bit by only having one method now. This method can be called in SetUpOnMainThread to enable a11y audit for all tests but can still be called from the test body to enable/disable only one test if necessary. A test shouldn't need to call RunAccessibilityChecks because they can just enable it. It's here so that it can be tested in InProcessAccessibilityBrowsertest. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:202: void RunAccessibilityChecks(std::string *test_result); On 2015/02/05 23:29:09, Jay Civelli wrote: > std::string* Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:217: // Returns false on loading error. Accessibility audit will fail if library is On 2015/02/05 23:29:09, Jay Civelli wrote: > Should it just assert if it fails to load? Only called one place. Moved this method's contents into RunAccessibilityChecks. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:94: * InProcessBrowserTest. These test do NOT validate the accessibility audit, On 2015/02/05 23:29:09, Jay Civelli wrote: > Typo: These tests Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:100: base::FilePath BuildURLToFile(const base::FilePath file_path) { On 2015/02/05 23:29:10, Jay Civelli wrote: > const base::FilePath& Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:107: bool NavigateToURL(base::FilePath address) { On 2015/02/05 23:29:09, Jay Civelli wrote: > const base::FilePath& Done. https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:126: RunAccessibilityChecks(&test_result); On 2015/02/05 23:29:10, Jay Civelli wrote: > Would it make sense for RunAccessibilityChecks to also return false when it > fails? (and it would accept nullptr as a param, so if you are not interested in > the test result, the check becomes a one-liner. And then you don't need to > expose kExpectedAccessibilityResults). Awesome idea. Done.
https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/150001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:197: void set_run_accessibility_checks(bool run_accessibility_checks) { On 2015/02/06 22:50:53, Hector Carmona wrote: > On 2015/02/05 23:29:09, Jay Civelli wrote: > > I am a bit confused by this. > > You can either enable it always (in which case RunAccessibilityChecks is > called > > by the test framework for you for all test cases) or on a test case bases by > > calling RunAccessibilityChecks from your test. > > Did I get it right? > > > > I think you're right and this is too confusing. I've simplified this a bit by > only having one method now. This method can be called in SetUpOnMainThread to > enable a11y audit for all tests but can still be called from the test body to > enable/disable only one test if necessary. > > A test shouldn't need to call RunAccessibilityChecks because they can just > enable it. It's here so that it can be tested in > InProcessAccessibilityBrowsertest. This looks much better, thanks! https://codereview.chromium.org/582493002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bookmarks_ui_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/bookmarks_ui_browsertest.cc:24: }; Nit: no comma needed after the } https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:144: " domAutomationController.send(axs.Audit.createReport(result));" Don't you need to return in that case? https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:298: *error_message = "browser is NULL"; Do we want to support |error_message| potentially be NULL? I wonder if using ASSERT for these was not a better approach. Arguably when you run this is to get the a11y errors and it's expected that you have an active browser/tab/frame. https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:343: return !error_message->compare(""); Shouldn't we just return true? If someone called this with a non empty |error_message| then this would return false when it should not. https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:189: // Will run accessibility checks and set an error message if it fails. Nit: Will run -> Runs
https://codereview.chromium.org/582493002/diff/170001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bookmarks_ui_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/browser/ui/webui... chrome/browser/ui/webui/bookmarks_ui_browsertest.cc:24: }; On 2015/02/07 01:11:12, Jay Civelli wrote: > Nit: no comma needed after the } Done. https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:144: " domAutomationController.send(axs.Audit.createReport(result));" On 2015/02/07 01:11:12, Jay Civelli wrote: > Don't you need to return in that case? Yes, now I'm wondering why this didn't fail on my machine when I ran it... Maybe the messages are appended? Either way, this needs to return here. Done https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:298: *error_message = "browser is NULL"; On 2015/02/07 01:11:12, Jay Civelli wrote: > Do we want to support |error_message| potentially be NULL? The error the audit returns is very descriptive about what elements are not accessible and for what reason. Having |error_message| not populate would make finding the cause of the error harder. > I wonder if using ASSERT for these was not a better approach. Arguably when you > run this is to get the a11y errors and it's expected that you have an active > browser/tab/frame. Should we just assume that all tests will have a browser/tab/frame? If a browser test run is missing one or more of these does that mean it's in a bad state? We can also move the asserts up a level to where this method is called and pass in |web_contents| and |focus_frame| https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:343: return !error_message->compare(""); On 2015/02/07 01:11:12, Jay Civelli wrote: > Shouldn't we just return true? If someone called this with a non empty > |error_message| then this would return false when it should not. We can't return true because running the audit populates |error_message| and we don't know if the test passed or failed until we check if it's empty. A non-empty |error_message| will be cleared because running the audit will overwrite it. https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.h:189: // Will run accessibility checks and set an error message if it fails. On 2015/02/07 01:11:12, Jay Civelli wrote: > Nit: Will run -> Runs Done.
Please run the trybots. Could you also format the CL message to 72 char lines? https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:343: return !error_message->compare(""); On 2015/02/07 02:22:12, Hector Carmona wrote: > On 2015/02/07 01:11:12, Jay Civelli wrote: > > Shouldn't we just return true? If someone called this with a non empty > > |error_message| then this would return false when it should not. > > We can't return true because running the audit populates |error_message| and we > don't know if the test passed or failed until we check if it's empty. > > A non-empty |error_message| will be cleared because running the audit will > overwrite it. Oh, right. Nit: you could do return !error_message->empty();
Updated description to not exceed 72 lines and ran try-bots. There was a compile issue on windows that should be fixed now. https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/170001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:343: return !error_message->compare(""); On 2015/02/07 02:36:35, Jay Civelli wrote: > On 2015/02/07 02:22:12, Hector Carmona wrote: > > On 2015/02/07 01:11:12, Jay Civelli wrote: > > > Shouldn't we just return true? If someone called this with a non empty > > > |error_message| then this would return false when it should not. > > > > We can't return true because running the audit populates |error_message| and > we > > don't know if the test passed or failed until we check if it's empty. > > > > A non-empty |error_message| will be cleared because running the audit will > > overwrite it. > Oh, right. > Nit: you could do return !error_message->empty(); Done.
lgtm
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm just some nits https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:141: " 'lowContrastElements'];" nit: ] on new line https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:147: " }" nit: no curlies https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:149: " return '';" nit: I think this could just be a for + break; https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:528: run_accessibility_checks_for_test_case_ = false; why not put this in the initializer list? coverity will whine about uninitialized members and it'll handle this case https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:97: class InProcessAccessibilityBrowsertest : public InProcessBrowserTest { Browsertest -> BrowserTest https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:122: nit: remove \n at top https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:135: same
The CQ bit was checked by hcarmona@chromium.org
The CQ bit was unchecked by hcarmona@chromium.org
New patchsets have been uploaded after l-g-t-m from jcivelli@chromium.org,dbeam@chromium.org
Address nits https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:141: " 'lowContrastElements'];" On 2015/02/10 01:13:38, Dan Beam wrote: > nit: ] on new line Done. https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:147: " }" On 2015/02/10 01:13:38, Dan Beam wrote: > nit: no curlies Curlies stay b/c of adding break https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:149: " return '';" On 2015/02/10 01:13:38, Dan Beam wrote: > nit: I think this could just be a for + break; Done. https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:528: run_accessibility_checks_for_test_case_ = false; On 2015/02/10 01:13:38, Dan Beam wrote: > why not put this in the initializer list? coverity will whine about > uninitialized members and it'll handle this case Initialized to make coverity happy, but also kept here because it needs to be reset between tests. https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test_browsertest.cc (right): https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:97: class InProcessAccessibilityBrowsertest : public InProcessBrowserTest { On 2015/02/10 01:13:39, Dan Beam wrote: > Browsertest -> BrowserTest Done. https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:122: On 2015/02/10 01:13:39, Dan Beam wrote: > nit: remove \n at top Done. https://codereview.chromium.org/582493002/diff/250001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test_browsertest.cc:135: On 2015/02/10 01:13:38, Dan Beam wrote: > same Done.
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582493002/270001
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/67cce527f37cb11e351333f65b18d0e25b0d2304 Cr-Commit-Position: refs/heads/master@{#315487} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
