Refactored cookies persistent store clean-up on shutdown.

chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc

Index: chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc b/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d3f112ad06f918c2e30800f494696acdf6e1e6be
--- /dev/null
+++ b/chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc
@@ -0,0 +1,125 @@
+// 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 "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/ref_counted.h"
+#include "base/scoped_temp_dir.h"
+#include "base/stl_util-inl.h"
+#include "base/time.h"
+#include "googleurl/src/gurl.h"

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

alphabetical ordering

[pastarmovj, 2010/12/02 16:29:07]

You got me on that one :). Done!

+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/net/sqlite_persistent_cookie_store.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/test/thread_test_helper.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class SQLitePersistentCookieStoreTest : public testing::Test {
+ public:
+  SQLitePersistentCookieStoreTest()
+      : message_loop_(MessageLoop::TYPE_UI),
+        ui_thread_(BrowserThread::UI, &message_loop_),
+        db_thread_(BrowserThread::DB) {}

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

why no message loop for the db thread?

[pastarmovj, 2010/12/02 16:29:07]

I start the thread myself in SetUp. A message loop is being created then and we
don't need to touch it anyhow. We need separate threads for DB and !DB threads
because the code has both checks "are we on DB thread" and "are we NOT on DB
thread".

+
+ protected:
+  virtual void SetUp() {
+    db_thread_.Start();

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

that shouldn't be required

[pastarmovj, 2010/12/02 16:29:07]

See previous comment.

+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    ASSERT_TRUE(store_ = new SQLitePersistentCookieStore(
+        temp_dir_.path().Append(chrome::kCookieFilename)));
+    ASSERT_TRUE(store_->Load(&cookies_));

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

in a new directory, there cannot be any cookies

[pastarmovj, 2010/12/02 16:29:07]

The method returns true whenever the database has been successfully loaded or
created if not existing yet.

+    ASSERT_TRUE(0 == cookies_.size());
+  }
+
+  virtual void TearDown() {
+    // Enforce deleting the store file upon destruction.
+    store_->SetClearLocalStateOnExit(true);

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

please don't add additional tests in tear down

[pastarmovj, 2010/12/02 16:29:07]

Done. Moved it to a separate test.

+    store_ = NULL;
+    // Make sure we wait until the destructor has run.
+    scoped_refptr<ThreadTestHelper> helper(
+        new ThreadTestHelper(BrowserThread::DB));
+    ASSERT_TRUE(helper->Run());
+
+    ASSERT_FALSE(file_util::PathExists(
+        temp_dir_.path().Append(chrome::kCookieFilename)));
+  }
+
+  void ClearCookies() {
+    STLDeleteContainerPointers(cookies_.begin(), cookies_.end());
+    cookies_.clear();
+  }
+
+  MessageLoop message_loop_;
+
+  BrowserThread ui_thread_;
+  BrowserThread db_thread_;
+  scoped_refptr<SQLitePersistentCookieStore> store_;

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

i think you should move the store into the single tests. then you also don't
have to replace it when you really want to delete it

[pastarmovj, 2010/12/02 16:29:07]

Well still share this between all 2 (now 3) tests so I find it nicer to keep
common parts in base class. Makes it more tidy I think...

+  ScopedTempDir temp_dir_;
+  std::vector<net::CookieMonster::CanonicalCookie*> cookies_;

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

this is only used in one test, so I'd move this variable to that test (same for
ClearCookies)

[pastarmovj, 2010/12/02 16:29:07]

Actually we need it in the initialization call of every class instance no matter
if we do something with it or not.

+};
+
+TEST_F(SQLitePersistentCookieStoreTest, KeepOnDestruction) {
+  // Make sure the store gets written at least once.
+  store_->AddCookie(
+      net::CookieMonster::CanonicalCookie("A", "B", "http://foo.bar", "/",
+                                          false, false,
+                                          base::Time(), base::Time(),
+                                          true, base::Time()));
+  store_->SetClearLocalStateOnExit(false);
+  // Replace the store effectively destroying the current one and forcing it
+  // to call it's destructor. We can't just NULL it so that our TearDown code
+  // won't crash.
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+  // Make sure we wait until the destructor has run.
+  scoped_refptr<ThreadTestHelper> helper(
+      new ThreadTestHelper(BrowserThread::DB));
+  ASSERT_TRUE(helper->Run());
+
+  ASSERT_TRUE(file_util::PathExists(
+      temp_dir_.path().Append(chrome::kCookieFilename)));
+}
+
+// Test if data is stored as expected in the SQLite database.
+TEST_F(SQLitePersistentCookieStoreTest, TestPersistance) {
+  // Make sure the store gets written at least once.
+  store_->AddCookie(
+      net::CookieMonster::CanonicalCookie("A", "B", "http://foo.bar", "/",
+                                          false, false,
+                                          base::Time::Now(), base::Time::Now(),
+                                          true, base::Time::Now()));
+  // Replace the store effectively destroying the current one and forcing it
+  // to write it's data to disk. Then we can see if after loading it again it
+  // is still there.
+  store_ = NULL;
+  scoped_refptr<ThreadTestHelper> helper(
+      new ThreadTestHelper(BrowserThread::DB));
+  // Make sure we wait until the destructor has run.
+  ASSERT_TRUE(helper->Run());
+  ClearCookies();
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+
+  // Reload and test for persistence
+  ASSERT_TRUE(store_->Load(&cookies_));
+  ASSERT_TRUE(1 == cookies_.size());

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

ASSERT_EQ

[pastarmovj, 2010/12/02 16:29:07]

Done.

+  ASSERT_EQ("http://foo.bar", cookies_[0]->Domain());
+  ASSERT_EQ("A", cookies_[0]->Name());

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

ASSERT_STREQ

[pastarmovj, 2010/12/02 16:29:07]

Done.

+  ASSERT_EQ("B", cookies_[0]->Value());
+
+  // Now delete the cookie and check persistence again.
+  store_->DeleteCookie(*cookies_[0]);
+  store_ = NULL;
+  // Make sure we wait until the destructor has run.
+  ASSERT_TRUE(helper->Run());
+  ClearCookies();
+  store_ = new SQLitePersistentCookieStore(
+      temp_dir_.path().Append(chrome::kCookieFilename));
+
+  // Reload and check if the cookie has been removed.
+  ASSERT_TRUE(store_->Load(&cookies_));
+  ASSERT_TRUE(0 == cookies_.size());

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

ASSERT_EQ

[pastarmovj, 2010/12/02 16:29:07]

Done.

+}
+
+

[jochen (gone - plz use gerrit), 2010/12/02 15:09:51]

no empty lines please

[pastarmovj, 2010/12/02 16:29:07]

Done.