Implement QuotaTemporaryStorageEvictor.

webkit/quota/quota_temporary_storage_evictor_unittest.cc

+// Copyright (c) 2011 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 "testing/gtest/include/gtest/gtest.h"
+
+#include <list>
+#include <map>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/task.h"
+#include "webkit/quota/mock_storage_client.h"
+#include "webkit/quota/quota_temporary_storage_evictor.h"
+
+namespace quota {
+
+class QuotaTemporaryStorageEvictorTest;
+
+namespace {
+
+int64 a_size = 400;
+int64 b_size = 150;
+int64 c_size = 120;
+int64 d_size = 292;
+int64 initial_total_size = a_size + b_size + c_size + d_size;
+int64 e_size = 275;

[michaeln, 2011/05/23 19:53:47]

These look like they should be const values?
  const int64 kSizeA

[Dai Mikurube (NOT FULLTIME), 2011/05/24 05:59:04]

Done in the test case function.

+
+class MockQuotaEvictionHandler : public quota::QuotaEvictionHandler {
+ public:
+  MockQuotaEvictionHandler(QuotaTemporaryStorageEvictorTest *test)
+      : quota_(0),
+        available_space_(0),
+        test_(test) {}
+  virtual void EvictOriginData(
+      const GURL& origin,
+      StorageType type,
+      EvictOriginDataCallback* callback) OVERRIDE {
+    int64 origin_usage = EnsureOriginRemoved(origin);
+    if (origin_usage >= 0)
+      available_space_ += origin_usage;
+    callback->Run(quota::kQuotaStatusOk);
+    delete callback;
+  }
+
+  virtual void GetUsageAndQuotaForEviction(
+      GetUsageAndQuotaForEvictionCallback* callback) OVERRIDE {
+    if (task_for_get_usage_and_quota_.get())
+      task_for_get_usage_and_quota_->Run();

[michaeln, 2011/05/23 19:53:47]

The indirection using a CancelableTask seems overly complicated. Maybe put that
logic directly into your MockHandler instead.

[Dai Mikurube (NOT FULLTIME), 2011/05/24 05:59:04]

As discussing with Kinuko-san,
* for RepeatedEvictionTest, we have to intercept the eviction loop
* for the other test cases, we don't want to intercept.

Prepare another Mock...Handler is an option, but it looks complicated, too.  I
chose the current design since this looks a little simpler than another
Mock...Handler.

+    callback->Run(quota::kQuotaStatusOk, GetUsage(), quota_, available_space_);
+    delete callback;
+  }
+
+  virtual void GetLRUOrigin(
+      StorageType type,
+      GetLRUOriginCallback* callback) OVERRIDE {
+    if (origin_order_.empty())
+      callback->Run(GURL());
+    else
+      callback->Run(origin_order_.front());
+    delete callback;
+  }
+
+  int64 GetUsage() const {
+    int64 total_usage = 0;
+    for (std::map<GURL, int64>::const_iterator p = origins_.begin();
+         p != origins_.end();
+         ++p)
+      total_usage += p->second;
+    return total_usage;
+  }
+
+  void set_quota(int64 quota) {
+    quota_ = quota;
+  }
+  void set_available_space(int64 available_space) {
+    available_space_ = available_space;
+  }
+  void set_task_for_get_usage_and_quota(CancelableTask* task) {
+    task_for_get_usage_and_quota_.reset(task);
+  }
+
+  // Simulates an access to |origin|.  It reorders the internal LRU list.
+  // It internally uses AddOrigin().
+  void AccessOrigin(const GURL& origin) {
+    std::map<GURL, int64>::iterator found = origins_.find(origin);
+    if (found != origins_.end())

[kinuko, 2011/05/23 12:10:30]

EXPECT_TRUE?

[Dai Mikurube (NOT FULLTIME), 2011/05/24 04:54:33]

Done.

+      AddOrigin(origin, found->second);
+  }
+
+  // Simulates adding or overwriting the |origin| to the internal origin set
+  // with the |usage|.  It also adds or moves the |origin| to the end of the
+  // LRU list.
+  void AddOrigin(const GURL& origin, int64 usage) {
+    EnsureOriginRemoved(origin);
+    origin_order_.push_back(origin);
+    origins_[origin] = usage;
+  }
+
+ private:
+  int64 EnsureOriginRemoved(const GURL& origin) {
+    int64 origin_usage;
+    if (origins_.find(origin) == origins_.end())
+      return -1;
+    else
+      origin_usage = origins_[origin];
+
+    origins_.erase(origin);
+    origin_order_.remove(origin);
+    return origin_usage;
+  }
+
+  int64 quota_;
+  int64 available_space_;
+  std::list<GURL> origin_order_;
+  std::map<GURL, int64> origins_;
+
+  QuotaTemporaryStorageEvictorTest *test_;
+  scoped_ptr<CancelableTask> task_for_get_usage_and_quota_;
+};
+
+}  // anonymous namespace
+
+class QuotaTemporaryStorageEvictorTest : public testing::Test {
+ public:
+  QuotaTemporaryStorageEvictorTest()
+      : num_get_usage_and_quota_for_eviction_(0),
+        runnable_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
+
+  void SetUp() {
+    quota_eviction_handler_.reset(new MockQuotaEvictionHandler(this));
+
+    // Run repeatedly in the single RunAllPending() when interval_ms == 0
+    temporary_storage_evictor_.reset(new QuotaTemporaryStorageEvictor(
+        quota_eviction_handler_.get(),
+        0, base::MessageLoopProxy::CreateForCurrentThread()));
+  }
+
+  void TearDown() {
+    temporary_storage_evictor_.reset();
+    quota_eviction_handler_.reset();
+    MessageLoop::current()->RunAllPending();
+  }
+
+  void TaskForRepeatTestAfterGetUsageAndQuota() {

[kinuko, 2011/05/23 12:10:30]

Is this method meant to be a part of RepeatedEvictionTest, a part of verifier of
the test?

Why are we using Task/CancelableTask for this?  If we use task can't we also
pass the expected values when we create a new task in RepeatedEvictionTest
rather than using global static vars?
(With task we can bind variables that are passed as parameters when it runs the
method)

Or we might be able to do something like this:

class MockQuotaEvictionHelper {
 public:
  virtual void GetUsageAndQuotaForEviction(
      GetUsageAndQuotaForEvictionCallback* callback) OVERRIDE {
    intermediate_usage_results_.push_back(GetUsage());
    if (++current_repeat_count_ >= max_repeat_count_)
      set_repeated_eviction(false);
    // ...
  }

  void set_max_repeat_count(int max) { max_repeat_count_ = max; }

  // Later we could verify the results using this accessor.
  const std::vector<int64>& intermediate_usage_results() const {
    return intermediate_usage_results_;
  }

 private:
  int current_repeat_count_;
  int max_repeat_count_;
  std::vector<int64> intermediate_usage_results_;
  // ...
};

Or maybe we could have a separate EvictionHandler class (like
MockQuotaEvictionHandlerForRepeatedTest) only for this test case, make it derive
from MockQuotaEvictionHelper and override the GetUsageAndQuotaForEviction
implementation to do something you did in
TaskForRepeatTestAfterGetUsageAndQuota.
 
Or... whatever.  anyways I'd like expected values for a test case to be defined
in the test case code.  Then we could easily add similar tests later using the
same code.  Can we somehow do that?

[Dai Mikurube (NOT FULLTIME), 2011/05/24 04:54:33]

Thank you for listing up our many options.  They helped me to compare pros/cons
of our choices so much!

In my understanding, you say :
* The bad point : the expected values are separated from the test case.
* To be satisfied : the expected values should be declared at the test case,
even if they are given to the other functions and tasks later.

Are these correct?


On 2011/05/23 12:10:30, kinuko wrote:
Finally, I chose this option because it can run some additional things, such as
AddOrigin(GURL("http://www.e.com"), ...), per test case.  It is required to test
repeated eviction.
I didn't choose this because it cannot run additional tasks per test case, as
described above.
It looked like a reasonable choice, but I didn't choose this since :
* it looked a little overkilling, and
* there are not so large difference from the first options, from the viewpoint
of test-case separation.

It requires some EXPECT_EQ (or logging values), AddOrigin() and
set_repeated_eviction() in
MockQuotaEvictionHandlerForRepeatedTest::GetUsageAndQuotaForEviction() which is
separated from the test case "RepeatedEvictionTest".  The difference from the
second option is small in comparison to it's overkilling.


Many options and examples help us very much to discuss the details.  Thank you.
I guess just "what is the bad point" and "what should be satisfied at least" are
sometimes enough and helpful to understand comments than "how about that?" or
"can we?".

+    switch (num_get_usage_and_quota_for_eviction_) {
+    case 1:
+      EXPECT_EQ(initial_total_size - d_size,
+                quota_eviction_handler()->GetUsage());
+      quota_eviction_handler()->AddOrigin(
+          GURL("http://www.e.com"), e_size);
+      quota_eviction_handler()->AccessOrigin(GURL("http://www.c.com"));

[kinuko, 2011/05/23 12:10:30]

Shouldn't we have a separate test case for the cases like this (i.e. calling
AddOrigin() / AccessOrigin() between two eviction rounds etc)?  I mean, could we
have a test that runs one eviction round without setting set_repeated_eviction
true, adds and/or accesses origin, verifies something and then calls another
round of eviction?

If we mix various cases I'm afraid the test code gets complicated and hard to
parameterize.

[Dai Mikurube (NOT FULLTIME), 2011/05/24 04:54:33]

What do you mean by "a test case" and "a test" ?

SimpleEvictionTest does "a test that runs one eviction round without setting
set_repeated_eviction true".  RepeatedEvictionTest does "accesses origin,
verifies something and then calls another round of eviction".

What is the bad point you mean?  BTW, I didn't choose the third option in the
above comment as described there.

+      EXPECT_EQ(initial_total_size - d_size + e_size,
+                quota_eviction_handler()->GetUsage());
+      break;
+    case 2:
+      EXPECT_EQ(initial_total_size - d_size + e_size - b_size,
+                quota_eviction_handler()->GetUsage());
+      temporary_storage_evictor()->set_repeated_eviction(false);
+      break;
+    }
+    ++num_get_usage_and_quota_for_eviction_;
+  }
+
+ protected:
+  MockQuotaEvictionHandler* quota_eviction_handler() const {
+    return static_cast<MockQuotaEvictionHandler*>(
+        quota_eviction_handler_.get());
+  }
+
+  QuotaTemporaryStorageEvictor* temporary_storage_evictor() const {
+    return temporary_storage_evictor_.get();
+  }
+
+  int num_get_usage_and_quota_for_eviction() const {
+    return num_get_usage_and_quota_for_eviction_;
+  }
+
+  scoped_ptr<QuotaEvictionHandler> quota_eviction_handler_;
+  scoped_ptr<QuotaTemporaryStorageEvictor> temporary_storage_evictor_;
+
+  int num_get_usage_and_quota_for_eviction_;
+
+  ScopedRunnableMethodFactory<QuotaTemporaryStorageEvictorTest>
+      runnable_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuotaTemporaryStorageEvictorTest);
+};
+
+TEST_F(QuotaTemporaryStorageEvictorTest, SimpleEvictionTest) {
+  quota_eviction_handler()->AddOrigin(GURL("http://www.z.com"), 3000);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.y.com"), 200);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.x.com"), 500);
+  quota_eviction_handler()->set_quota(4000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  EXPECT_EQ(3700, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(700, quota_eviction_handler()->GetUsage());
+}
+
+TEST_F(QuotaTemporaryStorageEvictorTest, MultipleEvictionTest) {
+  quota_eviction_handler()->AddOrigin(GURL("http://www.z.com"), 20);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.y.com"), 2900);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.x.com"), 450);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.w.com"), 400);
+  quota_eviction_handler()->set_quota(4000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  EXPECT_EQ(3770, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(850, quota_eviction_handler()->GetUsage());
+}
+
+TEST_F(QuotaTemporaryStorageEvictorTest, RepeatedEvictionTest) {
+  quota_eviction_handler()->AddOrigin(GURL("http://www.d.com"), d_size);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.c.com"), c_size);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.b.com"), b_size);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.a.com"), a_size);
+  quota_eviction_handler()->set_quota(1000);
+  quota_eviction_handler()->set_available_space(1000000000);
+  quota_eviction_handler()->set_task_for_get_usage_and_quota(
+      runnable_factory_.NewRunnableMethod(&QuotaTemporaryStorageEvictorTest::
+          TaskForRepeatTestAfterGetUsageAndQuota));
+  EXPECT_EQ(initial_total_size, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->set_repeated_eviction(true);
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(initial_total_size - d_size + e_size - b_size - a_size,
+            quota_eviction_handler()->GetUsage());
+  EXPECT_EQ(4, num_get_usage_and_quota_for_eviction());
+}
+
+TEST_F(QuotaTemporaryStorageEvictorTest, DiskSpaceEvictionTest) {
+  quota_eviction_handler()->AddOrigin(GURL("http://www.z.com"), 294);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.y.com"), 120);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.x.com"), 150);
+  quota_eviction_handler()->AddOrigin(GURL("http://www.w.com"), 300);
+  quota_eviction_handler()->set_quota(10000);
+  quota_eviction_handler()->set_available_space(QuotaTemporaryStorageEvictor::
+      kDefaultMinAvailableDiskSpaceToStartEviction - 350);
+  EXPECT_EQ(864, quota_eviction_handler()->GetUsage());
+  temporary_storage_evictor()->Start();
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(450, quota_eviction_handler()->GetUsage());
+}
+
+}  // namespace quota