Improve passwords EG tests |
||
Issue descriptionios/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
,
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/
,
Jul 16 2017
The change to settings_egtest.mm is in https://chromium-review.googlesource.com/c/572981/
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
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
,
Jul 25 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by vabr@chromium.org
, Jul 16 2017