Fix SavePageAsMHTMLBrowserTest

chrome/browser/download/save_page_browsertest.cc

Index: chrome/browser/download/save_page_browsertest.cc
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 096dcb27322a71a1144582760a1533697d0f0fea..aeee2c78f04481b5b2c74e24f617ccc1300e8d37 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -155,12 +155,14 @@ class SavePageBrowserTest : public InProcessBrowserTest {
     }
 
     bool operator() (const DownloadPersistentStoreInfo& info) const {
-      return info.url == url_ &&
-        info.path == path_ &&
-        // For save packages, received bytes is actually the number of files.
-        info.received_bytes == num_files_ &&
-        info.total_bytes == 0 &&
-        info.state == DownloadItem::COMPLETE;
+      return ((info.url == url_) &&

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

My personal feeling is that parens around the == is overkill, but it's up to
you.

[benjhayden, 2012/05/14 19:27:37]

My personal feeling is that ungrouped conditions are living dangerously, but
Done.

+              (info.path == path_) &&
+              // For non-MHTML save packages, received_bytes is actually the
+              // number of files.

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

Instead of just putting in an off switch, can you replumb this path so that
info.received_bytes is called something like "progress_count" and then pass in
either the expected number of files or the expected number of bytes depending on
the test?  

[benjhayden, 2012/05/14 19:27:37]

Renaming the variable won't fix the problem.
The problem is that SavePageAsMHTML doesn't know the expected number of bytes.
It could be 2759 or it could be 2760. It probably won't be a different number
until the year 10,000, but I'm not sure.
I could make it calculate the expected number of bytes based on the date, but
that seems a little silly. Or perhaps it just highlights the existing silliness.
I'd like to be able to set a flag to disable the date header in the mhtml
generator (I did something like this in video), but that's probably an
"abstraction violation".
I'd like to parse the generated mhtml and expect some of the headers and a hash
of the body, but that seems too heavy. I did something like this in video
("video_digest"), but it was many-purposed, whereas such a parser here would be
for this one test.

+              ((num_files_ < 0) ||
+               (info.received_bytes == num_files_)) &&
+              (info.total_bytes == 0) &&
+              (info.state == DownloadItem::COMPLETE));
     }
 
     GURL url_;
@@ -388,9 +390,7 @@ class SavePageAsMHTMLBrowserTest : public SavePageBrowserTest {
 SavePageAsMHTMLBrowserTest::~SavePageAsMHTMLBrowserTest() {
 }
 
-// Bug 127527: This test fails when the day of the month is >9.
-IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, DISABLED_SavePageAsMHTML) {
-  static const int64 kFileSize = 2759;
+IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) {
   GURL url = NavigateToMockURL("b");
   FilePath download_dir = DownloadPrefs::FromDownloadManager(
       GetDownloadManager())->download_path();
@@ -406,10 +406,10 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, DISABLED_SavePageAsMHTML) {
         content::NotificationService::AllSources());
   browser()->SavePage();
   observer.Wait();
-  CheckDownloadHistory(url, full_file_name, kFileSize);
+  CheckDownloadHistory(url, full_file_name, -1);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   int64 actual_file_size = -1;
   EXPECT_TRUE(file_util::GetFileSize(full_file_name, &actual_file_size));
-  EXPECT_EQ(kFileSize, actual_file_size);
+  EXPECT_LE(2758, actual_file_size);

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

I dislike this type of naked constant.  Is there some way to derive this value
(probe the file system for the files we're actually saving)?  If not, I do think
we should keep it as a defined constant.

[benjhayden, 2012/05/14 19:27:37]

Done.

 }