Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

BUG=610546, 649017
Review-Url: https://codereview.chromium.org/2672083003
Cr-Commit-Position: refs/heads/master@{#448353}
Committed: https://chromium.googlesource.com/chromium/src/+/45b6e7c94dabd216c9ce503f5259fd245af14cef

[eroman, 2017-02-03 22:13:34]

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

[commit-bot: I haz the power, 2017-02-03 22:14:29]

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-02-03 23:00:49]

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

[commit-bot: I haz the power, 2017-02-03 23:00:49]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-02-03 23:08:41]

eroman@chromium.org changed reviewers:
+ rsleevi@chromium.org

[eroman, 2017-02-03 23:08:41]

I changed the setup of this test a bit, WDYT? (going with synthetic chain
instead of the expired twitter one).

I would like to enable this test since I need coverage of the public key hashes
for testing CertVerifyProcBuiltin.

[eroman, 2017-02-03 23:09:15]

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

[commit-bot: I haz the power, 2017-02-03 23:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-02-03 23:54:21]

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:743: // verifies the public key hashes
chain rather than the chain itself.
This isn't necessary to match that. We're really just trying to test that it
fills PublicKeyHashes with all the values.

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:755: // Construct the chain out of order.
This isn't necessary for the test

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:773: HashValueVector sha256_hashes;
concrete suggestion: vector<SHA1HashValue> and vector<SHA256HashValue> for these
will allow you to make use of vector==

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:804: }
Interesting, the original test was relying on a (non-guaranteed) API behaviour,
which is that hashes are in a particular order (we don't presently guarantee
that, and every time I add a new search thing I keep wondering whether it's
worth paying the 1-time sort cost in CertVerifyProc and having the API guarantee
sort-stability

You can replace this with

EXPECT_THAT(sha256_hashes,
WhenSorted(UnorderedElementsAreArray(kExpectedSHA256Hashes)));

[commit-bot: I haz the power, 2017-02-04 00:03:35]

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

[commit-bot: I haz the power, 2017-02-04 00:03:36]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-02-04 00:56:05]

Description was changed from

==========
Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

BUG=610546
==========

to

==========
Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

BUG=610546,649017
==========

[eroman, 2017-02-04 00:58:11]

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

[commit-bot: I haz the power, 2017-02-04 00:58:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-02-04 00:59:28]

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:743: // verifies the public key hashes
chain rather than the chain itself.
On 2017/02/03 23:54:21, Ryan Sleevi wrote:
> This isn't necessary to match that. We're really just trying to test that it
> fills PublicKeyHashes with all the values.

Done.

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:755: // Construct the chain out of order.
On 2017/02/03 23:54:21, Ryan Sleevi wrote:
> This isn't necessary for the test

Done.

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:773: HashValueVector sha256_hashes;
On 2017/02/03 23:54:21, Ryan Sleevi wrote:
> concrete suggestion: vector<SHA1HashValue> and vector<SHA256HashValue> for
these
> will allow you to make use of vector==

no longer applicable

https://codereview.chromium.org/2672083003/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:804: }
On 2017/02/03 23:54:21, Ryan Sleevi wrote:
> Interesting, the original test was relying on a (non-guaranteed) API
behaviour,
> which is that hashes are in a particular order (we don't presently guarantee
> that, and every time I add a new search thing I keep wondering whether it's
> worth paying the 1-time sort cost in CertVerifyProc and having the API
guarantee
> sort-stability
> 
> You can replace this with
> 
> EXPECT_THAT(sha256_hashes,
> WhenSorted(UnorderedElementsAreArray(kExpectedSHA256Hashes)));

Done -- removed ordering requirement in test, and made some changes to the
documentation.

[eroman, 2017-02-04 01:08:12]

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

[commit-bot: I haz the power, 2017-02-04 01:08:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-02-04 01:16:18]

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

[commit-bot: I haz the power, 2017-02-04 01:16:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-02-04 01:47:03]

lgtm

https://codereview.chromium.org/2672083003/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2672083003/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:798: EXPECT_EQ(expected_public_key_hashes,
public_key_hash_strings);
So, to be clear: This totally works for me as is, but I do wonder whether we
should be adopting more GMock EXPECT_THAT assertions, as they do offer pretty
printers, and (for better or worse) we've already adopted things like EXPECT_OK
& friends

Even though googletest recommends it as the new way (and google3 is moving
there), the last thread on chromium-dev had more ambivalence to that, so like I
said, it's not a hard or fast thing.

It does seem prettier with an EXPECT_THAT when reading, and you get some nicer
printers, but at the same time, I'm conscious that if we push more GMock, we'll
eventually end up having to deal with the gross parts too :)

[eroman, 2017-02-04 01:58:45]

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

[commit-bot: I haz the power, 2017-02-04 01:59:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-02-04 01:59:56]

https://codereview.chromium.org/2672083003/diff/80001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc_unittest.cc (right):

https://codereview.chromium.org/2672083003/diff/80001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc_unittest.cc:798: EXPECT_EQ(expected_public_key_hashes,
public_key_hash_strings);
On 2017/02/04 01:47:02, Ryan Sleevi wrote:
> So, to be clear: This totally works for me as is, but I do wonder whether we
> should be adopting more GMock EXPECT_THAT assertions, as they do offer pretty
> printers, and (for better or worse) we've already adopted things like
EXPECT_OK
> & friends
> 
> Even though googletest recommends it as the new way (and google3 is moving
> there), the last thread on chromium-dev had more ambivalence to that, so like
I
> said, it's not a hard or fast thing.
> 
> It does seem prettier with an EXPECT_THAT when reading, and you get some nicer
> printers, but at the same time, I'm conscious that if we push more GMock,
we'll
> eventually end up having to deal with the gross parts too :)

Done.

[eroman, 2017-02-04 02:55:40]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2017-02-04 02:55:53]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-02-04 02:55:54]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2672083003/#ps100001
(title: "switch to EXPECT_THAT()")

[commit-bot: I haz the power, 2017-02-04 02:56:12]

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-02-04 04:56:40]

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

[commit-bot: I haz the power, 2017-02-04 04:56:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Ryan Sleevi, 2017-02-04 13:27:03]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2017-02-04 13:27:16]

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-02-04 15:27:42]

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

[commit-bot: I haz the power, 2017-02-04 15:27:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)

[eroman, 2017-02-06 18:10:30]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-02-06 18:11:12]

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-02-06 19:37:49]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486404630055340,
"parent_rev": "1b08650cd63cbd6fa11cc5a840d716a3b945306b", "commit_rev":
"45b6e7c94dabd216c9ce503f5259fd245af14cef"}

[commit-bot: I haz the power, 2017-02-06 19:38:25]

Description was changed from

==========
Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

BUG=610546,649017
==========

to

==========
Re-enable CertVerifyProcInternalTest.PublicKeyHashes.

BUG=610546,649017

Review-Url: https://codereview.chromium.org/2672083003
Cr-Commit-Position: refs/heads/master@{#448353}
Committed:
https://chromium.googlesource.com/chromium/src/+/45b6e7c94dabd216c9ce503f5259...
==========

[commit-bot: I haz the power, 2017-02-06 19:38:26]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/45b6e7c94dabd216c9ce503f5259...