New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 744058 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
hobby only
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocking:
issue 717163



Sign in to add a comment

Improve passwords EG tests

Project Member Reported by vabr@chromium.org, Jul 16 2017

Issue description

ios/chrome/browser/ui/settings/passwords_settings_egtest.mm could use a number of improvements:
* consistently integrated scrolling for short viewports (see  bug 717163 )
* separating layout constraints from button matchers into dedicated tests to make those matchers useful and light-weight in scenarios when layout checking is undesirable
* cleaning up the test fixture by separating helpers not dependent on the fixture to functions in the anonymous namespace
* use ScopedFeatureList instead of meddling with defaults to ensure the viewing feature is enabled
 

Comment 2 by vabr@chromium.org, Jul 16 2017

Three more + one additional planned to move passwords-related tests from settings_egtest.mm

https://chromium-review.googlesource.com/c/573540/ 
https://chromium-review.googlesource.com/c/573541/ 
https://chromium-review.googlesource.com/c/573542/ 

Comment 3 by vabr@chromium.org, Jul 16 2017

The change to settings_egtest.mm is in https://chromium-review.googlesource.com/c/572981/
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f09df8b94bb75fa6003ef7d385a251ce867ea39

commit 8f09df8b94bb75fa6003ef7d385a251ce867ea39
Author: Vaclav Brozek <vabr@chromium.org>
Date: Mon Jul 17 16:36:26 2017

Don't try to enable saving password in EG tests

The password settings EG tests currently try to enable saving passwords in
PrefService. This is not necesary, because saving passwords is enabled by
default (see PasswordManager::RegisterProfilePrefs).

It is also harmful, because it:
(1) Won't catch if the default setting being "enabled" regresses, or some other
    tests start failing to clean-up.
(2) Increases the complexity of the code.

Therefore, this CL removes the code which enables the password setting in the
passwords settings EG tests.

Bug:  744058 
Change-Id: I315935319cbaacb871ba6214b832c04902afffa9
Reviewed-on: https://chromium-review.googlesource.com/572903
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487109}
[modify] https://crrev.com/8f09df8b94bb75fa6003ef7d385a251ce867ea39/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1e3e182c4e133d7e635a9a4ddcdc5e54470578f

commit b1e3e182c4e133d7e635a9a4ddcdc5e54470578f
Author: Vaclav Brozek <vabr@chromium.org>
Date: Mon Jul 17 18:29:44 2017

Control viewing feature in EG tests through ScopedFeatureList

Currently, the passwords settings EG tests enable the "viewing passwords"
feature through NSDefaults (=experimental flags). Since recently, controlling
through a base::Feature is also possible.

This CL gets rid of the NSDefaults code, which needs to reset the state
manually with a simpler ScopedFeatureList, which limits the boilerplate.

Bug:  744058 
Change-Id: I594fb4bbd3f90dd81a1f0f35c7f1ccb70aee965c
Reviewed-on: https://chromium-review.googlesource.com/573023
Reviewed-by: Louis Romero <lpromero@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487168}
[modify] https://crrev.com/b1e3e182c4e133d7e635a9a4ddcdc5e54470578f/ios/chrome/browser/ui/settings/BUILD.gn
[modify] https://crrev.com/b1e3e182c4e133d7e635a9a4ddcdc5e54470578f/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/857e08190fa0f2b4a031b1ef98c74d4eebb52bc8

commit 857e08190fa0f2b4a031b1ef98c74d4eebb52bc8
Author: Vaclav Brozek <vabr@chromium.org>
Date: Wed Jul 19 15:27:06 2017

Passwords settings EG tests: Scroll in short viewports

While passwords_settings_egtest.mm already uses scrolling to find some
elements, it is not applied consistently. This CL ensures that scrolling is
applied everywhere where needed, including tapping the "Settings" button, so
that the tests also pass on iPhone SE in landscape mode.

Bug:  717163 , 744058 
Change-Id: Ifbc3438c78d47441c67a0d538a3895e2ed4a39b5
Reviewed-on: https://chromium-review.googlesource.com/573041
Reviewed-by: Mike Baxley <baxley@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487871}
[modify] https://crrev.com/857e08190fa0f2b4a031b1ef98c74d4eebb52bc8/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm
[modify] https://crrev.com/857e08190fa0f2b4a031b1ef98c74d4eebb52bc8/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm
[modify] https://crrev.com/857e08190fa0f2b4a031b1ef98c74d4eebb52bc8/ios/chrome/test/earl_grey/chrome_matchers.h
[modify] https://crrev.com/857e08190fa0f2b4a031b1ef98c74d4eebb52bc8/ios/chrome/test/earl_grey/chrome_matchers.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38b4a11f3f47cd944bb2a50ba85b8b5e7c7ff7c9

commit 38b4a11f3f47cd944bb2a50ba85b8b5e7c7ff7c9
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Jul 20 08:36:24 2017

Separate layout constraints from button matchers in passwords_settings_egtest.mm

The button matchers in passwords_settings_egtest.mm have currently some
redundant grey_layout()-based sub-matchers. The idea is that when matching for
the buttons, the layout of the detail view page is also checked.

However, these redundant matchers make the button matchers heavy and sometimes
unusable (e.g., in the presence of different layouts for federated or
blacklisted credentials). Therefore this CL removes the layout constraints
from the matchers and instead creates specialised tests dedicated to just
checking the layout.

The past test "testBlacklisted" has been subsumed in the more detailed new
"testLayoutBlacklisted", and hence the former was removed.

Bug:  744058 
Change-Id: I5428c9246e5de29f1124788e368550f7e6cfa72b
Reviewed-on: https://chromium-review.googlesource.com/573024
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488168}
[modify] https://crrev.com/38b4a11f3f47cd944bb2a50ba85b8b5e7c7ff7c9/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45f0132c1f0e09d0ef432d9e3442de0f6f806f89

commit 45f0132c1f0e09d0ef432d9e3442de0f6f806f89
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Jul 20 09:39:04 2017

Clean-up methods in PasswordsSettingsTestCase

PasswordsSettingsTestCase exposes a number of instance methods to do
instance-independent work. Those should either become class methods or
functions in the anonymous namespace.

Those methods are not related to the PasswordsSettingsTestCase class either,
and there already are helper functions in the anonymous namespace in
passwords_settings_egtest.mm. Therefore this CL moves the mentioned methods to
become functions in the anonymous namespace.

Bug:  744058 
Change-Id: I1f8ede1d5bb4ef950f701cdefe407dc2d3bac0dd
Reviewed-on: https://chromium-review.googlesource.com/573025
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488176}
[modify] https://crrev.com/45f0132c1f0e09d0ef432d9e3442de0f6f806f89/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/388e72ac063da259a895697023931170feae9568

commit 388e72ac063da259a895697023931170feae9568
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Jul 20 11:36:24 2017

Wait correctly for BG tasks in passwords_settings_egtest.mm

The passwords settings EG test uses PasswordStore. PasswordStore works on a
background task runner, and the test needs to ensure that it waits until
PasswordStore is finished.

Unfortunately, the test currently attempts to do this with
[[GREYUIThreadExecutor sharedInstance] drainUntilIdle];

As the name suggests, this waits for the UI thread, not for the background task
runner. There is no synchronisation with the password store currently and the
tests pass just by luck. Indeed, test configurations are possible to write
where PasswordStore is not done on time and the test fails.

Therefore this CL replaces the waiting for UI thread with a combination of
posting tasks to PasswordStore and using WaitUntilConditionOrTimeout to check
their completion. The timeout selected is 1 second. Using the pre-defined value
kSpinDelaySeconds turned out to be too short.

The CL not only replaces GREYUIThreadExecutor with the new synchronisation, it
also checks the state of the PasswordStore in more detail. Furthermore, it also
adds synchronisation with PasswordStore after the password list view loads,
because that view also depends on the response from PasswordStore.

Bug:  744058 
Change-Id: I1a86c0cdfa9b60a609505005b7d2df5e1896dd54
Reviewed-on: https://chromium-review.googlesource.com/573540
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488199}
[modify] https://crrev.com/388e72ac063da259a895697023931170feae9568/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c6649dbdc5bf4b9b2ddbf0ff0def3b929972034

commit 7c6649dbdc5bf4b9b2ddbf0ff0def3b929972034
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Jul 20 16:07:00 2017

Improve deletion tests for iOS passwords settings

This CL adds a test for deleting passwords in the list view. It checks that the
UI is updated correctly.

The CL also enhances all deletion-based tests to check the state of the
PasswordStore.

Bug:  744058 
Change-Id: Ib545312b62b5563fc284b54aa18158c68a0ef767
Reviewed-on: https://chromium-review.googlesource.com/573541
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488259}
[modify] https://crrev.com/7c6649dbdc5bf4b9b2ddbf0ff0def3b929972034/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c48bf2e2b72e1f3759d3c6c3f749e89af4c3564

commit 2c48bf2e2b72e1f3759d3c6c3f749e89af4c3564
Author: Vaclav Brozek <vabr@chromium.org>
Date: Thu Jul 20 16:25:50 2017

Clear PasswordStore in tearDown in passwords_settings_egtest.mm

Most of the tests in passwords_settings_egtest.mm clear PasswordStore at the
end. It does not hurt to clear it in the rest of the tests either, because
clearing an empty store keeps it empty.

To reduce boilerplate, this CL moves the call to clear PasswordStore from
individual last lines of tests into the tearDown method of the fixture.

Bug:  744058 
Change-Id: I350f73cef271534d01390d2e032b06e00afca0fb
Reviewed-on: https://chromium-review.googlesource.com/573542
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488264}
[modify] https://crrev.com/2c48bf2e2b72e1f3759d3c6c3f749e89af4c3564/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cd1f6575633e19829dbd5c12bcf55736b77eed3

commit 8cd1f6575633e19829dbd5c12bcf55736b77eed3
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Jul 25 17:28:38 2017

Remove passwords tests from settings_egtest.mm

This removes all tests related to the passwords settings from
settings_egtest.mm, because they are now covered by the content of
passwords_settings_egtest.mm. The only passwords-related tests left in
settings_egtest.mm are for clear-browsing-data dialogue, which allows to delete
passwords from a different part of settings than the passwords page.

Bug:  744058 
Change-Id: Ibe0149a188ca943a3b5ee675a9e40c970e8d60b1
Reviewed-on: https://chromium-review.googlesource.com/572981
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489352}
[modify] https://crrev.com/8cd1f6575633e19829dbd5c12bcf55736b77eed3/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm
[modify] https://crrev.com/8cd1f6575633e19829dbd5c12bcf55736b77eed3/ios/chrome/browser/ui/settings/settings_egtest.mm

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd6b1e5b91405e8294feb246d79e1498447fc0bc

commit cd6b1e5b91405e8294feb246d79e1498447fc0bc
Author: Vaclav Brozek <vabr@chromium.org>
Date: Tue Jul 25 17:40:42 2017

Use IMPLICIT_ACCESS in passwords settings EG tests

When accessing the PasswordStore, the EG tests for passwords settings
use EXPLICIT_ACCESS so far as an arbitrary value, given that the tests
are expected to run outside Incognito, and the access type only matters
for Incognito.

However, using IMPLICIT_ACCESS will help to flag a potential error if
the tests do start running in Incognito context. This CL switches the
access type to IMPLICIT_ACCESS.

Bug:  744058 
Change-Id: I782d136ac29a627a7dd54bbfa3d5f8513a120d12
Reviewed-on: https://chromium-review.googlesource.com/578068
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489356}
[modify] https://crrev.com/cd6b1e5b91405e8294feb246d79e1498447fc0bc/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm

Comment 14 by vabr@chromium.org, Jul 25 2017

Status: Fixed (was: Started)

Sign in to add a comment