Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2661473004:
Adding a11y test coverage for passwords edit page. (Closed)
|
Created:
3 years, 10 months ago by lindsayw Modified:
3 years, 10 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, gcasto+watchlist_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a11y test coverage for passwords edit page.
BUG=688429
Review-Url: https://codereview.chromium.org/2661473004
Cr-Commit-Position: refs/heads/master@{#452723}
Committed: https://chromium.googlesource.com/chromium/src/+/852ef153fa77826490053afe65ba63be0957b75f
Patch Set 1 #Patch Set 2 : Updated test and prepped the cleanup tasks #Patch Set 3 : Removing unneeded file. #Patch Set 4 : Updated to match settings test. #Patch Set 5 : Remove extra pass of clearing passwords #Patch Set 6 : Added a todo #
Total comments: 3
Patch Set 7 : Move test to settings_egtest.mm. #
Total comments: 18
Patch Set 8 : Addressed comments. #
Total comments: 4
Patch Set 9 : Addressed all remaining comments. #
Total comments: 10
Patch Set 10 : Addressed all comments. #
Total comments: 3
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Adding test coverage for passwords edit page. BUG= ========== to ========== Adding a11y test coverage for passwords edit page. BUG= ==========
Description was changed from ========== Adding a11y test coverage for passwords edit page. BUG= ========== to ========== Adding a11y test coverage for passwords edit page. BUG=688429 ==========
lindsayw@chromium.org changed reviewers: + baxley@chromium.org, rohitrao@chromium.org
I have a few high level questions first. I'd rather limit the code duplication, if possible. I'm also okay if we do a follow up CL right away (or in parallel), if that makes the code and reviews simpler. Let me know what you think! https://codereview.chromium.org/2661473004/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/password_settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/password_settings_egtest.mm:38: id<GREYMatcher> ClearCookiesButton() { High level question, it looks that a lot of these matchers are already in settings_egtest.mm I'd rather not have all of this duplicated logic. How would it look if we moved testClearPasswords (and it's helper clearPasswords, or just use yours) to this file? So then all the password tests would live here, and make things simpler? An alternative is to move the matchers to shared utilities, but I don't know if that's necessary. There is duplication because we have password tests in multiple files, not because we have multiple themed tests accessing this. WDYT? https://codereview.chromium.org/2661473004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/password_settings_egtest.mm:99: Could you add a comment as to why we have to uncheck everything, then re-check them? https://codereview.chromium.org/2661473004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/password_settings_egtest.mm:223: - (void)testAccessibilityOnPasswordEditing { nit: could you have all the test methods next to each other? Perhaps marked with a #pragma? You can look at history_ui_egtests.mm as an example. It also puts the helper methods into the interface.
PTAL
https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:125: } Thank you for the clean up, outside of your code! There are about 10 other matchers above this, would you mind fixing the spacing there too? https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:169: } nit: blank line? https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:423: // Helper to load a page with a login and submit it. nit:s/Helper to load/Loads https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:424: - (void)loadAndSubmitTheForm { Would it be okay to call it loadFormAndLogin ? "And" isn't great to use, but would it be more clear to say Login, than Submit? https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:447: // Helper to open the passwords page. nit: "Opens the passwords page. It requires no menus to be open." If there are other constraints (NTP, web view, ...), feel free to include that in the comment. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:968: performAction:grey_tap()]; Could we make this test match the new test you wrote? I don't feel super strongly about having the helper to open the password settings, but I think it should be done both ways. Up to you what to do: 1. Delete the previous 3 commands and replace with the openPasswordSettings() 2. Delete the helper and call the previous 3 lines in the next test. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:973: // Add a saved password and verify the edit password page is accessible. nit: should start with an active verb. What about something like this? (feel free to make it sound better) "Tests that saved passwords are accessible in Password Settings". (I'm not sure about capitalization) https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:997: IDS_IOS_NAVIGATION_BAR_DONE_BUTTON)] I filed a bug to create a custom matcher (crbug.com/693579). It's used about 50 times! in our tests, so definitely not part of this CL. This is just FYI, no action needed on your part in this CL, or any follow up. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:1000: [self clearPasswords]; Is this safe to call if there are no saved passwords? If so, can we call setTearDownHandler with this? Doing that will guarantee this method gets called. If some assertion above (i.e. VerifyAccessibilityOnCurrentScreen()) fails, then it exits and doesn't execute this part of the test.
PTAL https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:125: } On 2017/02/17 15:38:00, baxley wrote: > Thank you for the clean up, outside of your code! > > There are about 10 other matchers above this, would you mind fixing the spacing > there too? Done. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:169: } On 2017/02/17 15:38:00, baxley wrote: > nit: blank line? Done. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:423: // Helper to load a page with a login and submit it. On 2017/02/17 15:38:00, baxley wrote: > nit:s/Helper to load/Loads Done. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:424: - (void)loadAndSubmitTheForm { On 2017/02/17 15:38:00, baxley wrote: > Would it be okay to call it loadFormAndLogin ? > > "And" isn't great to use, but would it be more clear to say Login, than Submit? Done. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:447: // Helper to open the passwords page. On 2017/02/17 15:38:00, baxley wrote: > nit: "Opens the passwords page. It requires no menus to be open." > > If there are other constraints (NTP, web view, ...), feel free to include that > in the comment. Done. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:968: performAction:grey_tap()]; On 2017/02/17 15:38:00, baxley wrote: > Could we make this test match the new test you wrote? I don't feel super > strongly about having the helper to open the password settings, but I think it > should be done both ways. Up to you what to do: > 1. Delete the previous 3 commands and replace with the openPasswordSettings() > 2. Delete the helper and call the previous 3 lines in the next test. Done, Option 1. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:973: // Add a saved password and verify the edit password page is accessible. On 2017/02/17 15:38:00, baxley wrote: > nit: should start with an active verb. What about something like this? (feel > free to make it sound better) > > "Tests that saved passwords are accessible in Password Settings". > > (I'm not sure about capitalization) Done. "Verifies that saved passwords are accessible in Passwords page." https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:997: IDS_IOS_NAVIGATION_BAR_DONE_BUTTON)] On 2017/02/17 15:38:00, baxley wrote: > I filed a bug to create a custom matcher (crbug.com/693579). > > It's used about 50 times! in our tests, so definitely not part of this CL. > > This is just FYI, no action needed on your part in this CL, or any follow up. Acknowledged. https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:1000: [self clearPasswords]; On 2017/02/17 15:38:00, baxley wrote: > Is this safe to call if there are no saved passwords? If so, can we call > setTearDownHandler with this? > > Doing that will guarantee this method gets called. If some assertion above (i.e. > VerifyAccessibilityOnCurrentScreen()) fails, then it exits and doesn't execute > this part of the test. No, clearPasswords has the limitation discussed in crbug/689663 where an improvement needs to be made to first eval the settings and only set it for clearing if it's not already set, then go back and make sure all settings are left in the default state. As such, I think usiI added the tearDown to this method, but putting clearPasswords at the beginning of it dchecks and at the end of it causes an error because of the dismissSettings call. Please let me know what you think.
On 2017/02/17 22:01:57, lindsayw wrote: > PTAL > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > File ios/chrome/browser/ui/settings/settings_egtest.mm (right): > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:125: } > On 2017/02/17 15:38:00, baxley wrote: > > Thank you for the clean up, outside of your code! > > > > There are about 10 other matchers above this, would you mind fixing the > spacing > > there too? > > Done. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:169: } > On 2017/02/17 15:38:00, baxley wrote: > > nit: blank line? > > Done. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:423: // Helper to load a page > with a login and submit it. > On 2017/02/17 15:38:00, baxley wrote: > > nit:s/Helper to load/Loads > > Done. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:424: - > (void)loadAndSubmitTheForm { > On 2017/02/17 15:38:00, baxley wrote: > > Would it be okay to call it loadFormAndLogin ? > > > > "And" isn't great to use, but would it be more clear to say Login, than > Submit? > > Done. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:447: // Helper to open the > passwords page. > On 2017/02/17 15:38:00, baxley wrote: > > nit: "Opens the passwords page. It requires no menus to be open." > > > > If there are other constraints (NTP, web view, ...), feel free to include that > > in the comment. > > Done. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:968: > performAction:grey_tap()]; > On 2017/02/17 15:38:00, baxley wrote: > > Could we make this test match the new test you wrote? I don't feel super > > strongly about having the helper to open the password settings, but I think it > > should be done both ways. Up to you what to do: > > 1. Delete the previous 3 commands and replace with the openPasswordSettings() > > 2. Delete the helper and call the previous 3 lines in the next test. > > Done, Option 1. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:973: // Add a saved password > and verify the edit password page is accessible. > On 2017/02/17 15:38:00, baxley wrote: > > nit: should start with an active verb. What about something like this? (feel > > free to make it sound better) > > > > "Tests that saved passwords are accessible in Password Settings". > > > > (I'm not sure about capitalization) > > Done. "Verifies that saved passwords are accessible in Passwords page." > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:997: > IDS_IOS_NAVIGATION_BAR_DONE_BUTTON)] > On 2017/02/17 15:38:00, baxley wrote: > > I filed a bug to create a custom matcher (crbug.com/693579). > > > > It's used about 50 times! in our tests, so definitely not part of this CL. > > > > This is just FYI, no action needed on your part in this CL, or any follow up. > > Acknowledged. > > https://codereview.chromium.org/2661473004/diff/120001/ios/chrome/browser/ui/... > ios/chrome/browser/ui/settings/settings_egtest.mm:1000: [self clearPasswords]; > On 2017/02/17 15:38:00, baxley wrote: > > Is this safe to call if there are no saved passwords? If so, can we call > > setTearDownHandler with this? > > > > Doing that will guarantee this method gets called. If some assertion above > (i.e. > > VerifyAccessibilityOnCurrentScreen()) fails, then it exits and doesn't execute > > this part of the test. > > No, clearPasswords has the limitation discussed in crbug/689663 where an > improvement needs to be made to first eval the settings and only set it for > clearing if it's not already set, then go back and make sure all settings are > left in the default state. As such, I think usiI added the tearDown to this > method, but putting clearPasswords at the beginning of it dchecks and at the end > of it causes an error because of the dismissSettings call. Please let me know > what you think. Oops, this should read: I added the tearDown to this method, but putting clearPasswords at the beginning of the teardown dchecks and at the end of it causes an error because of the dismissSettings call. Please let me know what you think.
I put in two comments, mostly just to capture what we discussed. Feel free to send this to Yuke/Menglu, or just look at it with Rohit. Thanks! https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:991: }]; Is this the same as in the method above? Should we move it to a shared method since it's non-trivial? https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:1014: [self clearPasswords]; Similar to last review, it'd be nice if we could do this in the teardown handler, since this is teardown code that we want to execute if something fails before the test gets here, so we're not stuck with saved passwords for other tests. I know we talked about it offline, but we didn't have time to debug. If there's no way to make it work, then I wouldn't block this, but we should look a little more.
https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:991: }]; On 2017/02/18 06:02:12, baxley wrote: > Is this the same as in the method above? Should we move it to a shared method > since it's non-trivial? Done. https://codereview.chromium.org/2661473004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:1014: [self clearPasswords]; On 2017/02/18 06:02:12, baxley wrote: > Similar to last review, it'd be nice if we could do this in the teardown > handler, since this is teardown code that we want to execute if something fails > before the test gets here, so we're not stuck with saved passwords for other > tests. > > I know we talked about it offline, but we didn't have time to debug. If there's > no way to make it work, then I wouldn't block this, but we should look a little > more. I was finally able to get this to work, not sure what was going wrong before, I think it could have just been an issue with my client or xcode, but it's all working and in now.
liaoyuke@chromium.org changed reviewers: + liaoyuke@chromium.org
https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:404: - (void)passwordsTearDown { Some comments can be used to explain the intent of this function. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:412: [self clearPasswords]; Putting code and comment next to each other is confusing. How about adding a line break here: [self clearPasswords]; // Restore the password management pref state. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:418: // Restore the Clear Browsing Data checkmarks prefs to their default state. These are not specifically for password, could you please put these in a separate function? And by the way, these are also used in testClearCookies, so do you mind cleaning them up? https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:463: web::test::SetUpSimpleHttpServer(responses); Could you please add some line breaks to this function to improve readability? https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:973: [self enablePasswordManagement]; |enablePasswordManagement| changes the value of |kPasswordManagerSavingEnabled| to true, so it seems to me that you will lose the original value of |kPasswordManagerSavingEnabled|. I think you should assign the original value of |kPasswordManagerSavingEnabled| to some local variable and pass the variable as a parameter to |passwordsTearDown|.
https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:404: - (void)passwordsTearDown { On 2017/02/23 07:10:11, liaoyuke wrote: > Some comments can be used to explain the intent of this function. Done. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:412: [self clearPasswords]; On 2017/02/23 07:10:11, liaoyuke wrote: > Putting code and comment next to each other is confusing. How about adding a > line break here: > [self clearPasswords]; > > // Restore the password management pref state. Done. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:418: // Restore the Clear Browsing Data checkmarks prefs to their default state. On 2017/02/23 07:10:11, liaoyuke wrote: > These are not specifically for password, could you please put these in a > separate function? And by the way, these are also used in testClearCookies, so > do you mind cleaning them up? Done. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:463: web::test::SetUpSimpleHttpServer(responses); On 2017/02/23 07:10:11, liaoyuke wrote: > Could you please add some line breaks to this function to improve readability? Done. https://codereview.chromium.org/2661473004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:973: [self enablePasswordManagement]; On 2017/02/23 07:10:11, liaoyuke wrote: > |enablePasswordManagement| changes the value of |kPasswordManagerSavingEnabled| > to true, so it seems to me that you will lose the original value of > |kPasswordManagerSavingEnabled|. I think you should assign the original value of > |kPasswordManagerSavingEnabled| to some local variable and pass the variable as > a parameter to |passwordsTearDown|. Done.
lgtm with nits. Thank you for adding accessibility tests to our code base Lindsay! https://codereview.chromium.org/2661473004/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:789: [self restoreClearBrowsingDataCheckmarksToDefault]; Thank you for cleaning this up, outside of your code! https://codereview.chromium.org/2661473004/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:978: PrefService* preferences = browserState->GetPrefs(); nits: can you add a separate function this this piece of code, just like what you did in |enablePasswordManagement|. browserState is a very low level concept, and shouldn't be exposed to tests directly, so it's better to encapsulate it inside a function.
lgtm https://codereview.chromium.org/2661473004/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2661473004/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:978: PrefService* preferences = browserState->GetPrefs(); On 2017/02/23 17:59:11, liaoyuke wrote: > nits: can you add a separate function this this piece of code, just like what > you did in |enablePasswordManagement|. browserState is a very low level concept, > and shouldn't be exposed to tests directly, so it's better to encapsulate it > inside a function. I see EG tests as grey-box tests, so I'm fine with them knowing about things like BrowserState.
The CQ bit was checked by lindsayw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487901907468850,
"parent_rev": "052c11832f70f11fb220abb005bcf93143d2e4e8", "commit_rev":
"852ef153fa77826490053afe65ba63be0957b75f"}
Message was sent while issue was closed.
Description was changed from ========== Adding a11y test coverage for passwords edit page. BUG=688429 ========== to ========== Adding a11y test coverage for passwords edit page. BUG=688429 Review-Url: https://codereview.chromium.org/2661473004 Cr-Commit-Position: refs/heads/master@{#452723} Committed: https://chromium.googlesource.com/chromium/src/+/852ef153fa77826490053afe65ba... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/852ef153fa77826490053afe65ba... |
