Add EGTests for passwords settings on iOS

Add EGTests for passwords settings on iOS

This adds a basic integration test for saving on a test page, and also a test
for copy-password confirmation toast, which is not coverable by the unittests (yet).

BUG=159166
Review-Url: https://codereview.chromium.org/2846253002
Cr-Commit-Position: refs/heads/master@{#468910}
Committed: https://chromium.googlesource.com/chromium/src/+/3304cfee612c9ff570a7ede78513c969ccd74ac9

[vabr (Chromium), 2017-05-01 10:27:57]

Description was changed from

==========
[WIP] Add EGTests for passwords settings on iOS

COMMIT=false
This is not currently completed, just a backup copy.

This adds a basic integration test for saving on a test page, and also a test
for confirmation toast, which is not coverable by the unittests (yet).

BUG=159166
==========

to

==========
Add EGTests for passwords settings on iOS

This adds a basic integration test for saving on a test page, and also a test
for copy-password confirmation toast, which is not coverable by the unittests
(yet).

BUG=159166
==========

[vabr (Chromium), 2017-05-01 10:28:36]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 10:28:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 10:35:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 10:35:24]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[vabr (Chromium), 2017-05-01 10:42:58]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 10:43:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2017-05-01 11:08:25]

vabr@chromium.org changed reviewers:
+ sdefresne@chromium.org

[vabr (Chromium), 2017-05-01 11:08:26]

Hi sdefresne@,

Could you please review this?

Thanks,
Vaclav

[commit-bot: I haz the power, 2017-05-01 11:43:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 11:43:47]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-05-02 15:30:07]

sdefresne@chromium.org changed reviewers:
+ baxley@chromium.org

[sdefresne, 2017-05-02 15:30:08]

+baxley: should we use those small helper methods or not?

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/password_details_collection_view_controller_for_testing.h
(right):

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/password_details_collection_view_controller_for_testing.h:12:
// Allows to replace a |reauthenticaionModule| for a fake one in integration
reauthenticaionModule → reauthenticationModule

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:52: id<GREYMatcher>
SettingsButton() {
baxley: are those helper methods (used only once) what you complained about last
week?

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:269:
MockReauthenticationModule* reauth =
I think that the styleguide discourage uses of abbreviations for variable names.
Maybe use "reauthentication_module", "settings_navigation_controller",
"password_details_collection_view_controller".

[baxley, 2017-05-02 16:20:41]

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:52: id<GREYMatcher>
SettingsButton() {
On 2017/05/02 15:30:08, sdefresne wrote:
> baxley: are those helper methods (used only once) what you complained about
last
> week?

I think some of these fall under the category of aiding readability. The good
thing here is that they are matchers, and not C-methods wrapping calls to EG
that perform actions or assertions. I wouldn't push back on these matchers,
since I think this is starting to get into personal preference, rather than
violating any style rules or consistency problems.

[sdefresne, 2017-05-02 16:28:58]

lgtm as baxley is happy with the helper methods.

Please fix the typo either in the CL or as a followup.

[vabr (Chromium), 2017-05-03 05:41:37]

Thanks for the review!
Sending to CQ now.
Vaclav

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/password_details_collection_view_controller_for_testing.h
(right):

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/password_details_collection_view_controller_for_testing.h:12:
// Allows to replace a |reauthenticaionModule| for a fake one in integration
On 2017/05/02 15:30:07, sdefresne wrote:
> reauthenticaionModule → reauthenticationModule

Done.

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:52: id<GREYMatcher>
SettingsButton() {
On 2017/05/02 16:20:41, baxley wrote:
> On 2017/05/02 15:30:08, sdefresne wrote:
> > baxley: are those helper methods (used only once) what you complained about
> last
> > week?
> 
> I think some of these fall under the category of aiding readability. The good
> thing here is that they are matchers, and not C-methods wrapping calls to EG
> that perform actions or assertions. I wouldn't push back on these matchers,
> since I think this is starting to get into personal preference, rather than
> violating any style rules or consistency problems.

Thanks for raising this and for the detailed answer.

What I understood during last week is that slicing chunks out of the tests and
hiding them elsewhere is unhelpful. However, here I argued that what read
roughly as "perform tap on what has accessibility ID kSomething" now reads as
"perform tap on SettingsButton", which does not really hide the spirit, just the
details. Also, in the depending CLs, I am introducing more callsites of those
matchers. Finally, this borrowed from the existing settings EG Test, so at least
it is consistent in the directory.

https://codereview.chromium.org/2846253002/diff/60001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:269:
MockReauthenticationModule* reauth =
On 2017/05/02 15:30:08, sdefresne wrote:
> I think that the styleguide discourage uses of abbreviations for variable
names.
> Maybe use "reauthentication_module", "settings_navigation_controller",
> "password_details_collection_view_controller".

Good point, done.

[vabr (Chromium), 2017-05-03 05:41:58]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-05-03 05:41:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2846253002/#ps80001
(title: "Comments")

[commit-bot: I haz the power, 2017-05-03 05:42:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 06:50:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493790118144130,
"parent_rev": "cb129c01ece3b6201a3baeceeb5b988fa49ef1be", "commit_rev":
"3304cfee612c9ff570a7ede78513c969ccd74ac9"}

[commit-bot: I haz the power, 2017-05-03 06:50:40]

Description was changed from

==========
Add EGTests for passwords settings on iOS

This adds a basic integration test for saving on a test page, and also a test
for copy-password confirmation toast, which is not coverable by the unittests
(yet).

BUG=159166
==========

to

==========
Add EGTests for passwords settings on iOS

This adds a basic integration test for saving on a test page, and also a test
for copy-password confirmation toast, which is not coverable by the unittests
(yet).

BUG=159166

Review-Url: https://codereview.chromium.org/2846253002
Cr-Commit-Position: refs/heads/master@{#468910}
Committed:
https://chromium.googlesource.com/chromium/src/+/3304cfee612c9ff570a7ede78513...
==========

[commit-bot: I haz the power, 2017-05-03 06:50:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3304cfee612c9ff570a7ede78513...