Make test outputs involving PasswordForms more readable.

Make test outputs more readable whenever PasswordForms are involved.

This CL improves ContainsEqualPasswordFormsUnordered (and the corresponding GMock matcher) so that it outputs both the unmatched expected *and* actual values, and outputs them only once, even if the matcher is evaluated multiple times.

Furthermore, PasswordForms themselves are now outputted as JSON, prettiefied, and omitting the default values. Finally, pointers to PasswordForms are now outputted reasonably as well, showing the contents, instead of showing the memory addresses.

Dependent Sync integration tests have been refactored.

BUG=437865
Committed: https://crrev.com/c0830f221223891292888c0cef11c2751bb40f99
Cr-Commit-Position: refs/heads/master@{#323224}

[engedy, 2015-03-19 12:40:04]

Patchset #1 (id:1) has been deleted

[engedy, 2015-03-19 12:42:20]

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

[engedy, 2015-03-19 12:44:32]

@Vaclav, could you please take a look?

[engedy, 2015-03-19 12:45:30]

For reference, test outputs involving ContainsSamePasswordForms looked like
this:

[2499:2499:0319/134329:616719509463:ERROR:password_manager_test_utils.cc(83)] No
match for:
scheme: 0 signon_realm: https://one.example.com/ original_signon_realm:
https://two.example.com/ origin: https://one.example.com/origin action: 
submit_element:  username_elem:  username_marked_by_site: 0 username_value:
username_value_2 password_elem:  password_value:  new_password_element: 
new_password_value:  other_possible_usernames:  autocomplete_set:1 blacklisted:
0 preferred: 1 ssl_valid: 1 date_created: 1 date_synced: 86401 type: 0
times_used: 0 form_data:    0 1 Fields: generation_upload_status: 0
display_name: username_value_2 avatar_url: https://accounts.google.com/Avatar
federation_url: https://accounts.google.com/login skip_next_zero_click: 1
[2499:2499:0319/134329:616719509668:ERROR:password_manager_test_utils.cc(83)] No
match for:
scheme: 0 signon_realm: https://one.example.com/ original_signon_realm:
https://two.example.com/ origin: https://one.example.com/origin action: 
submit_element:  username_elem:  username_marked_by_site: 0 username_value:
username_value_2 password_elem:  password_value:  new_password_element: 
new_password_value:  other_possible_usernames:  autocomplete_set:1 blacklisted:
0 preferred: 1 ssl_valid: 1 date_created: 1 date_synced: 86401 type: 0
times_used: 0 form_data:    0 1 Fields: generation_upload_status: 0
display_name: username_value_2 avatar_url: https://accounts.google.com/Avatar
federation_url: https://accounts.google.com/login skip_next_zero_click: 1
[2499:2499:0319/134329:616719509741:ERROR:password_manager_test_utils.cc(83)] No
match for:
scheme: 0 signon_realm: https://one.example.com/ original_signon_realm:
https://two.example.com/ origin: https://one.example.com/origin action: 
submit_element:  username_elem:  username_marked_by_site: 0 username_value:
username_value_2 password_elem:  password_value:  new_password_element: 
new_password_value:  other_possible_usernames:  autocomplete_set:1 blacklisted:
0 preferred: 1 ssl_valid: 1 date_created: 1 date_synced: 86401 type: 0
times_used: 0 form_data:    0 1 Fields: generation_upload_status: 0
display_name: username_value_2 avatar_url: https://accounts.google.com/Avatar
federation_url: https://accounts.google.com/login skip_next_zero_click: 1
[2499:2499:0319/134329:616719509810:ERROR:password_manager_test_utils.cc(83)] No
match for:
scheme: 0 signon_realm: https://one.example.com/ original_signon_realm:
https://two.example.com/ origin: https://one.example.com/origin action: 
submit_element:  username_elem:  username_marked_by_site: 0 username_value:
username_value_2 password_elem:  password_value:  new_password_element: 
new_password_value:  other_possible_usernames:  autocomplete_set:1 blacklisted:
0 preferred: 1 ssl_valid: 1 date_created: 1 date_synced: 86401 type: 0
times_used: 0 form_data:    0 1 Fields: generation_upload_status: 0
display_name: username_value_2 avatar_url: https://accounts.google.com/Avatar
federation_url: https://accounts.google.com/login skip_next_zero_click: 1
unknown file: Failure

Unexpected mock function call - returning directly.
    Function call: OnGetPasswordStoreResultsConstRef(@0x7ffff7279198 {
0x2c1edfb1020, 0x2c1edfb0c20 })
Google Mock tried the following 1 expectation, but it didn't match:

../../components/password_manager/core/browser/password_store_unittest.cc:326:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(
ContainsSamePasswordForms(expected_results.get())))...
  Expected arg #0: contains same password forms { 0x2c1edfb0020, 0x2c1edfb0820 }
           Actual: { 0x2c1edfb1020, 0x2c1edfb0c20 }
         Expected: to be called once
           Actual: never called - unsatisfied and active
../../components/password_manager/core/browser/password_store_unittest.cc:326:
Failure
Actual function call count doesn't match EXPECT_CALL(mock_consumer,
OnGetPasswordStoreResultsConstRef(
ContainsSamePasswordForms(expected_results.get())))...
         Expected: to be called once
           Actual: never called - unsatisfied and active

[engedy, 2015-03-19 12:47:13]

Now they look like this:

Unexpected mock function call - returning directly.
    Function call: OnGetPasswordStoreResultsConstRef(@0x7fff8d926808 {
&PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_1",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_1"
}), &PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_2",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "original_signon_realm": "https://two.example.com/",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_2"
}) })
Google Mock tried the following 1 expectation, but it didn't match:

../../components/password_manager/core/browser/password_store_unittest.cc:329:
EXPECT_CALL(mock_consumer, OnGetPasswordStoreResultsConstRef(
UnorderedPasswordFormElementsAre(expected_results.get())))...
  Expected arg #0: unordered password form elements are { &PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_1",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_1"
}), &PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_1",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_1"
}) }
           Actual: { &PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_1",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_1"
}), &PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_2",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "original_signon_realm": "https://two.example.com/",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_2"
}) }, 
Unmatched actual form:
PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_2",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "original_signon_realm": "https://two.example.com/",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_2"
})
Unmatched expected form:
PasswordForm({
   "avatar_url": "https://accounts.google.com/Avatar",
   "date_created": 1.0,
   "date_synced": 86401.0,
   "display_name": "username_value_1",
   "federation_url": "https://accounts.google.com/login",
   "origin": "https://one.example.com/origin",
   "preferred": true,
   "signon_realm": "https://one.example.com/",
   "skip_next_zero_click": true,
   "ssl_valid": true,
   "username_value": "username_value_1"
})
         Expected: to be called once
           Actual: never called - unsatisfied and active
../../components/password_manager/core/browser/password_store_unittest.cc:329:
Failure
Actual function call count doesn't match EXPECT_CALL(mock_consumer,
OnGetPasswordStoreResultsConstRef(
UnorderedPasswordFormElementsAre(expected_results.get())))...
         Expected: to be called once
           Actual: never called - unsatisfied and active

[vabr (Chromium), 2015-03-19 13:28:12]

Thanks, this looks awesome!

LGTM, because it is a big improvement against the current state already.

I pointed out below a small confusion about sets vs. multisets, which was not
introduced by this CL. Maybe you cant still find a good way to make it clearer,
at least in the comments? If not, don't block this CL with it, though.

Cheers,
Vaclav

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:57:
typedef std::set<const autofill::PasswordForm*> SetOfForms;
Part of the problem I mentioned in the header-file comment is that SetOfForms
can still represent a multiset of PasswordForms. The type is only capable to
prevent pointer duplicities, not the pointed object duplicities.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:48: //
will be written in human readable form to |mismatches_output| if not NULL.
optional nit: "if not NULL" -> "unless it is NULL".
These are 2 proposed changes:
(1) NULL (which is a deprecated C++ language constant) -> null (an English
adjective)
(2) introducing "it is" to make clear that it is |mismatches_output| which are
potentially null, not the details.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:55: //
are equal to those in |forms|, regardless of order.
forms -> expected_forms

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:55: //
are equal to those in |forms|, regardless of order.
"equal to those in |forms|" sounds a bit incorrect, as it can be misinterpreted
as "equal to every single form in |forms|".

Also, it is not exactly clear, if this is in the set or multiset sense. The fact
a form can be represented twice because of both the same pointer appearing
twice, or two different pointers pointing to identical copies of a PasswordForm,
does not help with explaining this.

Would you mind trying to make the description a bit more precise in specifying
which equality (set vs. multiset) it checks, and what is assumed about the
expected_forms (e.g., not pointer duplicities, etc.)?

[engedy, 2015-03-20 13:17:16]

Thanks for pointing out the ambiguity introduced by duplicates.

I have changed the implementation to use multisets, and rephrased the comments
so that they no longer give the false sense that the addresses of pointers are
of any importance here; but, admittedly, tried to remain as concise as possible.

Could you please take another look at the comments.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:57:
typedef std::set<const autofill::PasswordForm*> SetOfForms;
On 2015/03/19 13:28:12, vabr (Chromium) wrote:
> Part of the problem I mentioned in the header-file comment is that SetOfForms
> can still represent a multiset of PasswordForms. The type is only capable to
> prevent pointer duplicities, not the pointed object duplicities.

I have changed this to multiset, and clarified the comment in the header.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.h
(right):

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:48: //
will be written in human readable form to |mismatches_output| if not NULL.
On 2015/03/19 13:28:12, vabr (Chromium) wrote:
> optional nit: "if not NULL" -> "unless it is NULL".
> These are 2 proposed changes:
> (1) NULL (which is a deprecated C++ language constant) -> null (an English
> adjective)
> (2) introducing "it is" to make clear that it is |mismatches_output| which are
> potentially null, not the details.

Both done.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:55: //
are equal to those in |forms|, regardless of order.
On 2015/03/19 13:28:12, vabr (Chromium) wrote:
> forms -> expected_forms

Done. I have updated the comment above, and completely removed it for the GMock
matcher -- I do not think it adds much value.

https://codereview.chromium.org/1022823002/diff/20001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.h:55: //
are equal to those in |forms|, regardless of order.
On 2015/03/19 13:28:12, vabr (Chromium) wrote:
> forms -> expected_forms

N/A anymore.

[engedy, 2015-03-20 13:57:37]

As per offline discussion, I got rid of the std::set. I am now also using a
lambda expression instead of the for loop, and a variant of the erase-remove
idiom.

PTAL.

[vabr (Chromium), 2015-03-20 14:10:49]

LGTM already, some minor comments to optionally consider.
Cheers,
Vaclav

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:61:
std::vector<PasswordForm*> remaining_expectations(expectations.begin(),
nit: Could this be a vector of const PasswordForm*, or would that break some of
the std:: magic below?

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:69: //
Erase the matched expectation by moving to last element to its place.
typo: to last -> the last

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:70:
std::swap(*it_matching_expectation, remaining_expectations.back());
nit: just copying the last element over the one matching expectation would do
this, no?

[engedy, 2015-03-20 16:15:06]

Thanks for pointing out the code smells! I have addressed all concerns modulo
the one we have discussed offline not to.

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:61:
std::vector<PasswordForm*> remaining_expectations(expectations.begin(),
On 2015/03/20 14:10:48, vabr (Chromium) wrote:
> nit: Could this be a vector of const PasswordForm*, or would that break some
of
> the std:: magic below?

As discussed, let us just keep these non-consts, as there is no difference to
the caller, and may be a bit confusing to the reader of the implementation.

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:65: auto
it_matching_expectation = std::remove_if(
Ahh, I also realized that I was using the erase-remove-remove idiom instead of
the erase-remove idiom, so I have fixed that too. :-)

The funny thing that the implementation was actually still correct this way, it
would only have reordered some elements without reason in case there were
duplicates.

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:69: //
Erase the matched expectation by moving to last element to its place.
On 2015/03/20 14:10:48, vabr (Chromium) wrote:
> typo: to last -> the last

Done.

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:70:
std::swap(*it_matching_expectation, remaining_expectations.back());
On 2015/03/20 14:10:48, vabr (Chromium) wrote:
> nit: just copying the last element over the one matching expectation would do
> this, no?

Done.

[vabr (Chromium), 2015-03-20 16:19:58]

LGTM unconditionally! :)

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
File components/password_manager/core/browser/password_manager_test_utils.cc
(right):

https://codereview.chromium.org/1022823002/diff/80001/components/password_man...
components/password_manager/core/browser/password_manager_test_utils.cc:65: auto
it_matching_expectation = std::remove_if(
On 2015/03/20 16:15:05, engedy wrote:
> Ahh, I also realized that I was using the erase-remove-remove idiom instead of
> the erase-remove idiom, so I have fixed that too. :-)
> 
> The funny thing that the implementation was actually still correct this way,
it
> would only have reordered some elements without reason in case there were
> duplicates.

Acknowledged.

[engedy, 2015-03-20 16:22:20]

engedy@chromium.org changed reviewers:
+ tim@chromium.org

[engedy, 2015-03-20 16:22:21]

@Tim, could you please review c/b/sync/test for OWNER's approval?

The sync integration test output in case of failures will slightly change after
this CL:
  1.) Only the mismatched forms will be outputted, not everything.
  2.) The original implementation wrote the first mismatched form to LOG(ERROR).
The new implementation will write all additional information about mismatched
forms to VLOG(1).

Happy to adjust if I underestimated the value of these outputs.

[engedy, 2015-03-31 09:52:29]

engedy@chromium.org changed reviewers:
+ atwilson@chromium.org
- tim@chromium.org

[engedy, 2015-03-31 09:55:42]

Rerouting to Drew.

@Drew, could you please take a look? (Please see caveats are in my previous
message.)

[engedy, 2015-03-31 10:56:28]

@Drew, Just to clarify, you need not look at the entire CL, only c/b/sync/test.

[Andrew T Wilson (Slow), 2015-03-31 14:27:48]

sync lgtm

[engedy, 2015-03-31 14:34:08]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-03-31 14:34:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022823002/100001

[commit-bot: I haz the power, 2015-03-31 14:37:14]

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

[commit-bot: I haz the power, 2015-03-31 14:37:16]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[engedy, 2015-04-01 12:26:17]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-04-01 12:26:18]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/1022823002/#ps120001
(title: "Rebase + added new field that was added inbetween.")

[commit-bot: I haz the power, 2015-04-01 12:26:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1022823002/120001

[commit-bot: I haz the power, 2015-04-01 13:17:05]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-01 13:18:07]

Patchset 6 (id:??) landed as
https://crrev.com/c0830f221223891292888c0cef11c2751bb40f99
Cr-Commit-Position: refs/heads/master@{#323224}