Remove the test-only X509Certificate constructor

net/ssl/ssl_client_auth_cache_unittest.cc

Index: net/ssl/ssl_client_auth_cache_unittest.cc
diff --git a/net/ssl/ssl_client_auth_cache_unittest.cc b/net/ssl/ssl_client_auth_cache_unittest.cc
index 3107ab8a605083f21f52519f57fdd17a0b8e4aa9..89a23c6094aa2110af6d9c0809e26630a1cbdb2d 100644
--- a/net/ssl/ssl_client_auth_cache_unittest.cc
+++ b/net/ssl/ssl_client_auth_cache_unittest.cc
@@ -6,8 +6,10 @@
 
 #include "base/macros.h"
 #include "base/time/time.h"
+#include "net/base/test_data_directory.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/ssl_private_key.h"
+#include "net/test/cert_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -43,64 +45,64 @@ class MockSSLPrivateKey : public SSLPrivateKey {
 TEST(SSLClientAuthCacheTest, LookupAddRemove) {
   SSLClientAuthCache cache;
 
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
-
   HostPortPair server1("foo1", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo1", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   HostPortPair server2("foo2", 443);
   scoped_refptr<X509Certificate> cert2(
-      new X509Certificate("foo2", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));
+  ASSERT_TRUE(cert2);
 
   HostPortPair server3("foo3", 443);
   scoped_refptr<X509Certificate> cert3(
-    new X509Certificate("foo3", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "root_ca_cert.pem"));
+  ASSERT_TRUE(cert3);
 
   scoped_refptr<X509Certificate> cached_cert;
   scoped_refptr<SSLPrivateKey> cached_pkey;
   // Lookup non-existent client certificate.
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
 
   // Add client certificate for server1.
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1, cached_cert);
 
   // Add client certificate for server2.
   cache.Add(server2, cert2.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1.get(), cached_cert.get());
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Overwrite the client certificate for server1.
   cache.Add(server1, cert3.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert3, cached_cert);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Remove client certificate of server1.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 
   // Remove non-existent client certificate.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert2, cached_cert);
 }
@@ -110,16 +112,15 @@ TEST(SSLClientAuthCacheTest, LookupAddRemove) {
 TEST(SSLClientAuthCacheTest, LookupWithPort) {
   SSLClientAuthCache cache;
 
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
-
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   HostPortPair server2("foo", 8443);
   scoped_refptr<X509Certificate> cert2(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem"));

[eroman, 2016/06/22 16:51:08]

Any reason for changing the test pattern here? (i.e. re-import "ok_cert.pem"
here).
Looks like all the test cares about is that cert1 and cert2 be different
pointers, not data.

I don't really care either way, just reviewing the intent of this change.

[Ryan Sleevi, 2016/06/22 17:31:45]

I'm not sure I follow. Were you suggesting to reimport ok_cert.pem? Not sure why
the comment here vs say line 55-61

If I reused ok_cert.pem, I would have had to sprinkle in assertions that
cert1.get() != cert2.get() and that seemed unnecessary to add new preconditions.

[eroman, 2016/06/22 17:38:21]

55-61 isn't changing the code pattern, whereas this is.

[Ryan Sleevi, 2016/06/22 20:27:33]

I suspect I'm not understanding what you mean by "changing the code pattern". As
best I can understand, you're talking about cert2 having the same input
parameters ("foo", "CA", etc) with the expectation of returning a different
cert, and assuming that was somehow intentional or meaningful. Not really, no -
it was intended that they be 'different' certs (and X509Certificate() has always
obeyed that, as is required by a ctor). However,
X509Certificate::CreateFrom(...) has historically not obeyed that, and that the
other constructor was intentionally private because of this.

That is to say, this change reflects exactly what the test was trying to do and
testing, even if it didn't change "foo" to "foo2", so no, it's not a change in
code pattern.

+  ASSERT_TRUE(cert2);
 
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
   cache.Add(server2, cert2.get(), new MockSSLPrivateKey);
@@ -132,58 +133,56 @@ TEST(SSLClientAuthCacheTest, LookupWithPort) {
   EXPECT_EQ(cert2.get(), cached_cert.get());
 }
 
-// Check that the a NULL certificate, indicating the user has declined to send
-// a certificate, is properly cached.
+// Check that the a nullptr certificate, indicating the user has declined to
+// send a certificate, is properly cached.
 TEST(SSLClientAuthCacheTest, LookupNullPreference) {
   SSLClientAuthCache cache;
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
 
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
-  cache.Add(server1, NULL, new MockSSLPrivateKey);
+  cache.Add(server1, nullptr, new MockSSLPrivateKey);
 
   scoped_refptr<X509Certificate> cached_cert(cert1);
   scoped_refptr<SSLPrivateKey> cached_pkey;
-  // Make sure that |cached_cert| is updated to NULL, indicating the user
+  // Make sure that |cached_cert| is updated to nullptr, indicating the user
   // declined to send a certificate to |server1|.
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 
   // Remove the existing cached certificate.
   cache.Remove(server1);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));
 
   // Add a new preference for a specific certificate.
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
-  cached_cert = NULL;
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
   EXPECT_EQ(cert1, cached_cert);
 
-  // Replace the specific preference with a NULL certificate.
-  cache.Add(server1, NULL, new MockSSLPrivateKey);
-  cached_cert = NULL;
+  // Replace the specific preference with a nullptr certificate.
+  cache.Add(server1, nullptr, new MockSSLPrivateKey);
+  cached_cert = nullptr;
   EXPECT_TRUE(cache.Lookup(server1, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 }
 
 // Check that the OnCertAdded() method removes all cache entries.
 TEST(SSLClientAuthCacheTest, OnCertAdded) {
   SSLClientAuthCache cache;
-  base::Time start_date = base::Time::Now();
-  base::Time expiration_date = start_date + base::TimeDelta::FromDays(1);
 
   HostPortPair server1("foo", 443);
   scoped_refptr<X509Certificate> cert1(
-      new X509Certificate("foo", "CA", start_date, expiration_date));
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
+  ASSERT_TRUE(cert1);
 
   cache.Add(server1, cert1.get(), new MockSSLPrivateKey);
 
   HostPortPair server2("foo2", 443);
-  cache.Add(server2, NULL, new MockSSLPrivateKey);
+  cache.Add(server2, nullptr, new MockSSLPrivateKey);
 
   scoped_refptr<X509Certificate> cached_cert;
   scoped_refptr<SSLPrivateKey> cached_pkey;
@@ -193,9 +192,9 @@ TEST(SSLClientAuthCacheTest, OnCertAdded) {
   EXPECT_EQ(cert1, cached_cert);
 
   EXPECT_TRUE(cache.Lookup(server2, &cached_cert, &cached_pkey));
-  EXPECT_EQ(NULL, cached_cert.get());
+  EXPECT_EQ(nullptr, cached_cert.get());
 
-  cache.OnCertAdded(NULL);
+  cache.OnCertAdded(nullptr);
 
   // Check that we no longer have entries for either server.
   EXPECT_FALSE(cache.Lookup(server1, &cached_cert, &cached_pkey));