Adding a11y test coverage for passwords edit page.

ios/chrome/browser/ui/settings/settings_egtest.mm

Index: ios/chrome/browser/ui/settings/settings_egtest.mm
diff --git a/ios/chrome/browser/ui/settings/settings_egtest.mm b/ios/chrome/browser/ui/settings/settings_egtest.mm
index 8f28c35173b0f3b49c0cf8c668d2cb4e3adf05c7..e3efc9d0ff5f3a185ada40162656b3e6b13a3bea 100644
--- a/ios/chrome/browser/ui/settings/settings_egtest.mm
+++ b/ios/chrome/browser/ui/settings/settings_egtest.mm
@@ -123,17 +123,14 @@ id<GREYMatcher> ClearBrowsingDataCell() {
 id<GREYMatcher> SearchEngineButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_SEARCH_ENGINE_SETTING_TITLE);
 }

[baxley, 2017/02/17 15:38:00]

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?

[lindsayw, 2017/02/17 22:01:57]

Done.

-
 // Matcher for the Autofill Forms cell on the main Settings screen.
 id<GREYMatcher> AutofillButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_AUTOFILL);
 }
-
 // Matcher for the Google Apps cell on the main Settings screen.
 id<GREYMatcher> GoogleAppsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_GOOGLE_APPS_SM_SETTINGS);
 }
-
 // Matcher for the Google Chrome cell on the main Settings screen.
 id<GREYMatcher> GoogleChromeButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_PRODUCT_NAME);
@@ -166,6 +163,10 @@ id<GREYMatcher> TranslateSettingsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_TRANSLATE_SETTING);
 }
 
+// Matcher for the save button in the save password bar.
+id<GREYMatcher> savePasswordButton() {
+  return ButtonWithAccessibilityLabelId(IDS_IOS_PASSWORD_MANAGER_SAVE_BUTTON);
+}

[baxley, 2017/02/17 15:38:00]

nit: blank line?

[lindsayw, 2017/02/17 22:01:56]

Done.

 // Asserts that there is no cookie in current web state.
 void AssertNoCookieExists() {
   NSError* error = nil;
@@ -419,6 +420,41 @@ bool IsCertificateCleared() {
   [self closeSubSettingsMenu];
 }
 
+// Helper to load a page with a login and submit it.

[baxley, 2017/02/17 15:38:00]

nit:s/Helper to load/Loads

[lindsayw, 2017/02/17 22:01:57]

Done.

+- (void)loadAndSubmitTheForm {

[baxley, 2017/02/17 15:38:00]

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?

[lindsayw, 2017/02/17 22:01:57]

Done.

+  std::map<GURL, std::string> responses;
+  const GURL URL = web::test::HttpServer::MakeUrl("http://testClearPasswords");
+  // TODO(crbug.com/432596): There looks to be a bug where the save password
+  // infobar is not displayed if the action is about:blank.
+  responses[URL] =
+      "<form method=\"POST\" action=\"dest\">"
+      "Username:<input type=\"text\" name=\"username\" value=\"name\" /><br />"
+      "Password:<input type=\"password\""
+      "name=\"password\" value=\"pass\"/><br />"
+      "<input type=\"submit\" value=\"Login\" id=\"Login\"/>"
+      "</form>";
+  const GURL destinationURL =
+      web::test::HttpServer::MakeUrl("http://testClearPasswords/dest");
+  responses[destinationURL] = "Logged in!";
+  web::test::SetUpSimpleHttpServer(responses);
+  // Login to page and click to save password and check that its saved.
+  [ChromeEarlGrey loadURL:URL];
+  chrome_test_util::TapWebViewElementWithId("Login");
+  [[EarlGrey selectElementWithMatcher:savePasswordButton()]
+      performAction:grey_tap()];
+}
+
+// Helper to open the passwords page.

[baxley, 2017/02/17 15:38:00]

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.

[lindsayw, 2017/02/17 22:01:57]

Done.

+- (void)openPasswordSettings {
+  // Open settings and verify data in the view controller.
+  [ChromeEarlGreyUI openToolsMenu];
+  [[EarlGrey
+      selectElementWithMatcher:grey_accessibilityID(kToolsMenuSettingsId)]
+      performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:PasswordsButton()]
+      performAction:grey_tap()];
+}
+
 // Checks for a given service that it is both recording and uploading, where
 // appropriate.
 - (void)assertMetricsServiceEnabled:(MetricsServiceType)serviceType {
@@ -658,21 +694,6 @@ bool IsCertificateCleared() {
 // Verifies that logging into a form on a web page allows the user to save and
 // then clear a password.
 - (void)testClearPasswords {
-  std::map<GURL, std::string> responses;
-  const GURL URL = web::test::HttpServer::MakeUrl("http://testClearPasswords");
-  // TODO(crbug.com/432596): There looks to be a bug where the save password
-  // infobar is not displayed if the action is about:blank.
-  responses[URL] =
-      "<form method=\"POST\" action=\"dest\">"
-      "Username:<input type=\"text\" name=\"username\" value=\"name\" /><br />"
-      "Password:<input type=\"password\""
-      "name=\"password\" value=\"pass\"/><br />"
-      "<input type=\"submit\" value=\"Login\" id=\"Login\"/>"
-      "</form>";
-  const GURL destinationURL =
-      web::test::HttpServer::MakeUrl("http://testClearPasswords/dest");
-  responses[destinationURL] = "Logged in!";
-  web::test::SetUpSimpleHttpServer(responses);
 
   // Enable password management.
   ios::ChromeBrowserState* browserState =
@@ -704,12 +725,7 @@ bool IsCertificateCleared() {
   [self checkIfPasswordsSaved:NO];
 
   // Login to page and click to save password and check that its saved.
-  [ChromeEarlGrey loadURL:URL];
-  chrome_test_util::TapWebViewElementWithId("Login");
-  [[EarlGrey selectElementWithMatcher:ButtonWithAccessibilityLabelId(
-                                          IDS_IOS_PASSWORD_MANAGER_SAVE_BUTTON)]
-      performAction:grey_tap()];
-
+  [self loadAndSubmitTheForm];
   [self checkIfPasswordsSaved:YES];
 
   // Clear passwords and check that none are saved.
@@ -954,4 +970,34 @@ bool IsCertificateCleared() {
   [self closeSubSettingsMenu];
 }
 
+// Add a saved password and verify the edit password page is accessible.

[baxley, 2017/02/17 15:38:00]

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)

[lindsayw, 2017/02/17 22:01:56]

Done. "Verifies that saved passwords are accessible in Passwords page."

+- (void)testAccessibilityOnPasswordEditing {
+  [self clearPasswords];
+  [self checkIfPasswordsSaved:NO];
+
+  [self loadAndSubmitTheForm];
+  [self openPasswordSettings];
+
+  // Switch on edit mode.
+  [[EarlGrey selectElementWithMatcher:ButtonWithAccessibilityLabelId(
+                                          IDS_IOS_NAVIGATION_BAR_EDIT_BUTTON)]
+      performAction:grey_tap()];
+  chrome_test_util::VerifyAccessibilityForCurrentScreen();
+
+  // Exit settings.
+  [[EarlGrey
+      selectElementWithMatcher:grey_allOf(
+                                   grey_accessibilityID(@"ic_arrow_back"),
+                                   grey_accessibilityTrait(
+                                       UIAccessibilityTraitButton),
+                                   nil)] performAction:grey_tap()];
+
+  [[EarlGrey
+      selectElementWithMatcher:chrome_test_util::ButtonWithAccessibilityLabelId(
+                                   IDS_IOS_NAVIGATION_BAR_DONE_BUTTON)]

[baxley, 2017/02/17 15:38:00]

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.

[lindsayw, 2017/02/17 22:01:56]

Acknowledged.

+      performAction:grey_tap()];
+
+  [self clearPasswords];

[baxley, 2017/02/17 15:38:00]

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.

[lindsayw, 2017/02/17 22:01:56]

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.

+}
+
 @end