Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15086005:
Extend accessability browser tests to also verify policy toggling. (Closed)
|
Created:
7 years, 7 months ago by David Roche Modified:
7 years, 6 months ago Reviewers:
stevenjb, Mattias Nissler (ping if slow), zel, commit-bot: I haz the power, dmazzoni, yoshiki CC:
chromium-reviews, zork+watch_chromium.org, oshima+watch_chromium.org, 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, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExtend accessability browser tests to also verify policy toggling.
BUG=221543
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202723
Patch Set 1 #
Total comments: 9
Patch Set 2 : addressed review comments #Patch Set 3 : rebase #Patch Set 4 : fix post-rebase build error #Messages
Total messages: 13 (0 generated)
You'll need to run this by a more qualified reviewer for accessibility. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:39: using policy::PolicyMap; Why usually don't add using declarations for these in other tests - it doesn't help much. In fact, in this case you have only a single reference to each of these symbols, so using declarations don't seem justified. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:58: public WithParamInterface<PrefSettingMechanism> { I'm not sure whether it's a good idea essentially duplicate the entire test. Subclassing the fixture and doing and writing a more targeted test case might be easier to understand. I'm not feeling strong though, so please consult with a qualified reviewer for this file. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:87: prefs->CommitPendingWrite(); This shouldn't be necessary. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:95: base::RunLoop().RunUntilIdle(); #include "base/run_loop.h"
lgtm for accessibility - ok with me if an owner approves https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:58: public WithParamInterface<PrefSettingMechanism> { On 2013/05/13 08:27:44, Mattias Nissler wrote: > I'm not sure whether it's a good idea essentially duplicate the entire test. > Subclassing the fixture and doing and writing a more targeted test case might be > easier to understand. I'm not feeling strong though, so please consult with a > qualified reviewer for this file. Actually in this case it seems reasonable. The tests to run are exactly the same, just the pref setting mechanism changes. Subclassing would mean moving the body of the tests to helper functions, which would make the actual tests less readable. My only question is whether the resulting test name (i.e. what you'd specify in gtest_filter) would be easy to associate with the different test and param. For example, if a sheriff wanted to disable just one test for one param, how hard is that?
Yoshiki, can you take a look, since you wrote this file recently? https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... File chrome/browser/chromeos/system/tray_accessibility_browsertest.cc (right): https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:39: using policy::PolicyMap; On 2013/05/13 08:27:44, Mattias Nissler wrote: > Why usually don't add using declarations for these in other tests - it doesn't > help much. In fact, in this case you have only a single reference to each of > these symbols, so using declarations don't seem justified. Done. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:58: public WithParamInterface<PrefSettingMechanism> { On 2013/05/14 17:42:14, Dominic Mazzoni wrote: > On 2013/05/13 08:27:44, Mattias Nissler wrote: > > I'm not sure whether it's a good idea essentially duplicate the entire test. > > Subclassing the fixture and doing and writing a more targeted test case might > be > > easier to understand. I'm not feeling strong though, so please consult with a > > qualified reviewer for this file. > > Actually in this case it seems reasonable. The tests to run > are exactly the same, just the pref setting mechanism changes. > Subclassing would mean moving the body of the tests to helper > functions, which would make the actual tests less readable. > > My only question is whether the resulting test name > (i.e. what you'd specify in gtest_filter) would be > easy to associate with the different test and param. > For example, if a sheriff wanted to disable just one test for > one param, how hard is that? The test names look like so: TrayAccessibilityTestInstance/TrayAccessibilityTest.ShowMenu/0 TrayAccessibilityTestInstance/TrayAccessibilityTest.ShowMenu/1 and these can be used directly as test filters. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:87: prefs->CommitPendingWrite(); On 2013/05/13 08:27:44, Mattias Nissler wrote: > This shouldn't be necessary. Done. https://codereview.chromium.org/15086005/diff/1/chrome/browser/chromeos/syste... chrome/browser/chromeos/system/tray_accessibility_browsertest.cc:95: base::RunLoop().RunUntilIdle(); On 2013/05/13 08:27:44, Mattias Nissler wrote: > #include "base/run_loop.h" Done.
lgtm. Thanks for doing this!
Zel, can I get an OWNERS approval from you?
Steven, can I get an OWNERS approval for these changes?
rubberstamp LGTM
sorry, got buried in my inbox LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidroche@chromium.org/15086005/14001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidroche@chromium.org/15086005/32001
Message was sent while issue was closed.
Change committed as 202723 |
