Refactor some certificate verification tests in preparation to adding

Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200
Committed: https://crrev.com/82ab15b8a6f7873ab9d520ad5c32142ec033dfe2
Cr-Commit-Position: refs/heads/master@{#411458}

[eroman, 2016-08-11 00:56:55]

Description was changed from

==========
Refactor some certificate verificaton tests in preparation to adding
trust anchor constraints.

 * Rename TRUSTED_CERTIFICATE --> TRUST_ANCHOR_UNCONSTRAINED
 * Misc other refactors related to this

BUG=635200
==========

to

==========
Refactor some certificate verificaton tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

[eroman, 2016-08-11 00:57:13]

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

[eroman, 2016-08-11 01:02:30]

https://codereview.chromium.org/2233233002/diff/40001/net/cert/internal/test_...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2233233002/diff/40001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:103: void
ReadVerifyCertChainTestFromFile(const std::string& file_name,
This is basically unchanged from the earlier definition, except for:

* Recognizes TRUST_ANCHOR_UNCONSTRAINED instead of TRUSTED_CERTIFICATE
* Outputs to a TrustAnchor rather than ParsedCertificate
* Asserts that the input contained a trust anchor (before it was optional)

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/README (right):

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/README:42: interpreted:
TRUST_ANCHOR_CONSTRAINED or TRUST_ANCHOR_UNCONSTRAINED.
Maybe I should remove mention of "_CONSTRAINED" since this CL does not yet
implement that... But this shows where I am going with the change :)

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:383: class
TestData(object):
Maybe this isn't idiomatic python.

But it felt a bit easier to encapsulate the variables into a container rather
than adding more.

I can drop some of these other refactors they aren't core to my CL (except for
the TrustAnchor struct above)

[eroman, 2016-08-11 18:59:27]

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
File net/data/verify_certificate_chain_unittest/common.py (right):

https://codereview.chromium.org/2233233002/diff/40001/net/data/verify_certifi...
net/data/verify_certificate_chain_unittest/common.py:383: class
TestData(object):
On 2016/08/11 01:02:30, eroman wrote:
> Maybe this isn't idiomatic python.
> 
> But it felt a bit easier to encapsulate the variables into a container rather
> than adding more.
> 
> I can drop some of these other refactors they aren't core to my CL (except for
> the TrustAnchor struct above)

Quick update: I removed this unnecessary refactor to keep this CL more focused,
so please see latest patchset instead when reviewing.

[mattm, 2016-08-11 19:44:02]

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:141: } else if (block_type ==
kTrustAnchorUnconstrained) {
Add an ASSERT_FALSE(*trust_anchor) here?

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:166: ASSERT_TRUE(has_trust_anchor);
could just ASSERT_TRUE(*trust_anchor) ?

https://codereview.chromium.org/2233233002/diff/100001/net/cert/internal/veri...
File net/cert/internal/verify_certificate_chain_typed_unittest.h (right):

https://codereview.chromium.org/2233233002/diff/100001/net/cert/internal/veri...
net/cert/internal/verify_certificate_chain_typed_unittest.h:217:
TYPED_TEST_CASE_P(VerifyCertificateChainNonSingleRootTest);
I think there is no need to have this as separate case now that unknown-root has
a bogus root instead of no root.

[eroman, 2016-08-11 21:57:09]

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

[commit-bot: I haz the power, 2016-08-11 21:58:34]

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

[eroman, 2016-08-11 22:01:23]

Thanks for the review!

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:141: } else if (block_type ==
kTrustAnchorUnconstrained) {
On 2016/08/11 19:44:02, mattm wrote:
> Add an ASSERT_FALSE(*trust_anchor) here?

Done.

https://codereview.chromium.org/2233233002/diff/60001/net/cert/internal/test_...
net/cert/internal/test_helpers.cc:166: ASSERT_TRUE(has_trust_anchor);
On 2016/08/11 19:44:02, mattm wrote:
> could just ASSERT_TRUE(*trust_anchor) ?

Done.

[mattm, 2016-08-11 22:14:50]

lgtm w/nits: 
* typo in description (verificaton)
* one more comment.

https://codereview.chromium.org/2233233002/diff/120001/net/cert/internal/test...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2233233002/diff/120001/net/cert/internal/test...
net/cert/internal/test_helpers.cc:108: chain->clear();
Oh, might want to *trust_anchor = nullptr; here too.

[eroman, 2016-08-11 22:17:39]

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

[commit-bot: I haz the power, 2016-08-11 22:18:30]

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

[eroman, 2016-08-11 22:18:45]

Description was changed from

==========
Refactor some certificate verificaton tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

to

==========
Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

[eroman, 2016-08-11 22:19:05]

(fixed CL description)

https://codereview.chromium.org/2233233002/diff/120001/net/cert/internal/test...
File net/cert/internal/test_helpers.cc (right):

https://codereview.chromium.org/2233233002/diff/120001/net/cert/internal/test...
net/cert/internal/test_helpers.cc:108: chain->clear();
On 2016/08/11 22:14:50, mattm wrote:
> Oh, might want to *trust_anchor = nullptr; here too.

Done (independently noticed this while testing :)

[commit-bot: I haz the power, 2016-08-11 23:51:15]

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

[commit-bot: I haz the power, 2016-08-11 23:51:16]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-08-11 23:53:03]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-08-11 23:53:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2233233002/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-08-11 23:54:00]

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, 2016-08-12 00:00:45]

Description was changed from

==========
Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

to

==========
Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

[commit-bot: I haz the power, 2016-08-12 00:00:46]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-12 00:03:18]

Description was changed from

==========
Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200
==========

to

==========
Refactor some certificate verification tests in preparation to adding
trust anchor constraints.

BUG=635200

Committed: https://crrev.com/82ab15b8a6f7873ab9d520ad5c32142ec033dfe2
Cr-Commit-Position: refs/heads/master@{#411458}
==========

[commit-bot: I haz the power, 2016-08-12 00:03:19]

Patchset 8 (id:??) landed as
https://crrev.com/82ab15b8a6f7873ab9d520ad5c32142ec033dfe2
Cr-Commit-Position: refs/heads/master@{#411458}