Adds a new class HttpCache::Writers for multiple cache transactions reading from the network.

net/http/http_cache_writers_unittest.cc

Index: net/http/http_cache_writers_unittest.cc
diff --git a/net/http/http_cache_writers_unittest.cc b/net/http/http_cache_writers_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f28749b0e594bc9326c17f841e403c9c22ee40be
--- /dev/null
+++ b/net/http/http_cache_writers_unittest.cc
@@ -0,0 +1,217 @@
+// Copyright (c) 2017 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 "net/http/http_cache_writers.h"
+
+#include "base/run_loop.h"
+#include "net/http/http_cache.h"
+#include "net/http/http_cache_transaction.h"
+#include "net/http/http_transaction.h"
+#include "net/http/http_transaction_test_util.h"
+#include "net/http/mock_http_cache.h"
+#include "net/test/gtest_util.h"
+
+using net::test::IsError;
+using net::test::IsOk;
+
+namespace net {
+
+class HttpCache::WritersTest {

[jkarlin, 2017/06/12 18:30:33]

Since every test uses this class, it seems like this ought to be a test fixture
class.

[Randy Smith (Not in Mondays), 2017/06/13 17:45:05]

+1

[shivanisha, 2017/06/14 02:33:17]

This class is mostly a collection of helper functions and does not follow any
Setup and TearDown . Each test requires a different collection of helper
functions from this class.

[Randy Smith (Not in Mondays), 2017/06/14 19:49:17]

Sure.  But if you put them in a text fixture with a null setup and teardown (I
don't think you need to declare the functions if you aren't using them) they're
accessible to all tests in that fixture without your having to declare them in
each test.  I think of this as what a text fixture is for (collecting a set of
helper functions in one place that'll be easily available to a set of tests).

[shivanisha, 2017/06/15 20:17:45]

Since the class needs to access a private nested class of HttpCache which is
HttpCache::Writers, it needs to itself be a nested class of HttpCache? And then
if I create as a fixture class the test with macro TEST_F does not take
HttpCache::WritersTest or WritersTest. Am I missing something here?

[Randy Smith (Not in Mondays), 2017/06/22 21:45:05]

Fair, though there are several ways around that.  You could certainly make it a
non-private class for testing, though that's got similar problems as to this
approach.  I would also think that creating a test fixture and making that a
friend of both HttpCache and HttpCache::Writers would work, and would be in line
with other existing test structuring. 
I wouldn't expect it to anything with a :: in it--it would need to be (in the
test file) a top level class.

(I'm not dead set on the structures I'm pushing, but I do think that this is
unusual, and the structures I'm pushing more in line with existing Chromium
practice.)

[shivanisha, 2017/06/26 20:47:02]

I will address this tomorrow but didn't want to delay uploading the other
changes on this.

[shivanisha, 2017/06/27 04:42:19]

It works on making the test class friends to both HttpCache and
HttpCache::Writers. Thanks!
Making it a friend class for only HttpCache did not work earlier.

+ public:
+  WritersTest() {}
+
+  // Since ActiveEntry and Transaction are private classes of HttpCache,
+  // accessing those only in this class instead of in tests.
+
+  void CreateWriters(const std::string& url) {
+    HttpCache* cache = cache_.http_cache();
+    ActiveEntry* entry = cache->FindActiveEntry(url);
+    writers_ = base::MakeUnique<Writers>(cache, entry);
+  }
+
+  void CreateNetworkTransaction(std::unique_ptr<HttpTransaction>* transaction) {
+    MockNetworkLayer* network_layer = cache_.network_layer();
+    network_layer->CreateTransaction(DEFAULT_PRIORITY, transaction);
+  }
+
+  void CreateAddTransactionToWriters(MockTransaction transaction_info,
+                                     bool is_exclusive = false) {
+    MockHttpRequest request(transaction_info);
+    std::unique_ptr<HttpTransaction> transaction;
+    int rv = cache_.CreateTransaction(&transaction);
+    DCHECK_EQ(rv, OK);
+    DCHECK(transaction.get());
+
+    // Create an ActiveEntry.
+    TestCompletionCallback callback;
+    rv = transaction->Start(&request, callback.callback(), NetLogWithSource());
+    base::RunLoop().RunUntilIdle();
+
+    CreateWriters(kSimpleGET_Transaction.url);
+
+    // Create and Start a mock network transaction.
+    std::unique_ptr<HttpTransaction> network_transaction;
+    CreateNetworkTransaction(&network_transaction);
+    network_transaction->Start(&request, callback.callback(),
+                               NetLogWithSource());
+    base::RunLoop().RunUntilIdle();
+
+    DCHECK(writers_->IsEmpty());
+    writers_->AddTransaction((Transaction*)(transaction.get()),
+                             std::move(network_transaction), is_exclusive);
+    transactions_.insert(std::move(transaction));
+  }
+
+  void AddTransactionToExistingWriters() {
+    std::unique_ptr<HttpTransaction> transaction;
+    int rv = cache_.CreateTransaction(&transaction);
+    DCHECK_EQ(rv, OK);
+    DCHECK(transaction.get());
+
+    DCHECK(writers_);
+    writers_->AddTransaction((Transaction*)(transaction.get()), nullptr, false);
+    transactions_.insert(std::move(transaction));
+  }
+
+  int Read(std::string* result) {
+    Transaction* transaction = (Transaction*)(transactions_.begin()->get());
+    TestCompletionCallback callback;
+
+    std::string content;
+    int rv;
+    do {
+      scoped_refptr<IOBuffer> buf(new IOBuffer(256));
+      rv = writers_->Read(buf.get(), 256, callback.callback(), transaction);
+      if (rv == ERR_IO_PENDING) {
+        rv = callback.WaitForResult();
+        base::RunLoop().RunUntilIdle();
+      }
+
+      if (rv > 0)
+        content.append(buf->data(), rv);
+      else if (rv < 0)
+        return rv;
+    } while (rv > 0);
+
+    result->swap(content);
+    return OK;
+  }
+
+  bool StopCaching() {
+    Transaction* transaction = (Transaction*)(transactions_.begin()->get());
+    return writers_->StopCaching(transaction);
+  }
+
+  void RemoveTransaction() {
+    Transaction* transaction = (Transaction*)(transactions_.begin()->get());
+    writers_->RemoveTransaction(transaction);
+  }
+
+  MockHttpCache cache_;
+  std::unique_ptr<Writers> writers_;
+  std::unordered_set<std::unique_ptr<HttpTransaction>> transactions_;
+};
+
+// Tests successful addition of a transaction.
+TEST(HttpCacheWriters, AddTransaction) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+}
+
+// Tests successful addition of multiple transaction.
+TEST(HttpCacheWriters, AddManyTransactions) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  for (int i = 0; i < 5; i++)
+    writersTest.AddTransactionToExistingWriters();
+
+  EXPECT_EQ(writersTest.writers_->CountTransactionsForTesting(), 6);
+}
+
+// Tests that CanAddWriters should return false if it is exclusive writing.
+TEST(HttpCacheWriters, AddTransactionsExclusive) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction, true);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  EXPECT_FALSE(writersTest.writers_->CanAddWriters());
+}
+
+// Tests StopCaching should not stop caching if there are multiple writers.
+TEST(HttpCacheWriters, StopCachingMultipleWriters) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  EXPECT_TRUE(writersTest.writers_->CanAddWriters());
+  writersTest.AddTransactionToExistingWriters();
+
+  EXPECT_FALSE(writersTest.StopCaching());
+  EXPECT_TRUE(writersTest.writers_->CanAddWriters());
+}
+
+// Tests StopCaching should stop caching if there is a single writer.
+TEST(HttpCacheWriters, StopCaching) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  EXPECT_TRUE(writersTest.StopCaching());
+  EXPECT_FALSE(writersTest.writers_->CanAddWriters());
+}
+
+// Tests removing of an idle transaction.
+TEST(HttpCacheWriters, RemoveIdleTransaction) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  writersTest.RemoveTransaction();
+  EXPECT_TRUE(writersTest.writers_->IsEmpty());
+}
+
+// Tests that Read is successful.
+TEST(HttpCacheWriters, Read) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  std::string content;
+  int rv = writersTest.Read(&content);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+  EXPECT_EQ(expected, content);
+}
+
+// Tests truncating the entry via Writers.
+TEST(HttpCacheWriters, TruncateEntry) {
+  HttpCache::WritersTest writersTest;
+
+  writersTest.CreateAddTransactionToWriters(kSimpleGET_Transaction);
+  EXPECT_FALSE(writersTest.writers_->IsEmpty());
+
+  std::string content;
+  int rv = writersTest.Read(&content);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+  EXPECT_EQ(expected, content);
+
+  writersTest.writers_->TruncateEntry();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(writersTest.writers_->IsTruncatedForTesting());
+}

[jkarlin, 2017/06/12 18:30:33]

I'd like to see tests for all of the public API in this file, with lots of tests
for behavior of multiple writers with writers/readers coming and going.

[shivanisha, 2017/06/14 02:33:17]

Added tests for all public APIs, multiple Reads , deletion of
active/waiting/idle transactions while reading, cache write failure and network
read failure cases. 

+
+}  // namespace net