Break test cases on inner asserts.

chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc

 // Copyright (c) 2013 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.
 
 // Browser test for basic Chrome OS file manager functionality:
 //  - The file list is updated when a file is added externally to the Downloads
 //    folder.
 //  - Selecting a file and copy-pasting it with the keyboard copies the file.
 //  - Selecting a file and pressing delete deletes it.
 
@@ skipping 121 matching lines @@
 }
 
 void TestFilePathWatcher::StartWatching() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
 
   watcher_.reset(new base::FilePathWatcher);
   bool ok = watcher_->Watch(
       path_, false /*recursive*/,
       base::Bind(&TestFilePathWatcher::FilePathWatcherCallback,
                  base::Unretained(this)));
-  ASSERT_TRUE(ok);
+  DCHECK(ok);
 
   // If the condition was already met before FilePathWatcher was launched,
   // FilePathWatcher won't be able to detect a change, so check the condition
   // here.
   if (condition_.Run(path_)) {

[hashimoto, 2013/05/23 06:45:09]

How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of DCHECK?

[mtomasz, 2013/05/23 07:07:32]

(1) Can't we just do failed_ = !watcher->Watch(...);
(2) If we do so, we will not be able to distinguish an internal error with
setting a watcher from an error, that a file did not get copied/deleted/etc.

WDYT?

[hashimoto, 2013/05/23 07:17:31]

It also looks good.
I think we don't want to know the difference between errors on
FilePathWatcher::Watch and on FilePathWatcherCallback, they are reporting the
same thing.
If you look at the implementation of base::FilePathWatcher in
file_path_watcher_linux.cc and file_path_watcher_win.cc, you can see that
|failed| passed to FilePathWatcherCallback and the boolean return value of
Watch() are coming from the same function UpdateWatch/UpdateWatches.

[mtomasz, 2013/05/23 07:43:42]

There are some edge cases they don't report the same especially in
file_path_watcher_kqueue.cc, but I am not sure if we use it anywhere.

Anyway, should be safe. I've changed to failed_ = !Watch(...); Done.

     watcher_.reset();
     content::BrowserThread::PostTask(content::BrowserThread::UI,
                                      FROM_HERE,
                                      quit_closure_);
     return;
   }
 }
 
 void TestFilePathWatcher::FilePathWatcherCallback(const base::FilePath& path,
                                                   bool failed) {
@@ skipping 111 matching lines @@
       CreateFile(entry.source_file_name, entry.target_name,
                  entry.last_modified_time_as_string);
     } else {
       NOTREACHED();
     }
   }
 
   void CreateFile(const std::string& source_file_name,
                   const std::string& target_name,
                   const std::string& modification_time) {
-
     std::string content_data;
     base::FilePath test_file_path =
         google_apis::test_util::GetTestFilePath("chromeos/file_manager").
             AppendASCII(source_file_name);
 
     base::FilePath path = local_path_.AppendASCII(target_name);
     ASSERT_TRUE(file_util::PathExists(test_file_path))
         << "Test file doesn't exist: " << test_file_path.value();
     ASSERT_TRUE(file_util::CopyFile(test_file_path, path));
     ASSERT_TRUE(file_util::PathExists(path))
@@ skipping 478 matching lines @@
   DriveTestVolume drive_volume_;
 };
 
 // FileManagerBrowserTransferTest depends on Drive and Drive is not supported in
 // the guest mode.
 INSTANTIATE_TEST_CASE_P(InNonGuestMode,
                         FileManagerBrowserTransferTest,
                         ::testing::Values(false));
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserLocalTest, TestFileDisplay) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   DoTestFileDisplay(&volume_);
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestKeyboardCopy) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   DoTestKeyboardCopy(&volume_);
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestKeyboardDelete) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   DoTestKeyboardDelete(&volume_);
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestOpenRecent) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("openSidebarRecent");
+  ASSERT_NO_FATAL_FAILURE(StartTest("openSidebarRecent"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 // TODO(hirono): Bring back the offline feature. http://crbug.com/238545
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, DISABLED_TestOpenOffline) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("openSidebarOffline");
+  ASSERT_NO_FATAL_FAILURE(StartTest("openSidebarOffline"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestOpenSharedWithMe) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("openSidebarSharedWithMe");
+  ASSERT_NO_FATAL_FAILURE(StartTest("openSidebarSharedWithMe"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserDriveTest, TestAutocomplete) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("autocomplete");
+  ASSERT_NO_FATAL_FAILURE(StartTest("autocomplete"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromDriveToDownloads) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromDriveToDownloads");
+  ASSERT_NO_FATAL_FAILURE(
+      StartTest("transferFromDriveToDownloads"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromDownloadsToDrive) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromDownloadsToDrive");
+  ASSERT_NO_FATAL_FAILURE(
+      StartTest("transferFromDownloadsToDrive"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromSharedToDownloads) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
   StartTest("transferFromSharedToDownloads");

[hirono, 2013/05/23 06:40:46]

Could you add ASSERT_NO_FATAL_FAILURE here?

[mtomasz, 2013/05/23 07:07:32]

Done.

   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromSharedToDrive) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromSharedToDrive");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromSharedToDrive"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromRecentToDownloads) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromRecentToDownloads");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromRecentToDownloads"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        TransferFromRecentToDrive) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
   StartTest("transferFromRecentToDrive");

[hirono, 2013/05/23 06:38:44]

Could you add ASSERT_NO_FATAL_FAILURE here?

[mtomasz, 2013/05/23 07:07:32]

Done.

   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 // TODO(hirono): Bring back the offline feature. http://crbug.com/238545
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        DISABLED_TransferFromOfflineToDownloads) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromOfflineToDownloads");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromOfflineToDownloads"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 // TODO(hirono): Bring back the offline feature. http://crbug.com/238545
 IN_PROC_BROWSER_TEST_P(FileManagerBrowserTransferTest,
                        DISABLED_TransferFromOfflineToDrive) {
-  PrepareVolume();
+  ASSERT_NO_FATAL_FAILURE(PrepareVolume());
   ResultCatcher catcher;
-  StartTest("transferFromOfflineToDrive");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromOfflineToDrive"));
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 }  // namespace