Convert DnsTransactionTest to use SequencedSocketData

net/dns/dns_transaction_unittest.cc

 // Copyright (c) 2012 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/dns/dns_transaction.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/rand_util.h"
@@ skipping 28 matching lines @@
                 uint16 qtype,
                 IoMode mode,
                 bool use_tcp)
       : query_(new DnsQuery(id, DomainFromDot(dotted_name), qtype)),
         use_tcp_(use_tcp) {
     if (use_tcp_) {
       scoped_ptr<uint16> length(new uint16);
       *length = base::HostToNet16(query_->io_buffer()->size());
       writes_.push_back(MockWrite(mode,
                                   reinterpret_cast<const char*>(length.get()),
-                                  sizeof(uint16)));
+                                  sizeof(uint16), num_reads_and_writes()));
       lengths_.push_back(length.release());
     }
-    writes_.push_back(MockWrite(mode,
-                                query_->io_buffer()->data(),
-                                query_->io_buffer()->size()));
+    writes_.push_back(MockWrite(mode, query_->io_buffer()->data(),
+                                query_->io_buffer()->size(),
+                                num_reads_and_writes()));
   }
   ~DnsSocketData() {}
 
   // All responses must be added before GetProvider.
 
   // Adds pre-built DnsResponse. |tcp_length| will be used in TCP mode only.
   void AddResponseWithLength(scoped_ptr<DnsResponse> response, IoMode mode,
                              uint16 tcp_length) {
     CHECK(!provider_.get());
     if (use_tcp_) {
       scoped_ptr<uint16> length(new uint16);
       *length = base::HostToNet16(tcp_length);
       reads_.push_back(MockRead(mode,
                                 reinterpret_cast<const char*>(length.get()),
-                                sizeof(uint16)));
+                                sizeof(uint16), num_reads_and_writes()));
       lengths_.push_back(length.release());
     }
-    reads_.push_back(MockRead(mode,
-                              response->io_buffer()->data(),
-                              response->io_buffer()->size()));
+    reads_.push_back(MockRead(mode, response->io_buffer()->data(),
+                              response->io_buffer()->size(),
+                              num_reads_and_writes()));
     responses_.push_back(response.release());
   }
 
   // Adds pre-built DnsResponse.
   void AddResponse(scoped_ptr<DnsResponse> response, IoMode mode) {
     uint16 tcp_length = response->io_buffer()->size();
     AddResponseWithLength(response.Pass(), mode, tcp_length);
   }
 
   // Adds pre-built response from |data| buffer.
@@ skipping 10 matching lines @@
                         query_->io_buffer()->size(),
                         0));
     dns_protocol::Header* header =
         reinterpret_cast<dns_protocol::Header*>(response->io_buffer()->data());
     header->flags |= base::HostToNet16(dns_protocol::kFlagResponse | rcode);
     AddResponse(response.Pass(), mode);
   }
 
   // Add error response.
   void AddReadError(int error, IoMode mode) {
-    reads_.push_back(MockRead(mode, error));
+    reads_.push_back(MockRead(mode, error, num_reads_and_writes()));
   }
 
   // Build, if needed, and return the SocketDataProvider. No new responses
   // should be added afterwards.
   SocketDataProvider* GetProvider() {
     if (provider_.get())
       return provider_.get();
     // Terminate the reads with ERR_IO_PENDING to prevent overrun and default to
     // timeout.
-    reads_.push_back(MockRead(ASYNC, ERR_IO_PENDING));
-    provider_.reset(new DelayedSocketData(1, &reads_[0], reads_.size(),
-                                          &writes_[0], writes_.size()));
+    reads_.push_back(
+        MockRead(ASYNC, ERR_IO_PENDING, writes_.size() + reads_.size()));
+    provider_.reset(new SequencedSocketData(&reads_[0], reads_.size(),
+                                            &writes_[0], writes_.size()));
     if (use_tcp_) {
       provider_->set_connect_data(MockConnect(reads_[0].mode, OK));
     }
     return provider_.get();
   }
 
   uint16 query_id() const {
     return query_->id();
   }
 
   // Returns true if the expected query was written to the socket.
   bool was_written() const {
     CHECK(provider_.get());
-    return provider_->write_index() > 0;
+    return provider_->WasDataWritten();

[mmenke, 2015/05/04 19:10:57]

For the TCP case, where there are two writes, this isn't write...err...right. 
WasAllDataWritten?  Or just call base::RunLoop().RunAllPending() first (Which is
ugly, but works).

[mmenke, 2015/05/04 19:10:57]

optional:  Seems a little weird for a lower-case named method to be calling a
capitalized one.  Suggest either renaming this, or renaming and inlining
was_data_written.  Latter seems simpler.

Google C++ style guide now says "You may also use lowercase letters for other
very short inlined functions. For example if a function were so cheap you would
not cache the value if you were calling it in a loop, then lowercase naming
would be acceptable."

That doesn't completely disallow this, but I'm assuming this is using lowercase
as an instance of the example, which does make it weird.

[Ryan Hamilton, 2015/05/04 19:20:30]

Good point! I've inlined WasDataWritten().

[Ryan Hamilton, 2015/05/04 19:24:46]

Whoops, sorry, missed this. I don't think I changed the semantics here, do you?
In the old code, we simply checked that write index was > 0, which happens as
soon as the write happen (even if it returns async). Now, the write index
doesn't move until the callback completes so we need to change the
implementation. Do you think I changed the semantics, or do you think that this
was a latent bug?

(What would calling RunAllPending do?)

[mmenke, 2015/05/04 19:34:49]

Ahh...You're right!  I saw the "at_write_eof" just below  WasDataWritten(), and
was assuming that's what the old code did.  I think this was a bug in the test. 
This CL is small enough that I'd like to see if we can fix here, if we can do so
easily and you don't mind (Since these tests are probably never going to be
updated otherwise).  If you prefer not to, I can live with that.
If we aren't at the "write eof", because there's a pending write, it would cause
the write to complete, so we *would* be at the write eof, so you can then just
use at_write_eof (And can probably check the read one, too, though that's less
important).

   }
 
  private:
+  size_t num_reads_and_writes() { return reads_.size() + writes_.size(); }

[mmenke, 2015/05/04 19:10:57]

nit:  const

[Ryan Hamilton, 2015/05/04 19:20:30]

Done.

+
   scoped_ptr<DnsQuery> query_;
   bool use_tcp_;
   ScopedVector<uint16> lengths_;
   ScopedVector<DnsResponse> responses_;
   std::vector<MockWrite> writes_;
   std::vector<MockRead> reads_;
-  scoped_ptr<DelayedSocketData> provider_;
+  scoped_ptr<SequencedSocketData> provider_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsSocketData);
 };
 
 class TestSocketFactory;
 
 // A variant of MockUDPClientSocket which always fails to Connect.
 class FailingUDPClientSocket : public MockUDPClientSocket {
  public:
   FailingUDPClientSocket(SocketDataProvider* data,
@@ skipping 850 matching lines @@
   config_.timeout = TestTimeouts::tiny_timeout();
   ConfigureFactory();
 
   TransactionHelper helper0(".", dns_protocol::kTypeA, ERR_INVALID_ARGUMENT);
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 }
 
 }  // namespace
 
 }  // namespace net