Initialize the simple cache backend at runtime.

net/disk_cache/disk_cache_test_base.cc

Index: net/disk_cache/disk_cache_test_base.cc
===================================================================
--- net/disk_cache/disk_cache_test_base.cc	(revision 188696)
+++ net/disk_cache/disk_cache_test_base.cc	(working copy)
@@ -10,8 +10,11 @@
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "net/disk_cache/backend_impl.h"
+#include "net/disk_cache/cache_util.h"
+#include "net/disk_cache/disk_cache.h"
 #include "net/disk_cache/disk_cache_test_util.h"
 #include "net/disk_cache/mem_backend_impl.h"
+#include "net/disk_cache/simple/simple_backend_impl.h"
 
 DiskCacheTest::DiskCacheTest() {
   CHECK(temp_dir_.CreateUniqueTempDir());
@@ -52,7 +55,7 @@
       size_(0),
       type_(net::DISK_CACHE),
       memory_only_(false),
-      implementation_(false),
+      simple_cache_mode_(false),
       force_creation_(false),
       new_eviction_(false),
       first_cleanup_(true),
@@ -63,10 +66,40 @@
 
 DiskCacheTestWithCache::~DiskCacheTestWithCache() {}
 
+disk_cache::Backend* DiskCacheTestWithCache::CreateBackendAsModeSuggests(

[rvargas (doing something else), 2013/03/25 22:06:04]

looks like the header is not part of the CL... without knowing if I'm missing
something, don't move the code (keep it where InitDiskCacheImpl was), to follow
changes easier.

[pasko-google - do not use, 2013/03/26 16:54:57]

argh, I forgot that I need to add files manually with svn-based checkout, sorry
about that. Uploading the better one.

Oh, and moving the code down in the file so that the difference is more visible.

+    uint32 flags, base::Thread* thread) {
+  base::MessageLoopProxy* runner;
+  if (use_current_thread_ || !thread)

[rvargas (doing something else), 2013/03/25 22:06:04]

!thread looks like a critical error that should not be masked

[pasko-google - do not use, 2013/03/26 16:54:57]

BackendShutdownWithPendingFileIO was using this codepath for reason unknown to
me, and you are right, it is better to as  use_current_thread_. Done.

+    runner = base::MessageLoopProxy::current();
+  else
+    runner = thread->message_loop_proxy();
+
+  if (simple_cache_mode_ && type_ == net::DISK_CACHE) {

[rvargas (doing something else), 2013/03/25 22:06:04]

The type should be under control of the specific test, not the framework.

[pasko-google - do not use, 2013/03/26 16:54:57]

Done.

+    net::TestCompletionCallback cb;
+    disk_cache::Backend* simple_backend;
+    int rv = disk_cache::SimpleBackendImpl::CreateBackend(cache_path_, size_,

[rvargas (doing something else), 2013/03/25 22:06:04]

Why a static factory method instead of following the same pattern of ctor +
init?

[pasko-google - do not use, 2013/03/26 16:54:57]

There is no real reason, I just avoided changing with this CL to be less in
conflict with that Felipe is doing. Added a TODO.

+        type_, disk_cache::kNone, make_scoped_refptr(runner), NULL,

[rvargas (doing something else), 2013/03/25 22:06:04]

I think we should assume for the time being that flags are not shared between
implementations. (doesn't have to be fixed by this CL)

[pasko-google - do not use, 2013/03/26 16:54:57]

Yes. I am seeing this as .. for testing reasons we would probably have 'struct
BackendCreationParams' that may contain both kind of flags if a test needs
analogous things to request from both backend variants. Let's wait for first
interesting parameters for the Simple Cache.

+        &simple_backend, cb.callback());
+    EXPECT_EQ(net::OK, cb.GetResult(rv));
+    return simple_backend;
+  }
+
+  if (mask_)
+    cache_impl_ = new disk_cache::BackendImpl(cache_path_, mask_, runner, NULL);
+  else
+    cache_impl_ = new disk_cache::BackendImpl(cache_path_, runner, NULL);
+  cache_impl_->SetFlags(flags);
+  cache_impl_->SetMaxSize(size_);
+  cache_impl_->SetType(type_);
+  if (new_eviction_)
+    cache_impl_->SetNewEviction();
+  net::TestCompletionCallback cb;
+  int rv = cache_impl_->Init(cb.callback());
+  EXPECT_EQ(net::OK, cb.GetResult(rv));
+  return cache_impl_;
+}
+
 void DiskCacheTestWithCache::InitCache() {
-  if (mask_ || new_eviction_)
-    implementation_ = true;
-
   if (memory_only_)
     InitMemoryCache();
   else
@@ -79,7 +112,7 @@
 
 // We are expected to leak memory when simulating crashes.
 void DiskCacheTestWithCache::SimulateCrash() {
-  ASSERT_TRUE(implementation_ && !memory_only_);
+  ASSERT_TRUE(!memory_only_);
   net::TestCompletionCallback cb;
   int rv = cache_impl_->FlushQueueForTest(cb.callback());
   ASSERT_EQ(net::OK, cb.GetResult(rv));
@@ -88,11 +121,11 @@
   delete cache_impl_;
   EXPECT_TRUE(CheckCacheIntegrity(cache_path_, new_eviction_, mask_));
 
-  InitDiskCacheImpl();
+  cache_ = CreateBackendAsModeSuggests(disk_cache::kNoRandom, &cache_thread_);

[rvargas (doing something else), 2013/03/25 22:06:04]

Why exposing the flags if all callers pass the same value?

[pasko-google - do not use, 2013/03/26 16:54:57]

In BackendShutdownWithPending* we sometimes add kNoBuffering to flags, the
simple cache ignores that. I did not create yet another layer of flags just
because there was no immediate use of it for the simple cache.

 }
 
 void DiskCacheTestWithCache::SetTestMode() {
-  ASSERT_TRUE(implementation_ && !memory_only_);
+  ASSERT_TRUE(!memory_only_);
   cache_impl_->SetUnitTestMode();
 }
 
@@ -235,11 +268,6 @@
 }
 
 void DiskCacheTestWithCache::InitMemoryCache() {
-  if (!implementation_) {
-    cache_ = disk_cache::MemBackendImpl::CreateBackend(size_, NULL);
-    return;
-  }
-
   mem_cache_ = new disk_cache::MemBackendImpl(NULL);
   cache_ = mem_cache_;
   ASSERT_TRUE(NULL != cache_);
@@ -252,49 +280,32 @@
 
 void DiskCacheTestWithCache::InitDiskCache() {
   if (first_cleanup_)
-    ASSERT_TRUE(CleanupCacheDir());
+    EXPECT_TRUE(CleanupCacheDir());

[rvargas (doing something else), 2013/03/25 22:06:04]

Why change this to an EXPECT? Seems better to skip the rest of the test at this
point.

[pasko-google - do not use, 2013/03/26 16:54:57]

I recently discovered that ASSERT_TRUE does only return from the current
function, and reports an error, but it does not wind down the whole test. Also
it fails to compile if invoked from non-void helper functions because it does a
'return something()' where 'something()' returns void.

I can leave ASSERT_TRUE here if it looks better, since this function returns
void, it would work, though it would be roughly equivalent to EXPECT_TRUE. 

So for consistency I prefer EXPECT_* in helper functions if we want to check
rare/unlikely failures in helper functions intended only for testing. Cleaner
way would be to return success/failure from the helper functions, but it is a
little too heavy for this CL already.

[rvargas (doing something else), 2013/03/26 18:30:00]

Yes, asserting inside this method doesn't prevent the rest of the test from
running... and this method returns void to be able to assert. I believe that
logically we want an assert, even if today it doesn't work that well. We can
leave the assert and fix the rest later, or leave the assert and look for the
error at return, or convert the method to return a failure and assert on return
(slightly worse because we miss the exact line that failed).

[pasko-google - do not use, 2013/03/27 20:36:08]

I won't be stubborn, reverting back to using ASSERT, even making some previous
EXPECTs to be ASSERTs for consistency. Though I think ASSERT will not in any
close future prevent the whole test from running. It's just complicated. EXPECT
is a good way to be more clear about what the code is doing. But since not many
people look at this specific portion of code, quite OK to have it the way it is
now.

Done.

 
   if (!cache_thread_.IsRunning()) {
     EXPECT_TRUE(cache_thread_.StartWithOptions(
                     base::Thread::Options(MessageLoop::TYPE_IO, 0)));
   }
-  ASSERT_TRUE(cache_thread_.message_loop() != NULL);
+  EXPECT_TRUE(cache_thread_.message_loop() != NULL);

[rvargas (doing something else), 2013/03/25 22:06:04]

Definitely an assert

[pasko-google - do not use, 2013/03/26 16:54:57]

Same as above, if you want me to revert, I am OK with that, just bringing
attention to irrelevant detail :)

 
-  if (implementation_)
-    return InitDiskCacheImpl();
-
-  scoped_refptr<base::MessageLoopProxy> thread =
-      use_current_thread_ ? base::MessageLoopProxy::current() :
-                            cache_thread_.message_loop_proxy();
-
-  net::TestCompletionCallback cb;
-  int rv = disk_cache::BackendImpl::CreateBackend(
-               cache_path_, force_creation_, size_, type_,
-               disk_cache::kNoRandom, thread, NULL, &cache_, cb.callback());
-  ASSERT_EQ(net::OK, cb.GetResult(rv));
+  cache_ = CreateBackendAsModeSuggests(disk_cache::kNoRandom, &cache_thread_);
 }
 
-void DiskCacheTestWithCache::InitDiskCacheImpl() {
-  scoped_refptr<base::MessageLoopProxy> thread =
-      use_current_thread_ ? base::MessageLoopProxy::current() :
-                            cache_thread_.message_loop_proxy();
-  if (mask_)
-    cache_impl_ = new disk_cache::BackendImpl(cache_path_, mask_, thread, NULL);
-  else
-    cache_impl_ = new disk_cache::BackendImpl(cache_path_, thread, NULL);
+// Testing backend creation retry logic is hard because the CacheCreator and
+// cache backend(s) are tightly coupled. So we take the default backend often.
+// Tests themselves need to be adjusted for platforms where the BackendImpl is
+// not the default backend.
+void DiskCacheTestWithCache::InitDefaultCacheViaCreator() {
+  if (!cache_thread_.IsRunning()) {
+    EXPECT_TRUE(cache_thread_.StartWithOptions(
+                    base::Thread::Options(MessageLoop::TYPE_IO, 0)));
+  }
+  EXPECT_TRUE(cache_thread_.message_loop() != NULL);
 
-  cache_ = cache_impl_;

[rvargas (doing something else), 2013/03/25 22:06:04]

missing this line

[pasko-google - do not use, 2013/03/26 16:54:57]

Good catch! Done.

-  ASSERT_TRUE(NULL != cache_);
-
-  if (size_)
-    EXPECT_TRUE(cache_impl_->SetMaxSize(size_));
-
-  if (new_eviction_)
-    cache_impl_->SetNewEviction();
-
-  cache_impl_->SetType(type_);
-  cache_impl_->SetFlags(disk_cache::kNoRandom);
   net::TestCompletionCallback cb;
-  int rv = cache_impl_->Init(cb.callback());
-  ASSERT_EQ(net::OK, cb.GetResult(rv));
+  disk_cache::CacheCreator* creator = new disk_cache::CacheCreator(cache_path_,

[rvargas (doing something else), 2013/03/25 22:06:04]

Call CreateCacheBackend() instead.

[pasko-google - do not use, 2013/03/26 16:54:57]

My main motivation was in the need of disk_cache::kNoRandom. Is it still needed
by the tests (to be non-flaky?) or the flag is obsolete?

+      true, 0, net::DISK_CACHE, disk_cache::kNoRandom,
+      cache_thread_.message_loop_proxy(), NULL, &cache_, cb.callback());
+  int rv = creator->Run();
+  EXPECT_EQ(net::OK, cb.GetResult(rv));
 }