Parameterize the CertVerifyProc tests so they can be run with

net/cert/cert_verify_proc_unittest.cc

Index: net/cert/cert_verify_proc_unittest.cc
diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc
index 2e3a0f5d458f3e649789168ea481adc01e33a9e1..c27ae520f12133363b8d0f29ed46e86af29647fe 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -100,37 +100,104 @@ int MockCertVerifyProc::VerifyInternal(
   return OK;
 }
 
-bool SupportsReturningVerifiedChain() {
+// This enum names a concrete implemenation of CertVerifyProc.
+enum CertVerifyProcName {

[Ryan Sleevi, 2017/01/12 20:01:56]

So I had much more feedback originally written here, but I think two things
deserve calling out:

1) We're effectively reimplementing RTTI and type_id() - that is, this is all a
bunch of dynamic_cast<>s - and that does feel a little weird - but I can see why
and where we need it
2) The naming throws me off here, because I associate 'name' with string and
'id' with identifier. So I expected CertVerifyProcName to be a string, when it's
an enum, which then makes ToString weird.

[eroman, 2017/01/12 21:42:48]

Before I dive into the other comments, let's hash out your design concerns.

I was actually satisfied with the resulting organization of the unit-tests,
finding it more readable than before (and having gone through several other
iterations that I didn't like).

Fundamentally the problem we have is that test expectations depend on internal
details of the verifier that is used (and to a lesser degree the OS version it
is run on).

Primarily it depends on the concrete class of CertVerifyProc that is being used
(and in some cases the OS version as well).

In the currently checked in code, this is expressed by sprinkling #if throughout
the code, such as:

#if defined(OS_MACOSX) && !defined(OS_IOS)
...
#endif

or

#if defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
...
#endif

These #ifs replicate logic internal to CertVerifyProc::CreateDefault(), and
constitute the same layering violation that you do not like with the
CertVerifyProcName.

My first step was to isolate these #ifs into functions, by representing the
verifier under test as a CertVerifyProcName, which re-organizes how that
information is accessed.

I found this to simplify readability and compostability of the code, for the
usual reasons that #if in code is bad (can get compile failures on all platforms
when modifying tests, rather than only when compiling in certain configurations;
something which bit me in past iterations when I kept the #ifdefs inline).

The second step then was to parameterize CertVerifyProcName so the tests can be
instantiated and run for more than one (and behave appropriately). The only real
alternatives in GTest is to parameterize on a type (which has problems I will
get into later), or instantiate tests manually (with its attendant problems).

Brainstorming through other ideas:

(1) Remove all verifier-specific differences between tests, by dumbing down the
tests. This has the disadvantage of losing test coverage, which I consider
undesirable.

(2) Instantiate tests separately for each of the CertVerifyProc*, in their own
_unittest.cc file. The difference in test expectations would still exist in the
generic code, however would be communicated via a delegate when instantiating
the CertVerifyProc tests. Testing would be explicit for concrete CertVerifyProc
rather than using CertVerifyProc::CreateDefault().

There are other variations on (2), but fundamentally the approach is to use a
test policy delegate, rather selecting the behavior by a dispatch on the type
(within the test).

Structurally I find this makes the tests (and differences in behavior) harder to
understand, because it is de-coupled from the type. The other structure problem
is disabling tests also needs to be described as such a policy.

How to split the tests in this manner is also non-trivial.

GTest has some support for type-parameterized tests, but it is quite difficult
to use and I gave up on getting it to work the way I wanted. Splitting things in
this manner will be a much less iterative change that what I have proposed, and
from my early attempts I found it to be quite a bit more complicated and not
something I would recommend.

[Ryan Sleevi, 2017/01/13 17:22:09]

VerifyProcType?

[eroman, 2017/02/01 01:13:55]

Done.

+  CERT_VERIFY_PROC_NSS,
+  CERT_VERIFY_PROC_OPENSSL,
+  CERT_VERIFY_PROC_ANDROID,
+  CERT_VERIFY_PROC_IOS,
+  CERT_VERIFY_PROC_MAC,
+  CERT_VERIFY_PROC_WIN,
+
+  // TODO(crbug.com/649017): The upcoming "built-in" implementation.
+  CERT_VERIFY_PROC_BUILTIN,

[eroman, 2017/01/12 07:26:09]

I will scrub all the "built-in" stuff prior to landing (so the CL remains a pure
refactor).

I left it in here temporarily to demonstrate how it plugs in.

[eroman, 2017/02/01 01:13:55]

Done.

+};
+
+// Returns the CertVerifyProcName corresponding to what
+// CertVerifyProc::CreateDefault() returns.
+CertVerifyProcName GetDefaultCertVerifyProcName() {

[Ryan Sleevi, 2017/01/12 20:01:56]

I'm torn on the appropriateness of this code duplication (and myself felt both
guilt and lack of creativity w/r/t doing something similar with
AreSHA1IntermediatesAllowed)

The problem is it seems that this design choice itself introduces a lot of
secondary complexity that I think we can reduce/eliminate. The problem is I'm
trying to find whether my unease at reading this code is simply because I wrote
the code you're replacing, or whether it's a legitimate remark about the
unnecessary complexity in your CL. I don't have a good answer - so I'd simply
ask - do you think the complexity is justified and there's no simpler, clearer
solution (ideally without so many helper functions)?

[eroman, 2017/02/01 01:13:55]

Acknowledged.

(Renamed ProcName --> ProcType)

+#if defined(USE_NSS_CERTS)
+  return CERT_VERIFY_PROC_NSS;
+#elif defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
+  return CERT_VERIFY_PROC_OPENSSL;
+#elif defined(OS_ANDROID)
+  return CERT_VERIFY_PROC_ANDROID;
+#elif defined(OS_IOS)
+  return CERT_VERIFY_PROC_IOS;
+#elif defined(OS_MACOSX)
+  return CERT_VERIFY_PROC_MAC;
+#elif defined(OS_WIN)
+  return CERT_VERIFY_PROC_WIN;
+#else
+// Will fail to compile.
+#endif
+}
+
+const char* CertVerifyProcNameToString(CertVerifyProcName verify_proc_name) {

[Ryan Sleevi, 2017/01/12 20:01:56]

I don't believe this should be a helper function - you have exactly one call,
which is just VerifyProcTypeToName

[eroman, 2017/02/01 01:13:55]

Done.

+  switch (verify_proc_name) {
+    case CERT_VERIFY_PROC_NSS:
+      return "CertVerifyProcNSS";
+    case CERT_VERIFY_PROC_OPENSSL:
+      return "CertVerifyProcOpenSSL";
+    case CERT_VERIFY_PROC_ANDROID:
+      return "CertVerifyProcAndroid";
+    case CERT_VERIFY_PROC_IOS:
+      return "CertVerifyProcIOS";
+    case CERT_VERIFY_PROC_MAC:
+      return "CertVerifyProcMac";
+    case CERT_VERIFY_PROC_WIN:
+      return "CertVerifyProcWin";
+    case CERT_VERIFY_PROC_BUILTIN:
+      return "CertVerifyProcBuiltin";
+  }
+
+  return nullptr;
+}
+
+bool TargetIsIphoneSimulator() {

[Ryan Sleevi, 2017/01/12 20:01:56]

Was it necessary to make this a function? Couldn't this just be a bool variable?

[eroman, 2017/02/01 01:13:55]

Done.

+#if TARGET_IPHONE_SIMULATOR
+  return true;
+#else
+  return false;
+#endif
+}
+
+bool SupportsReturningVerifiedChain(CertVerifyProcName verify_proc_name) {

[Ryan Sleevi, 2017/01/13 17:22:09]

Despite our discussion, I'm still a little hazy about why you made this a free
function versus adding the the test fixture.

I think some of my unease might trace to the fact that you tried to keep this CL
minimal (yay!), but that means it left it inconsistent for reasons that aren't
clear - why are some helpers free functions, why are some on the base class,
what rules do you use to distinguish when you pass the CVPName versus when you
access verify_proc_name_ , etc.

[eroman, 2017/02/01 01:13:55]

I went ahead and moved them all into the base fixture (and removed the
parameter).

I was trying to keep things minimal, but reorganizing now should provide some
clarity.

 #if defined(OS_ANDROID)
   // Before API level 17, Android does not expose the APIs necessary to get at
   // the verified certificate chain.
-  if (base::android::BuildInfo::GetInstance()->sdk_int() < 17)
+  if (verify_proc_name == CERT_VERIFY_PROC_ANDROID &&
+      base::android::BuildInfo::GetInstance()->sdk_int() < 17)
     return false;
 #endif
   return true;
 }
 
-bool SupportsDetectingKnownRoots() {
+bool SupportsDetectingKnownRoots(CertVerifyProcName verify_proc_name) {
 #if defined(OS_ANDROID)
   // Before API level 17, Android does not expose the APIs necessary to get at
   // the verified certificate chain and detect known roots.
-  if (base::android::BuildInfo::GetInstance()->sdk_int() < 17)
-    return false;
-#elif defined(OS_IOS)
-  // iOS does not expose the APIs necessary to get the known system roots.
-  return false;
+  if (verify_proc_name == CERT_VERIFY_PROC_ANDROID)
+    return base::android::BuildInfo::GetInstance()->sdk_int() >= 17;
 #endif
+
+  // iOS does not expose the APIs necessary to get the known system roots.
+  if (verify_proc_name == CERT_VERIFY_PROC_IOS)
+    return false;
+
   return true;
 }
 
-bool WeakKeysAreInvalid() {
+bool WeakKeysAreInvalid(CertVerifyProcName verify_proc_name) {

[Ryan Sleevi, 2017/01/12 20:01:56]

Considering you're introducing a test fixture, I'm not sure why these helpers
can't or shouldn't be moved to the base class and the passing of
CertVerifyProcName dropped

[eroman, 2017/02/01 01:13:55]

Done.

 #if defined(OS_MACOSX) && !defined(OS_IOS)
   // Starting with Mac OS 10.12, certs with weak keys are treated as
   // (recoverable) invalid certificate errors.
-  return base::mac::IsAtLeastOS10_12();
-#else
-  return false;
+  if (verify_proc_name == CERT_VERIFY_PROC_MAC &&
+      base::mac::IsAtLeastOS10_12()) {
+    return true;
+  }
 #endif
+  return false;
 }
 
 // Template helper to load a series of certificate files into a CertificateList.
@@ -149,33 +216,48 @@ void LoadCertificateFiles(const char* const (&cert_files)[N],
   }
 }
 
-}  // namespace
+bool SupportsCRLSetsInPathBuilding(CertVerifyProcName verify_proc_name) {
+  // TODO(crbug.com/649017): Support for built-in
+  return verify_proc_name == CERT_VERIFY_PROC_WIN ||
+         verify_proc_name == CERT_VERIFY_PROC_NSS;
+}
 
-class CertVerifyProcTest : public testing::Test {
- public:
-  CertVerifyProcTest()
-      : verify_proc_(CertVerifyProc::CreateDefault()) {
-  }
-  ~CertVerifyProcTest() override {}
+// Returns the name of the CertVerifyProc implementation that is being tested.
+std::string VerifyProcTypeToName(
+    const testing::TestParamInfo<CertVerifyProcName>& params) {
+  return CertVerifyProcNameToString(params.param);
+}

[Ryan Sleevi, 2017/01/13 17:22:09]

Let's definitely ditch this.

[eroman, 2017/02/01 01:13:55]

It is useful to to have the test name be:

   XXX/CertVerifyProcMac
   XXX/CertVerifyProcBuiltin

Instead of:
   XXX/1
   XXX/2

I have kept this for now, however deleted CertVerifyProcNameToString() per the
earlier comment.

+
+// The set of all CertVerifyProcNames to be used by tests.
+std::vector<CertVerifyProcName> AllCertVerifiers() {
+  // TODO(crbug.com/649017): Include CERT_VERIFY_PROC_BUILTIN
+  return {GetDefaultCertVerifyProcName()};
+}

[Ryan Sleevi, 2017/01/13 17:22:09]

Do you actually need this to be a function? Unittests are allowed to do static
initializers, and you could always slap a constexpr on the
GetDefaultCertVerifyProcName() and force it to be a compile-time check if you
wanted.

[eroman, 2017/02/01 01:13:55]

Done.

+
+scoped_refptr<CertVerifyProc> CreateCertVerifyProc(
+    CertVerifyProcName verify_proc_name) {
+  // TODO(crbug.com/649017): Handle CERT_VERIFY_PROC_BUILTIN
+  EXPECT_EQ(verify_proc_name, GetDefaultCertVerifyProcName());
+  return CertVerifyProc::CreateDefault();

[Ryan Sleevi, 2017/01/12 20:01:56]

My instinct is that trying to preserve this is perhaps the root cause of the
complexity. Do you feel the same?

[Ryan Sleevi, 2017/01/13 17:22:09]

So I don't think the EXPECT_EQ does anything to prevent the version shear you're
worried about, because you're still calling CreateDefault() here - the only
difference is that you might "think" you're testing CertVerifyProcMac, but
you're actually testing CertVerifyProcBuiltin (because that's what CreateDefault
returned)

It seems that the only defense against shear (where the test differs from what
CreateDefault returns) is that you're expecting different results on some
platforms, but that doesn't seem to be in and of itself a hard guarantee.

[eroman, 2017/02/01 01:13:55]

The EXPECT_EQ() was to guard against me adding CERT_VERIFY_PROC_BUILTIN to the
kAllVerifiers, but failing to update this line.

The consequence of that mistake is all the tests for BUILTIN would trivially
pass as they are testing the default verifier rather than the built-in verifier.

I can remove the check if you like, since I will be verifying manually at such
time too.

+}
+
+}  // namespace
 
+// Base class for writing tests against a specific CertVerifyProc
+// implementation.
+class CertVerifyProcBaseTest : public testing::Test {
  protected:
+  void SetUp() override { verify_proc_ = CreateVerifyProc(&verify_proc_name_); }
+
+  // Creates the CertVerifyProc to be tested by this fixture, and fills
+  // |*verify_proc_name| with the created instance's name.
+  virtual scoped_refptr<CertVerifyProc> CreateVerifyProc(
+      CertVerifyProcName* verify_proc_name) = 0;

[Ryan Sleevi, 2017/01/12 20:01:56]

So it's unclear why you have the function return the fully constructed object,
and stashing the name of it into the |verify_proc_name|, rather than just having
the virtual function that tells you what sort of CertVerifyProc to create

That is, 321 and 327 are identical - so why not just have the subclass tell you
the name to run?

[eroman, 2017/02/01 01:13:55]

Done.

+
   bool SupportsAdditionalTrustAnchors() {
     return verify_proc_->SupportsAdditionalTrustAnchors();
   }
 
-  // Returns true if the underlying CertVerifyProc supports integrating CRLSets
-  // into path building logic, such as allowing the selection of alternatively
-  // valid paths when one or more are revoked. As the goal is to integrate this
-  // into all platforms, this is a temporary, test-only flag to centralize the
-  // conditionals in tests.
-  bool SupportsCRLSetsInPathBuilding() {
-#if defined(OS_WIN) || defined(USE_NSS_CERTS)
-    return true;
-#else
-    return false;
-#endif
-  }
-
   int Verify(X509Certificate* cert,
              const std::string& hostname,
              int flags,
@@ -197,19 +279,75 @@ class CertVerifyProcTest : public testing::Test {
                                 additional_trust_anchors, verify_result);
   }
 
+  bool UsingCertVerifyProcAndroid() const {

[Ryan Sleevi, 2017/01/12 20:01:56]

We're in CertVerifyProcUnittest - the use of "CertVerifyProc" here and in the
types feels entirely redundant/unnecessary

UsingAndroid/UsingNSS/UsingOpenSSL/ are possible simplifications there, but I
also question whether more fundamentally, we're just interested in default vs
builtin?

[eroman, 2017/02/01 01:13:55]

Removed these functions.

+    return verify_proc_name_ == CERT_VERIFY_PROC_ANDROID;
+  }
+
+  bool UsingCertVerifyProcNSS() const {

[Ryan Sleevi, 2017/01/13 17:22:09]

If we're going to go with these helper functions (which, to be honest, I'm not
terribly keen on, but I can live with), it does seem some documentation about
guarantees - such as how there should only ever be one of them true.

[eroman, 2017/02/01 01:13:55]

N/A

+    return verify_proc_name_ == CERT_VERIFY_PROC_NSS;
+  }
+
+  bool UsingCertVerifyProcOpenSSL() const {
+    return verify_proc_name_ == CERT_VERIFY_PROC_OPENSSL;
+  }
+
+  bool UsingCertVerifyProcIOS() const {
+    return verify_proc_name_ == CERT_VERIFY_PROC_IOS;
+  }
+
+  bool UsingCertVerifyProcWin() const {
+    return verify_proc_name_ == CERT_VERIFY_PROC_WIN;
+  }
+
+  bool UsingCertVerifyProcMac() const {
+    return verify_proc_name_ == CERT_VERIFY_PROC_MAC;
+  }
+
   const CertificateList empty_cert_list_;
   scoped_refptr<CertVerifyProc> verify_proc_;
+  CertVerifyProcName verify_proc_name_;
+};
+
+// This test fixture is paramterized by the CertVerifyProc (which is specified
+// as an enum). Used for tests which should be run for multiple CertVerifyProc
+// implementations.
+class CertVerifyProcTest
+    : public CertVerifyProcBaseTest,
+      public testing::WithParamInterface<CertVerifyProcName> {
+ protected:
+  scoped_refptr<CertVerifyProc> CreateVerifyProc(
+      CertVerifyProcName* verify_proc_name) override {
+    *verify_proc_name = GetParam();
+    return CreateCertVerifyProc(*verify_proc_name);
+  }
+};
+
+INSTANTIATE_TEST_CASE_P(,
+                        CertVerifyProcTest,
+                        testing::ValuesIn(AllCertVerifiers()),
+                        VerifyProcTypeToName);
+
+// This test fixture is used for tests that are ONLY to be run with the
+// CertVerifyProc::CreateDefault() implementation.

[Ryan Sleevi, 2017/01/13 17:22:09]

Did you consider making an explicit fixture for each of those tests?

[eroman, 2017/02/01 01:13:55]

That entails duplicating fixtures in at least 4 places. Do you want me to go
ahead with that?

+class CertVerifyProcDefaultTest : public CertVerifyProcBaseTest {
+ public:
+  scoped_refptr<CertVerifyProc> CreateVerifyProc(
+      CertVerifyProcName* verify_proc_name) override {
+    *verify_proc_name = GetDefaultCertVerifyProcName();
+    return CreateCertVerifyProc(*verify_proc_name);
+  }
 };
 
-#if defined(OS_ANDROID) || defined(USE_OPENSSL_CERTS)
-// TODO(jnd): http://crbug.com/117478 - EV verification is not yet supported.
-#define MAYBE_EVVerification DISABLED_EVVerification
-#else
 // TODO(rsleevi): Reenable this test once comodo.chaim.pem is no longer
 // expired, http://crbug.com/502818
-#define MAYBE_EVVerification DISABLED_EVVerification
-#endif
-TEST_F(CertVerifyProcTest, MAYBE_EVVerification) {
+TEST_P(CertVerifyProcTest, DISABLED_EVVerification) {
+  if (UsingCertVerifyProcAndroid() || UsingCertVerifyProcOpenSSL()) {
+    // TODO(jnd): http://crbug.com/117478 - EV verification is not yet
+    // supported.
+    LOG(INFO) << "Skipping test as EV verification is not yet supported";
+    return;
+  }
+
   CertificateList certs = CreateCertificateListFromFile(
       GetTestCertsDirectory(),
       "comodo.chain.pem",
@@ -241,7 +379,7 @@ TEST_F(CertVerifyProcTest, MAYBE_EVVerification) {
 // configurations, so disable the test until it is fixed (better to have
 // a bug to track a failing test than a false sense of security due to
 // false positive).
-TEST_F(CertVerifyProcTest, DISABLED_PaypalNullCertParsing) {
+TEST_P(CertVerifyProcTest, DISABLED_PaypalNullCertParsing) {
   // A certificate for www.paypal.com with a NULL byte in the common name.
   // From http://www.gossamer-threads.com/lists/fulldisc/full-disclosure/70363
   SHA256HashValue paypal_null_fingerprint = {{0x00}};
@@ -264,37 +402,39 @@ TEST_F(CertVerifyProcTest, DISABLED_PaypalNullCertParsing) {
                      NULL,
                      empty_cert_list_,
                      &verify_result);
-#if defined(USE_NSS_CERTS) || defined(OS_ANDROID)
-  EXPECT_THAT(error, IsError(ERR_CERT_COMMON_NAME_INVALID));
-#elif defined(OS_IOS) && TARGET_IPHONE_SIMULATOR
-  // iOS returns a ERR_CERT_INVALID error on the simulator, while returning
-  // ERR_CERT_AUTHORITY_INVALID on the real device.
-  EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
-#else
-  // TOOD(bulach): investigate why macosx and win aren't returning
-  // ERR_CERT_INVALID or ERR_CERT_COMMON_NAME_INVALID.
-  EXPECT_THAT(error, IsError(ERR_CERT_AUTHORITY_INVALID));
-#endif
+
+  if (UsingCertVerifyProcNSS() || UsingCertVerifyProcAndroid()) {
+    EXPECT_THAT(error, IsError(ERR_CERT_COMMON_NAME_INVALID));
+  } else if (UsingCertVerifyProcIOS() && TargetIsIphoneSimulator()) {
+    // iOS returns a ERR_CERT_INVALID error on the simulator, while returning
+    // ERR_CERT_AUTHORITY_INVALID on the real device.
+    EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
+  } else {
+    // TOOD(bulach): investigate why macosx and win aren't returning
+    // ERR_CERT_INVALID or ERR_CERT_COMMON_NAME_INVALID.
+    EXPECT_THAT(error, IsError(ERR_CERT_AUTHORITY_INVALID));
+  }
+
   // Either the system crypto library should correctly report a certificate
   // name mismatch, or our certificate blacklist should cause us to report an
   // invalid certificate.
-#if defined(USE_NSS_CERTS) || defined(OS_WIN)
-  EXPECT_TRUE(verify_result.cert_status &
-              (CERT_STATUS_COMMON_NAME_INVALID | CERT_STATUS_INVALID));
-#endif
+  if (UsingCertVerifyProcNSS() || UsingCertVerifyProcWin()) {
+    EXPECT_TRUE(verify_result.cert_status &
+                (CERT_STATUS_COMMON_NAME_INVALID | CERT_STATUS_INVALID));
+  }
+
+  // TODO(crbug.com/649017): What expectations to use for the other verifiers?
 }
 
 // A regression test for http://crbug.com/31497.
-#if defined(OS_ANDROID)
-// Disabled on Android, as the Android verification libraries require an
-// explicit policy to be specified, even when anyPolicy is permitted.
-#define MAYBE_IntermediateCARequireExplicitPolicy \
-    DISABLED_IntermediateCARequireExplicitPolicy
-#else
-#define MAYBE_IntermediateCARequireExplicitPolicy \
-    IntermediateCARequireExplicitPolicy
-#endif
-TEST_F(CertVerifyProcTest, MAYBE_IntermediateCARequireExplicitPolicy) {
+TEST_P(CertVerifyProcTest, IntermediateCARequireExplicitPolicy) {
+  if (UsingCertVerifyProcAndroid()) {
+    // Disabled on Android, as the Android verification libraries require an
+    // explicit policy to be specified, even when anyPolicy is permitted.
+    LOG(INFO) << "Skipping test on Android";
+    return;
+  }
+
   base::FilePath certs_dir = GetTestCertsDirectory();
 
   CertificateList certs = CreateCertificateListFromFile(
@@ -324,7 +464,7 @@ TEST_F(CertVerifyProcTest, MAYBE_IntermediateCARequireExplicitPolicy) {
   EXPECT_EQ(0u, verify_result.cert_status);
 }
 
-TEST_F(CertVerifyProcTest, RejectExpiredCert) {
+TEST_P(CertVerifyProcTest, RejectExpiredCert) {
   base::FilePath certs_dir = GetTestCertsDirectory();
 
   // Load root_ca_cert.pem into the test root store.
@@ -363,7 +503,7 @@ static bool IsWeakKeyType(const std::string& key_type) {
   return false;
 }
 
-TEST_F(CertVerifyProcTest, RejectWeakKeys) {
+TEST_P(CertVerifyProcTest, RejectWeakKeys) {
   base::FilePath certs_dir = GetTestCertsDirectory();
   typedef std::vector<std::string> Strings;
   Strings key_types;
@@ -418,8 +558,9 @@ TEST_F(CertVerifyProcTest, RejectWeakKeys) {
         EXPECT_NE(OK, error);
         EXPECT_EQ(CERT_STATUS_WEAK_KEY,
                   verify_result.cert_status & CERT_STATUS_WEAK_KEY);
-        EXPECT_EQ(WeakKeysAreInvalid() ? CERT_STATUS_INVALID : 0,
-                  verify_result.cert_status & CERT_STATUS_INVALID);
+        EXPECT_EQ(
+            WeakKeysAreInvalid(verify_proc_name_) ? CERT_STATUS_INVALID : 0,
+            verify_result.cert_status & CERT_STATUS_INVALID);
       } else {
         EXPECT_THAT(error, IsOk());
         EXPECT_EQ(0U, verify_result.cert_status & CERT_STATUS_WEAK_KEY);
@@ -429,20 +570,20 @@ TEST_F(CertVerifyProcTest, RejectWeakKeys) {
 }
 
 // Regression test for http://crbug.com/108514.
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-// Disabled on OS X - Security.framework doesn't ignore superflous certificates
-// provided by servers. See CertVerifyProcTest.CybertrustGTERoot for further
-// details.
-#define MAYBE_ExtraneousMD5RootCert DISABLED_ExtraneousMD5RootCert
-#else
-#define MAYBE_ExtraneousMD5RootCert ExtraneousMD5RootCert
-#endif
-TEST_F(CertVerifyProcTest, MAYBE_ExtraneousMD5RootCert) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, ExtraneousMD5RootCert) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
 
+  if (UsingCertVerifyProcMac()) {
+    // Disabled on OS X - Security.framework doesn't ignore superflous
+    // certificates provided by servers.

[Ryan Sleevi, 2017/01/12 20:01:56]

This is probably fixed by mattm's changes.

[eroman, 2017/02/01 01:13:55]

Acknowledged -- left a TODO

+    LOG(INFO) << "Skipping this test as Security.framework doesn't ignore "
+                 "superflous certificates provided by servers.";
+    return;
+  }
+
   base::FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> server_cert =
@@ -487,7 +628,7 @@ TEST_F(CertVerifyProcTest, MAYBE_ExtraneousMD5RootCert) {
 }
 
 // Test for bug 94673.
-TEST_F(CertVerifyProcTest, GoogleDigiNotarTest) {
+TEST_P(CertVerifyProcTest, GoogleDigiNotarTest) {
   base::FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> server_cert =
@@ -528,7 +669,7 @@ TEST_F(CertVerifyProcTest, GoogleDigiNotarTest) {
 
 // Ensures the CertVerifyProc blacklist remains in sorted order, so that it
 // can be binary-searched.
-TEST_F(CertVerifyProcTest, BlacklistIsSorted) {
+TEST_P(CertVerifyProcTest, BlacklistIsSorted) {
 // Defines kBlacklistedSPKIs.
 #include "net/cert/cert_verify_proc_blacklist.inc"
   for (size_t i = 0; i < arraysize(kBlacklistedSPKIs) - 1; ++i) {
@@ -538,7 +679,7 @@ TEST_F(CertVerifyProcTest, BlacklistIsSorted) {
   }
 }
 
-TEST_F(CertVerifyProcTest, DigiNotarCerts) {
+TEST_P(CertVerifyProcTest, DigiNotarCerts) {
   static const char* const kDigiNotarFilenames[] = {
     "diginotar_root_ca.pem",
     "diginotar_cyber_ca.pem",
@@ -573,7 +714,7 @@ TEST_F(CertVerifyProcTest, DigiNotarCerts) {
   }
 }
 
-TEST_F(CertVerifyProcTest, NameConstraintsOk) {
+TEST_P(CertVerifyProcTest, NameConstraintsOk) {
   CertificateList ca_cert_list =
       CreateCertificateListFromFile(GetTestCertsDirectory(),
                                     "root_ca_cert.pem",
@@ -608,8 +749,8 @@ TEST_F(CertVerifyProcTest, NameConstraintsOk) {
   EXPECT_EQ(0U, verify_result.cert_status);
 }
 
-TEST_F(CertVerifyProcTest, NameConstraintsFailure) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, NameConstraintsFailure) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -644,7 +785,7 @@ TEST_F(CertVerifyProcTest, NameConstraintsFailure) {
             verify_result.cert_status & CERT_STATUS_NAME_CONSTRAINT_VIOLATION);
 }
 
-TEST_F(CertVerifyProcTest, TestHasTooLongValidity) {
+TEST_P(CertVerifyProcTest, TestHasTooLongValidity) {
   struct {
     const char* const file;
     bool is_valid_too_long;
@@ -675,8 +816,8 @@ TEST_F(CertVerifyProcTest, TestHasTooLongValidity) {
 }
 
 // TODO(crbug.com/610546): Fix and re-enable this test.
-TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) {
-  if (!SupportsDetectingKnownRoots()) {
+TEST_P(CertVerifyProcTest, DISABLED_TestKnownRoot) {
+  if (!SupportsDetectingKnownRoots(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test on this platform.";
     return;
   }
@@ -704,8 +845,8 @@ TEST_F(CertVerifyProcTest, DISABLED_TestKnownRoot) {
 }
 
 // TODO(crbug.com/610546): Fix and re-enable this test.
-TEST_F(CertVerifyProcTest, DISABLED_PublicKeyHashes) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, DISABLED_PublicKeyHashes) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -761,7 +902,7 @@ TEST_F(CertVerifyProcTest, DISABLED_PublicKeyHashes) {
 // A regression test for http://crbug.com/70293.
 // The Key Usage extension in this RSA SSL server certificate does not have
 // the keyEncipherment bit.
-TEST_F(CertVerifyProcTest, InvalidKeyUsage) {
+TEST_P(CertVerifyProcTest, InvalidKeyUsage) {

[Ryan Sleevi, 2017/01/12 20:01:56]

This seems like one of those tests we should be using our own files for, and if
so, would eliminate the result divergence.

[eroman, 2017/02/01 01:13:55]

Acknowledged -- left a TODO

   base::FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> server_cert =
@@ -770,27 +911,27 @@ TEST_F(CertVerifyProcTest, InvalidKeyUsage) {
 
   int flags = 0;
   CertVerifyResult verify_result;
-  int error = Verify(server_cert.get(),
-                     "jira.aquameta.com",
-                     flags,
-                     NULL,
-                     empty_cert_list_,
-                     &verify_result);
-#if defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID)
-  // This certificate has two errors: "invalid key usage" and "untrusted CA".
-  // However, OpenSSL returns only one (the latter), and we can't detect
-  // the other errors.
-  EXPECT_THAT(error, IsError(ERR_CERT_AUTHORITY_INVALID));
-#else
-  EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
-  EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_INVALID);
-#endif
+  int error = Verify(server_cert.get(), "jira.aquameta.com", flags, NULL,
+                     empty_cert_list_, &verify_result);
+
+  if (UsingCertVerifyProcOpenSSL()) {
+    // This certificate has two errors: "invalid key usage" and "untrusted CA".
+    // However, OpenSSL returns only one (the latter), and we can't detect
+    // the other errors.
+    EXPECT_THAT(error, IsError(ERR_CERT_AUTHORITY_INVALID));
+  } else {
+    EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
+    EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_INVALID);
+  }
   // TODO(wtc): fix http://crbug.com/75520 to get all the certificate errors
   // from NSS.
-#if !defined(USE_NSS_CERTS) && !defined(OS_IOS) && !defined(OS_ANDROID)
-  // The certificate is issued by an unknown CA.
-  EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_AUTHORITY_INVALID);
-#endif
+  if (!UsingCertVerifyProcNSS() && !UsingCertVerifyProcIOS() &&
+      !UsingCertVerifyProcAndroid()) {
+    // The certificate is issued by an unknown CA.
+    EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_AUTHORITY_INVALID);
+  }
+
+  // TODO(crbug.com/649017): What expectations to use for the other verifiers?
 }
 
 // Basic test for returning the chain in CertVerifyResult. Note that the
@@ -799,8 +940,8 @@ TEST_F(CertVerifyProcTest, InvalidKeyUsage) {
 // of the certificate to be verified. The remaining VerifyReturn* tests are
 // used to ensure that the actual, verified chain is being returned by
 // Verify().
-TEST_F(CertVerifyProcTest, VerifyReturnChainBasic) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, VerifyReturnChainBasic) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -853,8 +994,8 @@ TEST_F(CertVerifyProcTest, VerifyReturnChainBasic) {
 // known public registry controlled domain information) issued by well-known
 // CAs are flagged appropriately, while certificates that are issued by
 // internal CAs are not flagged.
-TEST_F(CertVerifyProcTest, IntranetHostsRejected) {
-  if (!SupportsDetectingKnownRoots()) {
+TEST_P(CertVerifyProcTest, IntranetHostsRejected) {
+  if (!SupportsDetectingKnownRoots(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -893,7 +1034,7 @@ TEST_F(CertVerifyProcTest, IntranetHostsRejected) {
 // that were issued after 1 January 2016, while still allowing those from
 // before that date, with SHA-1 in the intermediate, or from an enterprise
 // CA.
-TEST_F(CertVerifyProcTest, VerifyRejectsSHA1AfterDeprecationLegacyMode) {
+TEST_P(CertVerifyProcTest, VerifyRejectsSHA1AfterDeprecationLegacyMode) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(CertVerifyProc::kSHA1LegacyMode);
 
@@ -976,8 +1117,8 @@ TEST_F(CertVerifyProcTest, VerifyRejectsSHA1AfterDeprecationLegacyMode) {
 // a protocol violation if sent during a TLS handshake, if multiple sources
 // of intermediate certificates are combined, it's possible that order may
 // not be maintained.
-TEST_F(CertVerifyProcTest, VerifyReturnChainProperlyOrdered) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, VerifyReturnChainProperlyOrdered) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -1029,8 +1170,8 @@ TEST_F(CertVerifyProcTest, VerifyReturnChainProperlyOrdered) {
 
 // Test that Verify() filters out certificates which are not related to
 // or part of the certificate chain being verified.
-TEST_F(CertVerifyProcTest, VerifyReturnChainFiltersUnrelatedCerts) {
-  if (!SupportsReturningVerifiedChain()) {
+TEST_P(CertVerifyProcTest, VerifyReturnChainFiltersUnrelatedCerts) {
+  if (!SupportsReturningVerifiedChain(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
   }
@@ -1088,7 +1229,7 @@ TEST_F(CertVerifyProcTest, VerifyReturnChainFiltersUnrelatedCerts) {
                                             certs[2]->os_cert_handle()));
 }
 
-TEST_F(CertVerifyProcTest, AdditionalTrustAnchors) {
+TEST_P(CertVerifyProcTest, AdditionalTrustAnchors) {
   if (!SupportsAdditionalTrustAnchors()) {
     LOG(INFO) << "Skipping this test in this platform.";
     return;
@@ -1138,7 +1279,7 @@ TEST_F(CertVerifyProcTest, AdditionalTrustAnchors) {
 // Tests that certificates issued by user-supplied roots are not flagged as
 // issued by a known root. This should pass whether or not the platform supports
 // detecting known roots.
-TEST_F(CertVerifyProcTest, IsIssuedByKnownRootIgnoresTestRoots) {
+TEST_P(CertVerifyProcTest, IsIssuedByKnownRootIgnoresTestRoots) {
   // Load root_ca_cert.pem into the test root store.
   ScopedTestRoot test_root(
       ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem").get());
@@ -1157,11 +1298,21 @@ TEST_F(CertVerifyProcTest, IsIssuedByKnownRootIgnoresTestRoots) {
   EXPECT_FALSE(verify_result.is_issued_by_known_root);
 }
 
-#if defined(USE_NSS_CERTS) || defined(OS_WIN) || \
-    (defined(OS_MACOSX) && !defined(OS_IOS))
+bool SupportsCRLSet(CertVerifyProcName verify_proc_name) {
+  // TODO(crbug.com/649017): Support for built-in.
+  return verify_proc_name == CERT_VERIFY_PROC_NSS ||
+         verify_proc_name == CERT_VERIFY_PROC_WIN ||
+         verify_proc_name == CERT_VERIFY_PROC_MAC;

[Ryan Sleevi, 2017/01/12 20:01:56]

I wonder whether these should be different tests (like OCSP)

[eroman, 2017/02/01 01:13:54]

When you say "tests" do you mean unit-tests, or do you mean adding something
like CertVerifyProc::SupportsCRLSet ?

+}
+
 // Test that CRLSets are effective in making a certificate appear to be
 // revoked.
-TEST_F(CertVerifyProcTest, CRLSet) {
+TEST_P(CertVerifyProcTest, CRLSet) {
+  if (!SupportsCRLSet(verify_proc_name_)) {
+    LOG(INFO) << "Skipping test as verifier doesn't support CRLSet";
+    return;
+  }
+
   CertificateList ca_cert_list =
       CreateCertificateListFromFile(GetTestCertsDirectory(),
                                     "root_ca_cert.pem",
@@ -1215,7 +1366,12 @@ TEST_F(CertVerifyProcTest, CRLSet) {
   EXPECT_THAT(error, IsError(ERR_CERT_REVOKED));
 }
 
-TEST_F(CertVerifyProcTest, CRLSetLeafSerial) {
+TEST_P(CertVerifyProcTest, CRLSetLeafSerial) {
+  if (!SupportsCRLSet(verify_proc_name_)) {
+    LOG(INFO) << "Skipping test as verifier doesn't support CRLSet";
+    return;
+  }
+
   CertificateList ca_cert_list =
       CreateCertificateListFromFile(GetTestCertsDirectory(), "root_ca_cert.pem",
                                     X509Certificate::FORMAT_AUTO);
@@ -1271,8 +1427,8 @@ TEST_F(CertVerifyProcTest, CRLSetLeafSerial) {
 // 1. Revoking E by SPKI, so that only Path 1 is valid (as E is in Paths 2 & 3)
 // 2. Revoking C(D) and F(E) by serial, so that only Path 2 is valid.
 // 3. Revoking C by SPKI, so that only Path 3 is valid (as C is in Paths 1 & 2)
-TEST_F(CertVerifyProcTest, CRLSetDuringPathBuilding) {
-  if (!SupportsCRLSetsInPathBuilding()) {
+TEST_P(CertVerifyProcTest, CRLSetDuringPathBuilding) {
+  if (!SupportsCRLSetsInPathBuilding(verify_proc_name_)) {
     LOG(INFO) << "Skipping this test on this platform.";
     return;
   }
@@ -1370,8 +1526,6 @@ TEST_F(CertVerifyProcTest, CRLSetDuringPathBuilding) {
   }
 }
 
-#endif
-
 #if defined(OS_MACOSX) && !defined(OS_IOS)
 // Test that a CRLSet blocking one of the intermediates supplied by the server
 // can be worked around by the chopping workaround for path building. (Once the
@@ -1389,7 +1543,7 @@ TEST_F(CertVerifyProcTest, CRLSetDuringPathBuilding) {
 //
 // The verifier should rollback until it just tries A(B) alone, at which point
 // it will pull B(F) & F(E) from the keychain and succeed.
-TEST_F(CertVerifyProcTest, MacCRLIntermediate) {
+TEST_F(CertVerifyProcDefaultTest, MacCRLIntermediate) {
   if (base::mac::IsAtLeastOS10_12()) {
     // TODO(crbug.com/671889): Investigate SecTrustSetKeychains issue on Sierra.
     LOG(INFO) << "Skipping test, SecTrustSetKeychains does not work on 10.12";
@@ -1472,7 +1626,7 @@ TEST_F(CertVerifyProcTest, MacCRLIntermediate) {
 // Test that if a keychain is present which trusts a less-desirable root (ex,
 // one using SHA1), that the keychain reordering hack will cause the better
 // root in the System Roots to be used instead.
-TEST_F(CertVerifyProcTest, MacKeychainReordering) {
+TEST_F(CertVerifyProcDefaultTest, MacKeychainReordering) {
   // Note: target cert expires Apr  2 23:59:59 2018 GMT
   scoped_refptr<X509Certificate> cert = CreateCertificateChainFromFile(
       GetTestCertsDirectory(), "tripadvisor-verisign-chain.pem",
@@ -1516,7 +1670,7 @@ TEST_F(CertVerifyProcTest, MacKeychainReordering) {
 // Test that the system root certificate keychain is in the expected location
 // and can be opened. Other tests would fail if this was not true, but this
 // test makes the reason for the failure obvious.
-TEST_F(CertVerifyProcTest, MacSystemRootCertificateKeychainLocation) {
+TEST_F(CertVerifyProcDefaultTest, MacSystemRootCertificateKeychainLocation) {
   const char* root_keychain_path =
       "/System/Library/Keychains/SystemRootCertificates.keychain";
   ASSERT_TRUE(base::PathExists(base::FilePath(root_keychain_path)));
@@ -1526,20 +1680,21 @@ TEST_F(CertVerifyProcTest, MacSystemRootCertificateKeychainLocation) {
   ASSERT_EQ(errSecSuccess, status);
   CFRelease(keychain);
 }
-#endif
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
-bool AreSHA1IntermediatesAllowed() {
+bool AreSHA1IntermediatesAllowed(CertVerifyProcName verify_proc_name) {
 #if defined(OS_WIN)
   // TODO(rsleevi): Remove this once https://crbug.com/588789 is resolved
   // for Windows 7/2008 users.
   // Note: This must be kept in sync with cert_verify_proc.cc
-  return base::win::GetVersion() < base::win::VERSION_WIN8;
+  return verify_proc_name == CERT_VERIFY_PROC_WIN &&
+         base::win::GetVersion() < base::win::VERSION_WIN8;
 #else
   return false;
 #endif
 }
 
-TEST_F(CertVerifyProcTest, RejectsMD2) {
+TEST_P(CertVerifyProcTest, RejectsMD2) {
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
   ASSERT_TRUE(cert);
@@ -1556,7 +1711,7 @@ TEST_F(CertVerifyProcTest, RejectsMD2) {
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_INVALID);
 }
 
-TEST_F(CertVerifyProcTest, RejectsMD4) {
+TEST_P(CertVerifyProcTest, RejectsMD4) {
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
   ASSERT_TRUE(cert);
@@ -1573,7 +1728,7 @@ TEST_F(CertVerifyProcTest, RejectsMD4) {
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_INVALID);
 }
 
-TEST_F(CertVerifyProcTest, RejectsMD5) {
+TEST_P(CertVerifyProcTest, RejectsMD5) {
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
   ASSERT_TRUE(cert);
@@ -1590,7 +1745,7 @@ TEST_F(CertVerifyProcTest, RejectsMD5) {
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_WEAK_SIGNATURE_ALGORITHM);
 }
 
-TEST_F(CertVerifyProcTest, RejectsPublicSHA1Leaves) {
+TEST_P(CertVerifyProcTest, RejectsPublicSHA1Leaves) {
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
   ASSERT_TRUE(cert);
@@ -1609,7 +1764,7 @@ TEST_F(CertVerifyProcTest, RejectsPublicSHA1Leaves) {
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_WEAK_SIGNATURE_ALGORITHM);
 }
 
-TEST_F(CertVerifyProcTest, RejectsPublicSHA1IntermediatesUnlessAllowed) {
+TEST_P(CertVerifyProcTest, RejectsPublicSHA1IntermediatesUnlessAllowed) {
   scoped_refptr<X509Certificate> cert(ImportCertFromFile(
       GetTestCertsDirectory(), "39_months_after_2015_04.pem"));
   ASSERT_TRUE(cert);
@@ -1624,7 +1779,7 @@ TEST_F(CertVerifyProcTest, RejectsPublicSHA1IntermediatesUnlessAllowed) {
   CertVerifyResult verify_result;
   int error = Verify(cert.get(), "127.0.0.1", flags, nullptr /* crl_set */,
                      empty_cert_list_, &verify_result);
-  if (AreSHA1IntermediatesAllowed()) {
+  if (AreSHA1IntermediatesAllowed(verify_proc_name_)) {
     EXPECT_THAT(error, IsOk());
     EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_SHA1_SIGNATURE_PRESENT);
   } else {
@@ -1634,7 +1789,7 @@ TEST_F(CertVerifyProcTest, RejectsPublicSHA1IntermediatesUnlessAllowed) {
   }
 }
 
-TEST_F(CertVerifyProcTest, RejectsPrivateSHA1UnlessFlag) {
+TEST_P(CertVerifyProcTest, RejectsPrivateSHA1UnlessFlag) {
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
   ASSERT_TRUE(cert);
@@ -1694,15 +1849,16 @@ void PrintTo(const WeakDigestTestData& data, std::ostream* os) {
       << "; end-entity: " << data.ee_cert_filename;
 }
 
+// This test fixture does not subclass CertVerifyProcBaseTest as it is testing
+// a mock CertVerifyProc rather than one of the concrete types.

[Ryan Sleevi, 2017/01/12 20:01:56]

My two cents is I'm not sure that this comment is necessary.

[eroman, 2017/02/01 01:13:54]

Done (removed)

Alternately I could re-write it as I had in the follow-up patchset:

// Tests to ensure that CertVerifyProc::Verify() sets the has_* members for
// hash algorithms.

 class CertVerifyProcWeakDigestTest
-    : public CertVerifyProcTest,
-      public testing::WithParamInterface<WeakDigestTestData> {
+    : public testing::TestWithParam<WeakDigestTestData> {
  public:
   CertVerifyProcWeakDigestTest() {}
   virtual ~CertVerifyProcWeakDigestTest() {}
 };
 
-// Test that the CertVerifyProc::Verify() properly surfaces the (weak) hashing
+// Tests that the CertVerifyProc::Verify() properly surfaces the (weak) hash
 // algorithms used in the chain.
 TEST_P(CertVerifyProcWeakDigestTest, VerifyDetectsAlgorithm) {
   WeakDigestTestData data = GetParam();
@@ -1743,11 +1899,11 @@ TEST_P(CertVerifyProcWeakDigestTest, VerifyDetectsAlgorithm) {
   // |ee_chain|.
   //
   // This is sufficient for the purposes of this test, as the checking for weak
-  // hashing algorithms is done by CertVerifyProc::Verify().
+  // hash algorithms is done by CertVerifyProc::Verify().
   scoped_refptr<CertVerifyProc> proc =
       new MockCertVerifyProc(CertVerifyResult());
   proc->Verify(ee_chain.get(), "127.0.0.1", std::string(), flags, nullptr,
-               empty_cert_list_, &verify_result);
+               CertificateList(), &verify_result);
   EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD2), verify_result.has_md2);
   EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD4), verify_result.has_md4);
   EXPECT_EQ(!!(data.expected_algorithms & EXPECT_MD5), verify_result.has_md5);
@@ -1864,67 +2020,109 @@ INSTANTIATE_TEST_CASE_P(VerifyTrustedEE,
 
 // For the list of valid hostnames, see
 // net/cert/data/ssl/certificates/subjectAltName_sanity_check.pem
-static const struct CertVerifyProcNameData {
+struct CertVerifyProcNameData {
   const char* hostname;
   bool valid;  // Whether or not |hostname| matches a subjectAltName.
-} kVerifyNameData[] = {
-  { "127.0.0.1", false },  // Don't match the common name
-  { "127.0.0.2", true },  // Matches the iPAddress SAN (IPv4)
-  { "FE80:0:0:0:0:0:0:1", true },  // Matches the iPAddress SAN (IPv6)
-  { "[FE80:0:0:0:0:0:0:1]", false },  // Should not match the iPAddress SAN
-  { "FE80::1", true },  // Compressed form matches the iPAddress SAN (IPv6)
-  { "::127.0.0.2", false },  // IPv6 mapped form should NOT match iPAddress SAN
-  { "test.example", true },  // Matches the dNSName SAN
-  { "test.example.", true },  // Matches the dNSName SAN (trailing . ignored)
-  { "www.test.example", false },  // Should not match the dNSName SAN
-  { "test..example", false },  // Should not match the dNSName SAN
-  { "test.example..", false },  // Should not match the dNSName SAN
-  { ".test.example.", false },  // Should not match the dNSName SAN
-  { ".test.example", false },  // Should not match the dNSName SAN
 };
 
-// GTest 'magic' pretty-printer, so that if/when a test fails, it knows how
-// to output the parameter that was passed. Without this, it will simply
-// attempt to print out the first twenty bytes of the object, which depending
-// on platform and alignment, may result in an invalid read.
-void PrintTo(const CertVerifyProcNameData& data, std::ostream* os) {
-  *os << "Hostname: " << data.hostname << "; valid=" << data.valid;
-}
-
-class CertVerifyProcNameTest
-    : public CertVerifyProcTest,
-      public testing::WithParamInterface<CertVerifyProcNameData> {
+// Test fixture for verifying certificate names.
+class CertVerifyProcNameTest : public CertVerifyProcTest {
  public:
   CertVerifyProcNameTest() {}
   virtual ~CertVerifyProcNameTest() {}
+
+ protected:
+  void VerifyCertName(const char* hostname, bool valid) {
+    CertificateList cert_list = CreateCertificateListFromFile(
+        GetTestCertsDirectory(), "subjectAltName_sanity_check.pem",
+        X509Certificate::FORMAT_AUTO);
+    ASSERT_EQ(1U, cert_list.size());
+    scoped_refptr<X509Certificate> cert(cert_list[0]);
+
+    ScopedTestRoot scoped_root(cert.get());
+
+    CertVerifyResult verify_result;
+    int error =
+        Verify(cert.get(), hostname, 0, NULL, empty_cert_list_, &verify_result);
+    if (valid) {
+      EXPECT_THAT(error, IsOk());
+      EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_COMMON_NAME_INVALID);
+    } else {
+      EXPECT_THAT(error, IsError(ERR_CERT_COMMON_NAME_INVALID));
+      EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_COMMON_NAME_INVALID);
+    }
+  }
 };
 
-TEST_P(CertVerifyProcNameTest, VerifyCertName) {
-  CertVerifyProcNameData data = GetParam();
+// Don't match the common name
+TEST_P(CertVerifyProcNameTest, DontMatchCommonName) {
+  VerifyCertName("127.0.0.1", false);
+}
 
-  CertificateList cert_list = CreateCertificateListFromFile(
-      GetTestCertsDirectory(), "subjectAltName_sanity_check.pem",
-      X509Certificate::FORMAT_AUTO);
-  ASSERT_EQ(1U, cert_list.size());
-  scoped_refptr<X509Certificate> cert(cert_list[0]);
+// Matches the iPAddress SAN (IPv4)
+TEST_P(CertVerifyProcNameTest, MatchesIpSanIpv4) {
+  VerifyCertName("127.0.0.2", true);
+}
 
-  ScopedTestRoot scoped_root(cert.get());
+// Matches the iPAddress SAN (IPv6)
+TEST_P(CertVerifyProcNameTest, MatchesIpSanIpv6) {
+  VerifyCertName("FE80:0:0:0:0:0:0:1", true);
+}
 
-  CertVerifyResult verify_result;
-  int error = Verify(cert.get(), data.hostname, 0, NULL, empty_cert_list_,
-                     &verify_result);
-  if (data.valid) {
-    EXPECT_THAT(error, IsOk());
-    EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_COMMON_NAME_INVALID);
-  } else {
-    EXPECT_THAT(error, IsError(ERR_CERT_COMMON_NAME_INVALID));
-    EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_COMMON_NAME_INVALID);
-  }
+// Should not match the iPAddress SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchIpSanIpv6) {
+  VerifyCertName("[FE80:0:0:0:0:0:0:1]", false);
+}
+
+// Compressed form matches the iPAddress SAN (IPv6)
+TEST_P(CertVerifyProcNameTest, MatchesIpSanCompressedIpv6) {
+  VerifyCertName("FE80::1", true);
+}
+
+// IPv6 mapped form should NOT match iPAddress SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchIpSanIPv6Mapped) {
+  VerifyCertName("::127.0.0.2", false);
+}
+
+// Matches the dNSName SAN
+TEST_P(CertVerifyProcNameTest, MatchesDnsSan) {
+  VerifyCertName("test.example", true);
+}
+
+// Matches the dNSName SAN (trailing . ignored)
+TEST_P(CertVerifyProcNameTest, MatchesDnsSanTrailingDot) {
+  VerifyCertName("test.example.", true);
+}
+
+// Should not match the dNSName SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchDnsSan) {
+  VerifyCertName("www.test.example", false);
+}
+
+// Should not match the dNSName SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchDnsSanInvalid) {
+  VerifyCertName("test..example", false);
+}
+
+// Should not match the dNSName SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchDnsSanTwoTrailingDots) {
+  VerifyCertName("test.example..", false);
+}
+
+// Should not match the dNSName SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchDnsSanLeadingAndTrailingDot) {
+  VerifyCertName(".test.example.", false);
+}
+
+// Should not match the dNSName SAN
+TEST_P(CertVerifyProcNameTest, DoesntMatchDnsSanTrailingDot) {
+  VerifyCertName(".test.example", false);
 }
 
 INSTANTIATE_TEST_CASE_P(VerifyName,
                         CertVerifyProcNameTest,
-                        testing::ValuesIn(kVerifyNameData));
+                        testing::ValuesIn(AllCertVerifiers()),
+                        VerifyProcTypeToName);
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
 // Test that CertVerifyProcMac reacts appropriately when Apple's certificate
@@ -1933,7 +2131,7 @@ INSTANTIATE_TEST_CASE_P(VerifyName,
 // (Since 10.12, this causes a recoverable error instead of a fatal one.)
 // TODO(mattm): Try to find a different way to cause a fatal error that works
 // on 10.12.
-TEST_F(CertVerifyProcTest, LargeKey) {
+TEST_F(CertVerifyProcDefaultTest, LargeKey) {
   // Load root_ca_cert.pem into the test root store.
   ScopedTestRoot test_root(
       ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem").get());
@@ -1956,7 +2154,7 @@ TEST_F(CertVerifyProcTest, LargeKey) {
 // Tests that CertVerifyProc records a histogram correctly when a
 // certificate chaining to a private root contains the TLS feature
 // extension and does not have a stapled OCSP response.
-TEST_F(CertVerifyProcTest, HasTLSFeatureExtensionUMA) {
+TEST_P(CertVerifyProcTest, HasTLSFeatureExtensionUMA) {
   base::HistogramTester histograms;
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "tls_feature_extension.pem"));
@@ -1982,7 +2180,7 @@ TEST_F(CertVerifyProcTest, HasTLSFeatureExtensionUMA) {
 // Tests that CertVerifyProc records a histogram correctly when a
 // certificate chaining to a private root contains the TLS feature
 // extension and does have a stapled OCSP response.
-TEST_F(CertVerifyProcTest, HasTLSFeatureExtensionWithStapleUMA) {
+TEST_P(CertVerifyProcTest, HasTLSFeatureExtensionWithStapleUMA) {
   base::HistogramTester histograms;
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "tls_feature_extension.pem"));
@@ -2009,7 +2207,7 @@ TEST_F(CertVerifyProcTest, HasTLSFeatureExtensionWithStapleUMA) {
 // Tests that CertVerifyProc records a histogram correctly when a
 // certificate chaining to a private root does not contain the TLS feature
 // extension.
-TEST_F(CertVerifyProcTest, DoesNotHaveTLSFeatureExtensionUMA) {
+TEST_P(CertVerifyProcTest, DoesNotHaveTLSFeatureExtensionUMA) {
   base::HistogramTester histograms;
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
@@ -2034,7 +2232,7 @@ TEST_F(CertVerifyProcTest, DoesNotHaveTLSFeatureExtensionUMA) {
 // Tests that CertVerifyProc does not record a histogram when a
 // certificate contains the TLS feature extension but chains to a public
 // root.
-TEST_F(CertVerifyProcTest, HasTLSFeatureExtensionWithPublicRootUMA) {
+TEST_P(CertVerifyProcTest, HasTLSFeatureExtensionWithPublicRootUMA) {
   base::HistogramTester histograms;
   scoped_refptr<X509Certificate> cert(
       ImportCertFromFile(GetTestCertsDirectory(), "tls_feature_extension.pem"));