Make members of Singleton<T> private and only visible to the singleton type.

net/base/cert_database_nss_unittest.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <cert.h>
 #include <pk11pub.h>
 
 #include <algorithm>
 
 #include "base/crypto/scoped_nss_types.h"
@@ skipping 85 matching lines @@
 }
 
 }  // namespace
 
 // TODO(mattm): when https://bugzilla.mozilla.org/show_bug.cgi?id=588269 is
 // fixed, switch back to using a separate userdb for each test.
 // (When doing so, remember to add some standalone tests of DeleteCert since it
 // won't be tested by TearDown anymore.)
 class CertDatabaseNSSTest : public testing::Test {
  public:
   virtual void SetUp() {

[M-A Ruel, 2010/12/10 14:52:18]

Isn't there already a base class that creates a temp dir for you? If not, it
would probably be a good idea but that's outside the scope of this test.

-    if (!temp_db_initialized_) {
-      ScopedTempDir* temp_db_dir = Singleton<
-          ScopedTempDir,
-          DefaultSingletonTraits<ScopedTempDir>,
-          CertDatabaseNSSTest>::get();
-      ASSERT_TRUE(temp_db_dir->CreateUniqueTempDir());
+    if (!temp_db_dir_.get()) {
+      temp_db_dir_.reset(new ScopedTempDir());
+      ASSERT_TRUE(temp_db_dir_->CreateUniqueTempDir());
       ASSERT_TRUE(
-          base::OpenTestNSSDB(temp_db_dir->path(), "CertDatabaseNSSTest db"));
-      temp_db_initialized_ = true;
+          base::OpenTestNSSDB(temp_db_dir_->path(), "CertDatabaseNSSTest db"));
     }
     slot_.reset(base::GetDefaultNSSKeySlot());
 
     // Test db should be empty at start of test.
     EXPECT_EQ(0U, ListCertsInSlot(slot_.get()).size());
   }
   virtual void TearDown() {
     // Don't try to cleanup if the setup failed.
-    ASSERT_TRUE(temp_db_initialized_);
+    ASSERT_TRUE(temp_db_dir_.get());
     ASSERT_TRUE(slot_.get());
 
     EXPECT_TRUE(CleanupSlotContents(slot_.get()));
     EXPECT_EQ(0U, ListCertsInSlot(slot_.get()).size());
   }
 
  protected:
   base::ScopedPK11Slot slot_;
   CertDatabase cert_db_;
 
  private:
-  static bool temp_db_initialized_;
+  static scoped_ptr<ScopedTempDir> temp_db_dir_;
 };
 
 // static
-bool CertDatabaseNSSTest::temp_db_initialized_ = false;
+scoped_ptr<ScopedTempDir> CertDatabaseNSSTest::temp_db_dir_;

[M-A Ruel, 2010/12/10 14:52:18]

And why do you think it's better to use a scoped_ptr here? Even if it's harmless
in a unit test, I'm afraid people would copy-paste it on real source code.

[Satish, 2010/12/10 15:04:57]

When getting rid of the singleton I considered either a LazyInstance or a
scopedptr, and scopedptr seemed to fit with the class. However it is different
from the singleton because the ScopedTempDir was earlier living across tests,
intentional or not. I can switch it to LazyInstance outside this class if you
think that is better.

[M-A Ruel, 2010/12/10 15:09:42]

I'm simply afraid of showing bad use cases. A global scoped_ptr is a bad
example.

[Satish, 2010/12/10 16:10:11]

I agree, though this is really just declaring the class's static variable it
could be just used as a global during cut/paste. But a global LazyInstance would
stay alive after these tests finish. I thought the //static comment was
sufficient to identify this is not a plain global. Do you have any suggestion on
what I should change this to?

[M-A Ruel, 2010/12/10 16:18:37]

A class member variable. Creating the directory on each unit test doesn't take
long and reduce inter-tests side-effects. I don't think there's any need for any
global state here.

[Satish, 2010/12/10 17:13:48]

Done.

 
 TEST_F(CertDatabaseNSSTest, ListCerts) {
   // This test isn't terribly useful, though it will at least let valgrind test
   // for leaks.
   CertificateList certs;
   cert_db_.ListCerts(&certs);
   // The test DB is empty, but let's assume there will always be something in
   // the other slots.
   EXPECT_LT(0U, certs.size());
 }
@@ skipping 342 matching lines @@
       puny_cert.get(), CA_CERT,
       CertDatabase::TRUSTED_SSL | CertDatabase::TRUSTED_EMAIL));
 
   verify_result.Reset();
   error = puny_cert->Verify("xn--wgv71a119e.com", flags, &verify_result);
   EXPECT_EQ(OK, error);
   EXPECT_EQ(0, verify_result.cert_status);
 }
 
 }  // namespace net