Fix URLFetcherDownloadProgressTest to work with remote test server

net/url_request/url_fetcher_impl_unittest.cc

Index: net/url_request/url_fetcher_impl_unittest.cc
diff --git a/net/url_request/url_fetcher_impl_unittest.cc b/net/url_request/url_fetcher_impl_unittest.cc
index f3f06595321c451b3eca7a5f052896f5b2dbcddd..ab8ee7d31899a34f0232c89f802ee3575fa2d1a1 100644
--- a/net/url_request/url_fetcher_impl_unittest.cc
+++ b/net/url_request/url_fetcher_impl_unittest.cc
@@ -61,6 +61,7 @@ class ThrottlingTestURLRequestContextGetter
         context_(request_context) {
   }
 
+  // TestURLRequestContextGetter:
   virtual TestURLRequestContext* GetURLRequestContext() OVERRIDE {
     return context_;
   }
@@ -88,7 +89,7 @@ class URLFetcherTest : public testing::Test,
   // 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.
@@ -106,6 +107,7 @@ class URLFetcherTest : public testing::Test,
   }
 
  protected:
+  // testing::Test:
   virtual void SetUp() OVERRIDE {
     testing::Test::SetUp();
 
@@ -166,47 +168,53 @@ 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.
-  virtual void CreateFetcher(const GURL& url) OVERRIDE;
+  URLFetcherDownloadProgressTest()
+      : previous_progress_(0),
+        first_call_(true),
+        expected_total_size_(0) {
+  }
 
-  // URLFetcherDelegate
+  // URLFetcherTest:
+  virtual void CreateFetcher(const GURL& url) OVERRIDE;
   virtual void OnURLFetchDownloadProgress(const URLFetcher* source,
-                                          int64 current, int64 total) OVERRIDE;
+                                          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_;
 };
@@ -214,11 +222,11 @@ class URLFetcherDownloadProgressCancelTest : public URLFetcherTest {
 // 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_;
@@ -228,14 +236,14 @@ class URLFetcherUploadProgressTest : public URLFetcherTest {
 // 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_;
@@ -248,9 +256,8 @@ class URLFetcherStopOnRedirectTest : public URLFetcherTest {
   URLFetcherStopOnRedirectTest();
   virtual ~URLFetcherStopOnRedirectTest();
 
-  // URLFetcherTest override.
+  // URLFetcherTest:
   virtual void CreateFetcher(const GURL& url) OVERRIDE;
-  // URLFetcherDelegate
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
@@ -263,9 +270,8 @@ class URLFetcherStopOnRedirectTest : public URLFetcherTest {
 // 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_;
@@ -275,9 +281,8 @@ class URLFetcherProtectTest : public URLFetcherTest {
 // 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_;
@@ -288,7 +293,7 @@ class URLFetcherBadHTTPSTest : public URLFetcherTest {
  public:
   URLFetcherBadHTTPSTest();
 
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  private:
@@ -298,9 +303,8 @@ class URLFetcherBadHTTPSTest : public URLFetcherTest {
 // 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();
@@ -332,6 +336,8 @@ class CancelTestURLRequestContextGetter
         context_created_(false, false),
         throttle_for_url_(throttle_for_url) {
   }
+
+  // TestURLRequestContextGetter:
   virtual TestURLRequestContext* GetURLRequestContext() OVERRIDE {
     if (!context_.get()) {
       context_.reset(new CancelTestURLRequestContext());
@@ -352,9 +358,11 @@ class CancelTestURLRequestContextGetter
     }
     return context_.get();
   }
+
   virtual scoped_refptr<base::MessageLoopProxy> GetIOMessageLoopProxy() const {
     return io_message_loop_proxy_;
   }
+
   void WaitForContextCreation() {
     context_created_.Wait();
   }
@@ -372,7 +380,7 @@ class CancelTestURLRequestContextGetter
 // 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_;
@@ -386,7 +394,7 @@ class URLFetcherFileTest : public URLFetcherTest {
   void CreateFetcherForFile(const GURL& url, const FilePath& file_path);
   void CreateFetcherForTempFile(const GURL& url);
 
-  // URLFetcherDelegate
+  // URLFetcherTest:
   virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
 
  protected:
@@ -444,18 +452,23 @@ 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) {
@@ -855,8 +868,12 @@ TEST_F(URLFetcherDownloadProgressTest, Basic) {
   // 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));