Fix URLFetcherDownloadProgressTest to work with remote test server

net/url_request/url_fetcher_impl_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/url_request/url_fetcher_impl.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/file_util.h"
@@ skipping 43 matching lines @@
 class ThrottlingTestURLRequestContextGetter
     : public TestURLRequestContextGetter {
  public:
   ThrottlingTestURLRequestContextGetter(
       base::MessageLoopProxy* io_message_loop_proxy,
       TestURLRequestContext* request_context)
       : TestURLRequestContextGetter(io_message_loop_proxy),
         context_(request_context) {
   }
 
+  // TestURLRequestContextGetter:
   virtual TestURLRequestContext* GetURLRequestContext() OVERRIDE {
     return context_;
   }
 
  protected:
   virtual ~ThrottlingTestURLRequestContextGetter() {}
 
   TestURLRequestContext* const context_;
 };
 
 }  // namespace
 
 class URLFetcherTest : public testing::Test,
                        public URLFetcherDelegate {
  public:
   URLFetcherTest()
       : fetcher_(NULL),
         context_(new ThrottlingTestURLRequestContext()) {
   }
 
   static int GetNumFetcherCores() {
     return URLFetcherImpl::GetNumFetcherCores();
   }
 
   // Creates a URLFetcher, using the program's main thread to do IO.
   virtual void CreateFetcher(const GURL& url);
 
-  // URLFetcherDelegate
+  // URLFetcherDelegate:
   // Subclasses that override this should either call this function or
   // CleanupAfterFetchComplete() at the end of their processing, depending on
   // whether they want to check for a non-empty HTTP 200 response or not.
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
   // Deletes |fetcher| and terminates the message loop.
   void CleanupAfterFetchComplete();
 
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy() {
     return io_message_loop_proxy_;
   }
 
   TestURLRequestContext* request_context() {
     return context_.get();
   }
 
  protected:
+  // testing::Test:
   virtual void SetUp() OVERRIDE {
     testing::Test::SetUp();
 
     io_message_loop_proxy_ = base::MessageLoopProxy::current();
 
 #if defined(USE_NSS)
     crypto::EnsureNSSInit();
     EnsureNSSHttpIOInit();
 #endif
   }
@@ skipping 40 matching lines @@
   io_message_loop_proxy()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
   // If the current message loop is not the IO loop, it will be shut down when
   // the main loop returns and this thread subsequently goes out of scope.
 }
 
 namespace {
 
 // Version of URLFetcherTest that does a POST instead
 class URLFetcherPostTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-
-  // URLFetcherDelegate

[wtc, 2012/11/26 23:19:06]

Strictly speaking, these "URLFetcherDelegate" comments for
OnURLFetchComplete are correct because the OnURLFetchComplete
method ultimately comes from URLFetcherDelegate.

I don't know what is the right class to use in these
comments though...  net/socket/tcp_client_socket_win.h uses
the most specific class name in these method override
comments. I would probably do that, too. I'll let you
decide.

[ppi, 2012/11/29 13:07:59]

I agree - I think that precise "URLFetcherDelegate:" comments make the class
interface more readable. I have restored them in patch set 7.

On 2012/11/26 23:19:06, wtc wrote:

   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 };
 
 // Version of URLFetcherTest that does a POST instead with empty upload body
 class URLFetcherEmptyPostTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 };
 
 // Version of URLFetcherTest that tests download progress reports.
 class URLFetcherDownloadProgressTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  URLFetcherDownloadProgressTest()
+      : previous_progress_(0),
+        first_call_(true),
+        expected_total_size_(0) {
+  }
+
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
+  virtual void OnURLFetchDownloadProgress(const URLFetcher* source,
+                                          int64 current,
+                                          int64 total) OVERRIDE;
 
-  // URLFetcherDelegate
-  virtual void OnURLFetchDownloadProgress(const URLFetcher* source,
-                                          int64 current, int64 total) OVERRIDE;
  protected:
+  // Data returned by the previous callback.

[wtc, 2012/11/26 23:19:06]

"Data" => "Download progress" or "Number of bytes"

[ppi, 2012/11/29 13:07:59]

Absolutely, fixed.

On 2012/11/26 23:19:06, wtc wrote:

   int64 previous_progress_;
-  int64 expected_total_;
+  // Indicates that |previous_progress| is not yet set.
+  bool first_call_;
+  // Size of the file being downloaded, known in advance (provided by each test
+  // case).
+  int64 expected_total_size_;

[wtc, 2012/11/26 23:19:06]

1. We don't need first_call_ any more. Since previous_progress_
is initialized to 0,
    EXPECT_LE(previous_progress_, current);
(on line 468) is also true for the first call.

2. We can reduce the changes in this CL by changing
expected_total_size_ back to its original name expected_total_.

[ppi, 2012/11/29 13:07:59]

I find "expected_total_size_" more descriptive and clear than "expected_total_",
but I am flexible on that. Fixed both in patch set 7.

On 2012/11/26 23:19:06, wtc wrote:

 };
 
-/// Version of URLFetcherTest that tests progress reports at cancellation.
+// Version of URLFetcherTest that tests progress reports at cancellation.
 class URLFetcherDownloadProgressCancelTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
   virtual void OnURLFetchDownloadProgress(const URLFetcher* source,
-                                          int64 current, int64 total) OVERRIDE;
+                                          int64 current,
+                                          int64 total) OVERRIDE;
  protected:
   bool cancelled_;
 };
 
 // Version of URLFetcherTest that tests upload progress reports.
 class URLFetcherUploadProgressTest : public URLFetcherTest {
  public:
-  virtual void CreateFetcher(const GURL& url);
-
-  // URLFetcherDelegate
+  // URLFetcherTest:
+  virtual void CreateFetcher(const GURL& url) OVERRIDE;
   virtual void OnURLFetchUploadProgress(const URLFetcher* source,
-                                        int64 current, int64 total);
+                                        int64 current,
+                                        int64 total) OVERRIDE;
  protected:
   int64 previous_progress_;
   std::string chunk_;
   int64 number_of_chunks_added_;
 };
 
 // Version of URLFetcherTest that tests headers.
 class URLFetcherHeadersTest : public URLFetcherTest {
  public:
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 };
 
 // Version of URLFetcherTest that tests SocketAddress.
 class URLFetcherSocketAddressTest : public URLFetcherTest {
  public:
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
  protected:
   std::string expected_host_;
   uint16 expected_port_;
 };
 
 // Version of URLFetcherTest that tests stopping on a redirect.
 class URLFetcherStopOnRedirectTest : public URLFetcherTest {
  public:
   URLFetcherStopOnRedirectTest();
   virtual ~URLFetcherStopOnRedirectTest();
 
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
   // The URL we should be redirected to.
   static const char* kRedirectTarget;
 
   bool callback_called_;  // Set to true in OnURLFetchComplete().
 };
 
 // Version of URLFetcherTest that tests overload protection.
 class URLFetcherProtectTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
  private:
   Time start_time_;
 };
 
 // Version of URLFetcherTest that tests overload protection, when responses
 // passed through.
 class URLFetcherProtectTestPassedThrough : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
  private:
   Time start_time_;
 };
 
 // Version of URLFetcherTest that tests bad HTTPS requests.
 class URLFetcherBadHTTPSTest : public URLFetcherTest {
  public:
   URLFetcherBadHTTPSTest();
 
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  private:
   FilePath cert_dir_;
 };
 
 // Version of URLFetcherTest that tests request cancellation on shutdown.
 class URLFetcherCancelTest : public URLFetcherTest {
  public:
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
   void CancelRequest();
 };
 
 // Version of TestURLRequestContext that posts a Quit task to the IO
 // thread once it is deleted.
 class CancelTestURLRequestContext : public ThrottlingTestURLRequestContext {
  public:
   explicit CancelTestURLRequestContext() {
@@ skipping 11 matching lines @@
     : public TestURLRequestContextGetter {
  public:
   CancelTestURLRequestContextGetter(
       base::MessageLoopProxy* io_message_loop_proxy,
       const GURL& throttle_for_url)
       : TestURLRequestContextGetter(io_message_loop_proxy),
         io_message_loop_proxy_(io_message_loop_proxy),
         context_created_(false, false),
         throttle_for_url_(throttle_for_url) {
   }
+
+  // TestURLRequestContextGetter:
   virtual TestURLRequestContext* GetURLRequestContext() OVERRIDE {
     if (!context_.get()) {
       context_.reset(new CancelTestURLRequestContext());
       DCHECK(context_->throttler_manager());
 
       // Registers an entry for test url. The backoff time is calculated by:
       //     new_backoff = 2.0 * old_backoff + 0
       // The initial backoff is 2 seconds and maximum backoff is 4 seconds.
       // Maximum retries allowed is set to 2.
       scoped_refptr<URLRequestThrottlerEntry> entry(
           new URLRequestThrottlerEntry(
               context_->throttler_manager(),
               "", 200, 3, 2000, 2.0, 0.0, 4000));
       context_->throttler_manager()->OverrideEntryForTests(
           throttle_for_url_, entry);
 
       context_created_.Signal();
     }
     return context_.get();
   }
+
   virtual scoped_refptr<base::MessageLoopProxy> GetIOMessageLoopProxy() const {
     return io_message_loop_proxy_;
   }
+
   void WaitForContextCreation() {
     context_created_.Wait();
   }
 
  protected:
   virtual ~CancelTestURLRequestContextGetter() {}
 
  private:
   scoped_ptr<TestURLRequestContext> context_;
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
   base::WaitableEvent context_created_;
   GURL throttle_for_url_;
 };
 
 // Version of URLFetcherTest that tests retying the same request twice.
 class URLFetcherMultipleAttemptTest : public URLFetcherTest {
  public:
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
  private:
   std::string data_;
 };
 
 class URLFetcherFileTest : public URLFetcherTest {
  public:
   URLFetcherFileTest() : take_ownership_of_file_(false),
                          expected_file_error_(base::PLATFORM_FILE_OK) {}
 
   void CreateFetcherForFile(const GURL& url, const FilePath& file_path);
   void CreateFetcherForTempFile(const GURL& url);
 
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
   FilePath expected_file_;
   FilePath file_path_;
 
   // Set by the test. Used in OnURLFetchComplete() to decide if
   // the URLFetcher should own the temp file, so that we can test
   // disowning prevents the file from being deleted.
   bool take_ownership_of_file_;
@@ skipping 37 matching lines @@
 
   CleanupAfterFetchComplete();
   // Do not call the super class method URLFetcherTest::OnURLFetchComplete,
   // since it expects a non-empty response.
 }
 
 void URLFetcherDownloadProgressTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
   fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
       io_message_loop_proxy(), request_context()));
-  previous_progress_ = 0;
   fetcher_->Start();
 }
 
 void URLFetcherDownloadProgressTest::OnURLFetchDownloadProgress(
     const URLFetcher* source, int64 current, int64 total) {
-  // Increasing between 0 and total.
+  // Progress has to be between 0 and total size.
   EXPECT_LE(0, current);
   EXPECT_GE(total, current);
-  EXPECT_LE(previous_progress_, current);
+
+  EXPECT_EQ(expected_total_size_, total);
+
+  // Progress has to be non-decreasing.
+  if (!first_call_)
+    EXPECT_LE(previous_progress_, current);
+
   previous_progress_ = current;
-  EXPECT_EQ(expected_total_, total);
+  first_call_ = false;

[wtc, 2012/11/26 23:19:06]

If we remove first_call_ and use the original expected_total_
name, I believe this method doesn't need any change.

[ppi, 2012/11/29 13:07:59]

Removed "first_call_". I did rename "current" to "progress", to pair well with
"previous_progress_" though.

On 2012/11/26 23:19:06, wtc wrote:

 }
 
 void URLFetcherDownloadProgressCancelTest::CreateFetcher(const GURL& url) {
   fetcher_ = new URLFetcherImpl(url, URLFetcher::GET, this);
   fetcher_->SetRequestContext(new ThrottlingTestURLRequestContextGetter(
       io_message_loop_proxy(), request_context()));
   cancelled_ = false;
   fetcher_->Start();
 }
 
@@ skipping 379 matching lines @@
 
 TEST_F(URLFetcherDownloadProgressTest, Basic) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
 
   // Get a file large enough to require more than one read into
   // URLFetcher::Core's IOBuffer.
   static const char kFileToFetch[] = "animate1.gif";
-  file_util::GetFileSize(test_server.document_root().AppendASCII(kFileToFetch),
-                         &expected_total_);
+  // Hardcoded file size - it cannot be easily fetched when a remote http server
+  // is used for testing.
+  static const int64 kFileSize = 19021;
+
+  expected_total_size_ = kFileSize;
+
   CreateFetcher(test_server.GetURL(
       std::string(kTestServerFilePrefix) + kFileToFetch));
 
   MessageLoop::current()->Run();
 }
 
 TEST_F(URLFetcherDownloadProgressCancelTest, CancelWhileProgressReport) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,
                          FilePath(kDocRoot));
@@ skipping 375 matching lines @@
   MessageLoop::current()->Run();  // OnURLFetchComplete() will Quit().
 
   MessageLoop::current()->RunUntilIdle();
   ASSERT_FALSE(file_util::PathExists(file_path_))
       << file_path_.value() << " not removed.";
 }
 
 }  // namespace
 
 }  // namespace net