Refactor CrxDownloaderTest tests.

Refactor CrxDownloaderTest tests.

The refactoring is done to accomodate a new callback to be introduced
in the next CL. In addition, the change improves the tests by:
* verifying the content of the downloaded file is correct.
* deleting the temporary file, which was left behind.

BUG=366388
TBR=waffles

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266722

[Sorin Jianu, 2014-04-26 17:42:51]

I am landing this as a tbr to unblock some future work related to adding the
progress callback.

For the most part this is refactoring is a NOP but I've slightly improved the
tests by verifying the integrity of the downloaded file and cleaning up the
temporary file created by the fetcher before the test ends.

[Sorin Jianu, 2014-04-26 17:43:07]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-26 17:44:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/20001

[commit-bot: I haz the power, 2014-04-26 17:44:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 17:44:33]

Failed to apply patch for
chrome/browser/component_updater/test/crx_downloader_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/component_updater/test/crx_downloader_unittest.cc
  Hunk #3 succeeded at 60 with fuzz 1 (offset 2 lines).
  Hunk #4 FAILED at 76.
  Hunk #5 succeeded at 97 (offset 5 lines).
  Hunk #6 succeeded at 113 (offset 2 lines).
  Hunk #7 FAILED at 139.
  Hunk #8 FAILED at 152.
  Hunk #9 FAILED at 179.
  Hunk #10 FAILED at 203.
  Hunk #11 FAILED at 225.
  Hunk #12 FAILED at 251.
  Hunk #13 FAILED at 276.
  Hunk #14 succeeded at 305 (offset 5 lines).
  Hunk #15 FAILED at 310.
  9 out of 15 hunks FAILED -- saving rejects to file
chrome/browser/component_updater/test/crx_downloader_unittest.cc.rej

Patch:       chrome/browser/component_updater/test/crx_downloader_unittest.cc
Index: chrome/browser/component_updater/test/crx_downloader_unittest.cc
diff --git a/chrome/browser/component_updater/test/crx_downloader_unittest.cc
b/chrome/browser/component_updater/test/crx_downloader_unittest.cc
index
c06339cd10009263494422ddfee9469b2c343252..ad92ea5cfee7f3dc2a453db8e370bed5b009b6af
100644
--- a/chrome/browser/component_updater/test/crx_downloader_unittest.cc
+++ b/chrome/browser/component_updater/test/crx_downloader_unittest.cc
@@ -20,6 +20,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserThread;
+using base::ContentsEqual;
 
 namespace component_updater {
 
@@ -28,7 +29,9 @@ namespace {
 // Intercepts HTTP GET requests sent to "localhost".
 typedef content::URLLocalHostRequestPrepackagedInterceptor GetInterceptor;
 
-base::FilePath test_file(const char* file) {
+const char kTestFileName[] = "jebgalgnebhfojomionfpkfelancnnkf.crx";
+
+base::FilePath MakeTestFilePath(const char* file) {
   base::FilePath path;
   PathService::Get(chrome::DIR_TEST_DATA, &path);
   return path.AppendASCII("components").AppendASCII(file);
@@ -55,10 +58,9 @@ class CrxDownloaderTest : public testing::Test {
   scoped_ptr<CrxDownloader> crx_downloader_;
 
   int crx_context_;
-  int error_;
-  base::FilePath response_;
 
-  int num_calls_;
+  int num_download_complete_calls_;
+  CrxDownloader::Result download_complete_result_;
 
   // A magic value for the context to be used in the tests.
   static const int kExpectedContext = 0xaabb;
@@ -74,8 +76,7 @@ const int CrxDownloaderTest::kExpectedContext;
 
 CrxDownloaderTest::CrxDownloaderTest()
     : crx_context_(0),
-      error_(0),
-      num_calls_(0),
+      num_download_complete_calls_(0),
       blocking_task_runner_(BrowserThread::GetBlockingPool()->
           GetSequencedTaskRunnerWithShutdownBehavior(
               BrowserThread::GetBlockingPool()->GetSequenceToken(),
@@ -90,7 +91,8 @@ CrxDownloaderTest::~CrxDownloaderTest() {
 }
 
 void CrxDownloaderTest::SetUp() {
-  num_calls_ = 0;
+  num_download_complete_calls_ = 0;
+  download_complete_result_ = CrxDownloader::Result();
   crx_downloader_.reset(CrxDownloader::Create(
       false,    // Do not use the background downloader in these tests.
       context_.get(),
@@ -108,13 +110,11 @@ void CrxDownloaderTest::Quit() {
     quit_closure_.Run();
 }
 
-void CrxDownloaderTest::DownloadComplete(
-    int crx_context,
-    const CrxDownloader::Result& result) {
-  ++num_calls_;
+void CrxDownloaderTest::DownloadComplete(int crx_context,
+                                         const CrxDownloader::Result& result) {
+  ++num_download_complete_calls_;
   crx_context_ = crx_context;
-  error_ = result.error;
-  response_ = result.response;
+  download_complete_result_ = result;
   Quit();
 }
 
@@ -138,12 +138,12 @@ void CrxDownloaderTest::RunThreadsUntilIdle() {
 TEST_F(CrxDownloaderTest, NoUrl) {
   std::vector<GURL> urls;
   crx_downloader_->StartDownload(urls);
-
   RunThreadsUntilIdle();
-  EXPECT_EQ(-1, error_);
-  EXPECT_EQ(kExpectedContext, crx_context_);
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_EQ(1, num_download_complete_calls_);
+  EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_EQ(-1, download_complete_result_.error);
+  EXPECT_TRUE(download_complete_result_.response.empty());
 }
 
 // Tests that downloading from one url is successful.
@@ -151,19 +151,21 @@ TEST_F(CrxDownloaderTest, OneUrl) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   crx_downloader_->StartDownloadFromUrl(expected_crx_url);
-
   RunThreads();
+
   EXPECT_EQ(1, interceptor.GetHitCount());
-  EXPECT_EQ(0, error_);
+
+  EXPECT_EQ(1, num_download_complete_calls_);
   EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_EQ(0, download_complete_result_.error);
+  EXPECT_TRUE(ContentsEqual(download_complete_result_.response, test_file));
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_TRUE(base::DeleteFile(download_complete_result_.response, false));
 }
 
 // Tests that specifying from two urls has no side effects. Expect a successful
@@ -178,23 +180,25 @@ TEST_F(CrxDownloaderTest, MAYBE_TwoUrls) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   std::vector<GURL> urls;
   urls.push_back(expected_crx_url);
   urls.push_back(expected_crx_url);
 
   crx_downloader_->StartDownload(urls);
-
   RunThreads();
+
   EXPECT_EQ(1, interceptor.GetHitCount());
-  EXPECT_EQ(0, error_);
+
+  EXPECT_EQ(1, num_download_complete_calls_);
   EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_EQ(0, download_complete_result_.error);
+  EXPECT_TRUE(ContentsEqual(download_complete_result_.response, test_file));
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_TRUE(base::DeleteFile(download_complete_result_.response, false));
 }
 
 // Tests that an invalid host results in a download error.
@@ -202,21 +206,21 @@ TEST_F(CrxDownloaderTest, OneUrl_InvalidHost) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   crx_downloader_->StartDownloadFromUrl(
       GURL("http://no.such.host"
            "/download/jebgalgnebhfojomionfpkfelancnnkf.crx"));
-
   RunThreads();
+
   EXPECT_EQ(0, interceptor.GetHitCount());
-  EXPECT_NE(0, error_);
-  EXPECT_EQ(kExpectedContext, crx_context_);
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_EQ(1, num_download_complete_calls_);
+  EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_NE(0, download_complete_result_.error);
+  EXPECT_TRUE(download_complete_result_.response.empty());
 }
 
 // Tests that an invalid path results in a download error.
@@ -224,19 +228,19 @@ TEST_F(CrxDownloaderTest, OneUrl_InvalidPath) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   crx_downloader_->StartDownloadFromUrl(GURL("http://localhost/no/such/file"));
-
   RunThreads();
+
   EXPECT_EQ(0, interceptor.GetHitCount());
-  EXPECT_NE(0, error_);
-  EXPECT_EQ(kExpectedContext, crx_context_);
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_EQ(1, num_download_complete_calls_);
+  EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_NE(0, download_complete_result_.error);
+  EXPECT_TRUE(download_complete_result_.response.empty());
 }
 
 // Tests that the fallback to a valid url is successful.
@@ -250,23 +254,25 @@ TEST_F(CrxDownloaderTest, MAYBE_TwoUrls_FirstInvalid) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   std::vector<GURL> urls;
   urls.push_back(GURL("http://localhost/no/such/file"));
   urls.push_back(expected_crx_url);
 
   crx_downloader_->StartDownload(urls);
-
   RunThreads();
+
   EXPECT_EQ(1, interceptor.GetHitCount());
-  EXPECT_EQ(0, error_);
+
+  EXPECT_EQ(1, num_download_complete_calls_);
   EXPECT_EQ(kExpectedContext, crx_context_);
+  EXPECT_EQ(0, download_complete_result_.error);
+  EXPECT_TRUE(ContentsEqual(download_complete_result_.response, test_file));
 
-  EXPECT_EQ(1, num_calls_);
+  EXPECT_TRUE(base::DeleteFile(download_complete_result_.response, false));
 }
 
 // Tests that the download succeeds if the first url is correct and the
@@ -275,23 +281,25 @@ TEST_F(CrxDownloaderTest, TwoUrls_SecondInvalid) {
   const GURL expected_crx_url =
     GURL("http://localhost/download/jebgalgnebhfojomionfpkfelancnnkf.crx");
 
+  const base::FilePath test_file(MakeTestFilePath(kTestFileName));
   GetInterceptor interceptor;
-  interceptor.SetResponse(
-       expected_crx_url,
-       test_file("jebgalgnebhfojomionfpkfelancnnkf.crx"));
+  interceptor.SetResponse(expected_crx_url, test_file);
 
   std::vector<GURL> urls;
   urls.push_back(expected_crx_url);
   urls.push_back(GURL("http://localhost/no…
(message too large)

[Sorin Jianu, 2014-04-26 19:16:12]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-26 19:16:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/40001

[Sorin Jianu, 2014-04-26 19:18:52]

This failed this change depends on a previous change which had landed on trunk
but it did not clear lkgr yet.

[commit-bot: I haz the power, 2014-04-26 19:22:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 19:22:13]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Sorin Jianu, 2014-04-26 19:31:38]

Seems impossible to put this in the CQ until LKGR (now at 266009) clears
r266304, which this CL depends on.
Also, I am learning something today: some bots sync to LKGR and some bots sync
to the tip of the tree.

[Sorin Jianu, 2014-04-28 14:45:45]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:46:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/40001

[commit-bot: I haz the power, 2014-04-28 14:47:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 14:47:14]

Try jobs failed on following builders:
  tryserver.chromium on android_dbg_triggered_tests

[Sorin Jianu, 2014-04-28 15:06:01]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-28 15:07:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/40001

[waffles, 2014-04-28 15:53:55]

lgtm

[commit-bot: I haz the power, 2014-04-28 16:39:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 16:39:25]

Try jobs failed on following builders:
  tryserver.chromium on android_dbg_triggered_tests

[Sorin Jianu, 2014-04-28 18:58:23]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/60001

[commit-bot: I haz the power, 2014-04-28 19:03:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:03:59]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_gn_rel

[Sorin Jianu, 2014-04-28 20:37:24]

The CQ bit was checked by sorin@chromium.org

[commit-bot: I haz the power, 2014-04-28 20:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sorin@chromium.org/254793003/60001

[commit-bot: I haz the power, 2014-04-29 00:54:31]

Change committed as 266722