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..390cca15e2e967d8547c641f575c1ed26bb92f38
--- /dev/null
+++ b/net/http/http_cache_writers_unittest.cc
@@ -0,0 +1,619 @@
+// 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 <memory>
+#include <string>
+#include <vector>
+
+#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 WritersTest : public testing::Test {
+ public:
+  WritersTest() : request_(kSimpleGET_Transaction) {}
+
+  ~WritersTest() override { cache_.disk_cache()->ReleaseAll(); }

[jkarlin, 2017/06/28 15:50:10]

This call to ReleaseAll isn't necessary, the mock disk cache calls that in its
destructor.

[shivanisha, 2017/06/28 19:38:21]

Yes, removed it

+
+  // Since ActiveEntry and Transaction are private classes of HttpCache,
+  // accessing those only in this class instead of in tests.

[jkarlin, 2017/06/28 15:50:10]

This comment is kind of floating and I don't think it's really necessary, please
remove.

[shivanisha, 2017/06/28 19:38:20]

removed

+
+  void CreateWriters(const std::string& url) {
+    disk_cache::Entry* disk_entry = nullptr;

[jkarlin, 2017/06/28 15:50:10]

You need to Close() each entry when you're done with it, else you'll have a
leak.

[shivanisha, 2017/06/28 19:38:20]

done

+    cache_.CreateBackendEntry(kSimpleGET_Transaction.url, &disk_entry, nullptr);
+    writers_ = base::MakeUnique<HttpCache::Writers>(disk_entry);

[Randy Smith (Not in Mondays), 2017/06/27 18:50:56]

Sorry, missed this on the last round.  Josh suggested using MockDiskEntry here,
and I agree.

[shivanisha, 2017/06/27 19:30:41]

cache_.CreateBackendEntry is creating a MockDiskEntry and disk_cache::Entry is
its abstract base class.

[Randy Smith (Not in Mondays), 2017/06/28 15:48:27]

Ooops.  Ok, ignore this note :-}.

+  }
+
+  std::unique_ptr<HttpTransaction> CreateNetworkTransaction() {
+    std::unique_ptr<HttpTransaction> transaction;
+    MockNetworkLayer* network_layer = cache_.network_layer();
+    network_layer->CreateTransaction(DEFAULT_PRIORITY, &transaction);
+    return transaction;
+  }
+
+  void CreateWritersAddTransaction(bool is_exclusive = false) {
+    std::unique_ptr<HttpTransaction> transaction;
+    int rv = cache_.CreateTransaction(&transaction);
+    DCHECK_EQ(rv, OK);

[jkarlin, 2017/06/28 15:50:10]

Avoid using DCHECK* macros in tests, as they crash the test binary and leave
bots in a bad state. Use the ASSERT_xx() and EXPECT_xx() family of macros, which
report failures gracefully and can continue running other tests.

[shivanisha, 2017/06/28 19:38:20]

done

+    DCHECK(transaction.get());

[jkarlin, 2017/06/28 15:50:10]

s/transaction.get()/transaction/

[shivanisha, 2017/06/28 19:38:21]

done

+
+    TestCompletionCallback callback;
+
+    CreateWriters(kSimpleGET_Transaction.url);
+
+    // Create and Start a mock network transaction.
+    std::unique_ptr<HttpTransaction> network_transaction;
+    network_transaction = CreateNetworkTransaction();
+    network_transaction->Start(&request_, callback.callback(),
+                               NetLogWithSource());
+    base::RunLoop().RunUntilIdle();
+
+    DCHECK(writers_->IsEmpty());
+    writers_->AddTransaction((HttpCache::Transaction*)(transaction.get()),

[jkarlin, 2017/06/28 15:50:10]

This cast (here and elsewhere) makes me nervous, as there is no guarantee that
MockHttpCache will return a HttpCache::Transaction.

Instead, I suggest changing MockHttpCache::CreateTransaction to:

int CreateTransaction(std::unique_ptr<HttpCache::Transaction>* trans);

If, in the future, we make a mock transaction we can change it to that instead.

Also note that when casting use static_cast<>() instead of C-style casts.

[shivanisha, 2017/06/28 19:38:21]

But MockHttpCache::CreateTransaction invokes HttpCache::CreateTransaction which
requires an HttpTransaction

Changed to static_cast

[jkarlin, 2017/06/30 18:20:39]

Ah.. because we're passing unique_ptrs to things around.. right. Given that it's
a test it seems reasonable.

+                             std::move(network_transaction), is_exclusive);
+    transactions_.push_back(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((HttpCache::Transaction*)(transaction.get()),
+                             nullptr, false);
+    transactions_.push_back(std::move(transaction));
+  }
+
+  int Read(std::string* result) {
+    DCHECK_GE(transactions_.size(), (size_t)1);
+    HttpCache::Transaction* transaction =
+        (HttpCache::Transaction*)(transactions_.begin()->get());
+    TestCompletionCallback callback;
+
+    std::string content;
+    int rv = 0;
+    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;
+  }
+
+  int ReadAll(std::vector<std::string>& results) {

[jkarlin, 2017/06/28 15:50:10]

output parameters should be raw pointers here and below

[shivanisha, 2017/06/28 19:38:21]

done

+    int rv = 0;
+    do {
+      std::vector<scoped_refptr<IOBuffer>> bufs;
+      std::vector<TestCompletionCallback> callbacks(results.size());
+
+      // Multiple transactions should be able to read.
+      for (size_t i = 0; i < transactions_.size(); i++) {
+        bufs.push_back(new IOBuffer(256));
+        rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(),
+                            (HttpCache::Transaction*)transactions_[i].get());
+        EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+      }
+
+      if (rv == ERR_IO_PENDING) {
+        int prev_rv = callbacks[0].WaitForResult();
+        for (size_t i = 1; i < callbacks.size(); i++) {
+          rv = callbacks[i].WaitForResult();
+          EXPECT_EQ(rv, prev_rv);
+          prev_rv = rv;
+        }
+      }
+
+      if (rv > 0) {
+        for (size_t i = 0; i < results.size(); i++)
+          results[i].append(bufs[i]->data(), rv);
+      } else if (rv <= 0) {
+        return rv;
+      }
+    } while (rv > 0);
+
+    return OK;
+  }
+
+  int ReadAllDeleteActiveTransaction(std::vector<std::string>& results) {
+    int rv = 0;
+    bool first_iter = true;
+    do {
+      std::vector<scoped_refptr<IOBuffer>> bufs;
+      std::vector<TestCompletionCallback> callbacks(results.size());
+
+      // Multiple transactions should be able to read.
+      for (size_t i = 0; i < transactions_.size(); i++) {
+        bufs.push_back(new IOBuffer(256));
+        if (!first_iter && i == 0)
+          continue;
+
+        rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(),
+                            (HttpCache::Transaction*)transactions_[i].get());
+        EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+      }
+
+      if (rv == ERR_IO_PENDING) {
+        if (first_iter) {
+          // Delete active transaction.
+          RemoveFirstTransaction();
+        }
+        int prev_rv = callbacks[1].WaitForResult();
+        for (size_t i = 2; i < callbacks.size(); i++) {
+          rv = callbacks[i].WaitForResult();
+          EXPECT_EQ(rv, prev_rv);
+          prev_rv = rv;
+        }
+      }
+
+      if (rv > 0) {
+        for (size_t i = 0; i < results.size(); i++)
+          results[i].append(bufs[i]->data(), rv);
+      }
+      first_iter = false;
+    } while (rv > 0);
+
+    return rv;
+  }
+
+  int ReadAllDeleteWaitingTransaction(std::vector<std::string>& results,

[jkarlin, 2017/06/28 15:50:10]

output params come after input params here and below

[shivanisha, 2017/06/28 19:38:21]

done

+                                      size_t waiting_index) {
+    int rv = 0;
+    bool first_iter = true;
+    do {
+      std::vector<scoped_refptr<IOBuffer>> bufs;
+      std::vector<TestCompletionCallback> callbacks(results.size());
+
+      // Multiple transactions should be able to read.
+      for (size_t i = 0; i < transactions_.size(); i++) {
+        bufs.push_back(new IOBuffer(256));
+        if (!first_iter && i == waiting_index)
+          continue;
+
+        rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(),
+                            (HttpCache::Transaction*)transactions_[i].get());
+        EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+      }
+
+      if (rv == ERR_IO_PENDING) {
+        if (first_iter) {
+          // Delete waiting transaction.
+          writers_->RemoveTransaction(
+              (HttpCache::Transaction*)transactions_.at(waiting_index).get());
+        }
+        for (size_t j = 0; j < callbacks.size(); j++) {
+          if (j == waiting_index)
+            continue;
+          rv = callbacks[j].WaitForResult();

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

Confused--this looks like we throw away all rvs except for the last.  Should we
be expecting/asserting consistency?  (Or expected inconsistency?)

If this routine is only expected to be used in cases in which the rvs will be
positive (so that the first error means a test failure and we don't really care
about what happens after), document that in a comment above the routine?  

(Dual comments applies to anywhere else in the file where we're stopping short
on error rather than executing code for all the transactions.)

[shivanisha, 2017/06/28 18:02:33]

Added asserts to check that same rv is returned by all transactions

+        }
+      }
+
+      if (rv > 0) {
+        size_t i = 0;
+        for (auto& result : results) {

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

Missed a case on the "Increment i && use for (auto&" pattern? 
(https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...)

[shivanisha, 2017/06/28 18:02:33]

This was missed, changed now.

+          if (i == waiting_index) {
+            i++;
+            continue;
+          }
+          result.append(bufs[i]->data(), rv);
+          i++;
+        }
+      }
+      first_iter = false;
+    } while (rv > 0);
+
+    return rv;
+  }
+
+  int ReadAllDeleteIdleTransaction(std::vector<std::string>& results,
+                                   size_t idle_index) {
+    int rv = 0;
+    bool first_iter = true;
+    do {
+      std::vector<scoped_refptr<IOBuffer>> bufs;
+      std::vector<TestCompletionCallback> callbacks(results.size());
+
+      // Multiple transactions should be able to read.
+      for (size_t i = 0; i < transactions_.size(); i++) {
+        bufs.push_back(new IOBuffer(256));
+        if (i == idle_index)
+          continue;
+
+        rv = writers_->Read(bufs[i].get(), 256, callbacks[i].callback(),
+                            (HttpCache::Transaction*)transactions_[i].get());
+        EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+      }
+
+      if (rv == ERR_IO_PENDING) {
+        if (first_iter) {
+          // Delete idle transaction.
+          writers_->RemoveTransaction(
+              (HttpCache::Transaction*)transactions_.at(idle_index).get());
+        }
+        for (size_t j = 0; j < callbacks.size(); j++) {
+          if (j == idle_index)
+            continue;
+          rv = callbacks[j].WaitForResult();
+        }
+      }
+
+      if (rv > 0) {
+        size_t i = 0;
+        for (auto& result : results) {

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

Missed a case on the "Increment i && use for (auto&" pattern? 
(https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...)

[shivanisha, 2017/06/28 18:02:33]

Changed. Also did a search on all uses of auto to figure out all such cases.
Thanks

+          if (i == idle_index) {
+            i++;
+            continue;
+          }
+          result.append(bufs[i]->data(), rv);
+          i++;
+        }
+      }
+      first_iter = false;
+    } while (rv > 0);
+
+    return rv;
+  }
+
+  int ReadCacheWriteFailure(std::vector<std::string>& results) {
+    int rv = 0;
+    bool first_iter = true;
+    do {
+      std::vector<scoped_refptr<IOBuffer>> bufs;
+      std::vector<TestCompletionCallback> callbacks(results.size());
+
+      // Fail the request.
+      cache_.disk_cache()->set_soft_failures(true);
+
+      // We have to open the entry again to propagate the failure flag.
+      disk_cache::Entry* en;
+      cache_.OpenBackendEntry(kSimpleGET_Transaction.url, &en);
+      en->Close();
+
+      for (size_t i = 0; i < transactions_.size(); i++) {
+        bufs.push_back(new IOBuffer(30));
+
+        if (!first_iter && i > 0)
+          break;
+        rv = writers_->Read(bufs[i].get(), 30, callbacks[i].callback(),
+                            (HttpCache::Transaction*)transactions_[i].get());
+        EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+      }
+
+      if (rv == ERR_IO_PENDING) {
+        int i = 0;
+        for (auto& callback : callbacks) {

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

Missed a case on the "Increment i && use for (auto&" pattern? 
(https://codereview.chromium.org/2886483002/diff/440001/net/http/http_cache_wr...)

[shivanisha, 2017/06/28 18:02:33]

Changed to use index in for loop

+          // Only active transaction should succeed.
+          if (i == 0) {
+            rv = callback.WaitForResult();
+            EXPECT_TRUE(rv >= 0);
+            results[0].append(bufs[i]->data(), rv);
+            if (rv == 0)
+              return rv;

[Randy Smith (Not in Mondays), 2017/06/27 18:49:37]

Is this because we don't expect to get an rv of zero in this routine?  This
seems like an "early return on success" case, which seems strange--if it's
actually what's wanted, a comment as to why?

[shivanisha, 2017/06/28 18:02:33]

rv == 0 implies the end of reading by the active transaction and thus we return
from here because there is nothing else to do. 
Changing this loop for readability.
Changed the loop to be while (active_transaction_rv > 0) instead of while (true)
and then we do not need this if condition

+          } else if (first_iter) {
+            rv = callback.WaitForResult();
+            EXPECT_EQ(ERR_CACHE_WRITE_FAILURE, rv);
+          }
+          i++;
+        }
+      }
+
+      first_iter = false;
+    } while (true);
+
+    return OK;
+  }
+
+  int ReadNetworkFailure(std::vector<std::string>& results) {
+    int rv = 0;
+    std::vector<scoped_refptr<IOBuffer>> bufs;
+    std::vector<TestCompletionCallback> callbacks(results.size());
+
+    // Fail the request.
+    MockNetworkTransaction* network_transaction =
+        (MockNetworkTransaction*)(writers_->network_transaction());
+    network_transaction->SetReadError(ERR_INTERNET_DISCONNECTED);
+
+    for (size_t i = 0; i < transactions_.size(); i++) {
+      bufs.push_back(new IOBuffer(30));
+
+      rv = writers_->Read(bufs[i].get(), 30, callbacks[i].callback(),
+                          (HttpCache::Transaction*)transactions_[i].get());
+      EXPECT_EQ(rv, ERR_IO_PENDING);  // Since the default is asynchronous.
+    }
+
+    if (rv == ERR_IO_PENDING) {
+      for (auto& callback : callbacks) {
+        rv = callback.WaitForResult();
+        EXPECT_EQ(ERR_INTERNET_DISCONNECTED, rv);
+      }
+    }
+
+    return ERR_INTERNET_DISCONNECTED;
+  }
+
+  bool StopCaching() {
+    HttpCache::Transaction* transaction =
+        (HttpCache::Transaction*)(transactions_.begin()->get());
+    DCHECK(transaction);
+    return writers_->StopCaching(transaction);
+  }
+
+  void RemoveFirstTransaction() {
+    HttpCache::Transaction* transaction =
+        (HttpCache::Transaction*)(transactions_.begin()->get());
+    DCHECK(transaction);
+    writers_->RemoveTransaction(transaction);
+  }
+
+  MockHttpCache cache_;
+  std::unique_ptr<HttpCache::Writers> writers_;
+
+  // Should be before transactions_ since it is accessed in the network
+  // transaction's destructor.
+  MockHttpRequest request_;
+
+  std::vector<std::unique_ptr<HttpTransaction>> transactions_;
+};
+
+// Tests successful addition of a transaction.
+TEST_F(WritersTest, AddTransaction) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+}
+
+// Tests successful addition of multiple transaction.
+TEST_F(WritersTest, AddManyTransactions) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  for (int i = 0; i < 5; i++)
+    AddTransactionToExistingWriters();
+
+  EXPECT_EQ(writers_->CountTransactionsForTesting(), 6);
+}
+

[jkarlin, 2017/06/28 15:50:10]

I don't see a test for the public IsPresent function.

[shivanisha, 2017/06/28 19:38:20]

Added in CreateWritersAddTransaction

+// Tests that CanAddWriters should return false if it is exclusive writing.
+TEST_F(WritersTest, AddTransactionsExclusive) {
+  CreateWritersAddTransaction(true);

[jkarlin, 2017/06/28 15:50:10]

nit: true /* is_exclusive */

[shivanisha, 2017/06/28 19:38:21]

done

+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_FALSE(writers_->CanAddWriters());
+}
+
+// Tests StopCaching should not stop caching if there are multiple writers.
+TEST_F(WritersTest, StopCachingMultipleWriters) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+
+  EXPECT_FALSE(StopCaching());
+  EXPECT_TRUE(writers_->CanAddWriters());
+}
+

[jkarlin, 2017/06/28 15:50:10]

It'd be good to add a test for UpdatePriority. Since WritersTest is a friend
class of writers it could verify that priority_ is properly updated. 

[shivanisha, 2017/06/28 19:38:21]

Added in  RemoveIdleTransaction

+// Tests StopCaching should stop caching if there is a single writer.
+TEST_F(WritersTest, StopCaching) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(StopCaching());
+  EXPECT_FALSE(writers_->CanAddWriters());
+}
+
+// Tests removing of an idle transaction.
+TEST_F(WritersTest, RemoveIdleTransaction) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  RemoveFirstTransaction();
+  EXPECT_TRUE(writers_->IsEmpty());
+}
+
+// Tests that Read is successful.
+TEST_F(WritersTest, Read) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  std::string content;
+  int rv = Read(&content);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+  EXPECT_EQ(expected, content);

[jkarlin, 2017/06/28 15:50:10]

nit: seems reasonable to combine these two lines here and elsewhere:

EXPECT_EQ(std::string(kSimpleGET_Transaction.data), content);

+}
+
+// Tests that multiple transactions can read the same data simultaneously.
+TEST_F(WritersTest, ReadMultiple) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(2);
+  int rv = ReadAll(contents);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+
+  for (auto content : contents)
+    EXPECT_EQ(expected, content);
+}
+
+// Tests that ongoing Read completes even when active transaction is deleted
+// mid-read. Any transactions waiting should be able to get the read buffer.
+TEST_F(WritersTest, ReadMultipleDeleteActiveTransaction) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(3);
+  int rv = ReadAllDeleteActiveTransaction(contents);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+
+  for (auto content : contents)
+    EXPECT_EQ(expected, content);
+}
+
+// Tests that removing a waiting for read transaction does not impact other
+// transactions.
+TEST_F(WritersTest, ReadMultipleDeleteWaitingTransaction) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(4);
+  size_t waiting_index = 1;
+  int rv = ReadAllDeleteWaitingTransaction(contents, waiting_index);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+
+  size_t i = 0;
+  for (auto content : contents) {
+    if (i == waiting_index)
+      EXPECT_EQ("", content);
+    else
+      EXPECT_EQ(expected, content);
+    i++;
+  }
+}
+
+// Tests that removing an idle transaction does not impact other transactions.
+TEST_F(WritersTest, ReadMultipleDeleteIdleTransaction) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(3);
+  size_t idle_index = 1;
+  int rv = ReadAllDeleteIdleTransaction(contents, idle_index);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+
+  size_t i = 0;
+  for (auto content : contents) {
+    if (i == idle_index) {
+      i++;
+      continue;
+    }
+    EXPECT_EQ(expected, content);
+    i++;
+  }
+}
+
+// Tests cache write failure.
+TEST_F(WritersTest, ReadMultipleCacheWriteFailed) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(3);
+  int rv = ReadCacheWriteFailure(contents);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+
+  // Only active_transaction_ should succeed.
+  EXPECT_EQ(expected, contents.at(0));
+
+  // No new transactions should now be added.
+  EXPECT_FALSE(writers_->CanAddWriters());
+}
+
+// Tests that network read failure fails all transactions: active, waiting and
+// idle.
+TEST_F(WritersTest, ReadMultipleNetworkReadFailed) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  std::vector<std::string> contents(3);
+  int rv = ReadNetworkFailure(contents);
+
+  EXPECT_EQ(rv, ERR_INTERNET_DISCONNECTED);
+}
+
+// Tests moving idle writers to readers.
+TEST_F(WritersTest, MoveIdleWritersToReaders) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_TRUE(writers_->CanAddWriters());
+  AddTransactionToExistingWriters();
+  AddTransactionToExistingWriters();
+
+  EXPECT_FALSE(writers_->IsEmpty());
+  writers_->RemoveAllIdleWriters();
+  EXPECT_TRUE(writers_->IsEmpty());
+}
+
+// Tests GetWriterLoadState.
+TEST_F(WritersTest, GetWriterLoadState) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());
+
+  EXPECT_EQ(LOAD_STATE_IDLE, writers_->GetWriterLoadState());
+}
+
+// Tests truncating the entry via Writers.
+TEST_F(WritersTest, TruncateEntry) {
+  CreateWritersAddTransaction();
+  EXPECT_FALSE(writers_->IsEmpty());

[jkarlin, 2017/06/28 15:50:10]

These are the first two lines of every test. How about making them part of
WritesTest initialization?

[shivanisha, 2017/06/28 19:38:21]

I feel it will make the test body less contained so one would have to look up in
atleast two places to understand the test.

[jkarlin, 2017/06/30 18:20:39]

Okay, up to you.

+
+  std::string content;
+  int rv = Read(&content);
+
+  EXPECT_THAT(rv, IsOk());
+  std::string expected(kSimpleGET_Transaction.data);
+  EXPECT_EQ(expected, content);
+
+  writers_->TruncateEntry();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(writers_->IsTruncatedForTesting());
+}
+
+}  // namespace net