Break test cases on inner asserts.

Break test cases on inner asserts.

We were calling asserts withing a subroutines. In that case, asserts breaks the current subroutine but not the test case. This patch wraps calls to subroutines with ASSERT_NO_FATAL_FAILURE, which checks if it failed on any asserts, and if so, then terminates the test case.

TEST=browser_tests
BUG=235334
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202010

[mtomasz, 2013-05-23 06:32:31]

@hashimoto, @hirono: PTAL.

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

Thank you for addressing this!

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:866:
StartTest("transferFromRecentToDrive");
Could you add ASSERT_NO_FATAL_FAILURE here?

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

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:842:
StartTest("transferFromSharedToDownloads");
Could you add ASSERT_NO_FATAL_FAILURE here?

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

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:147:
if (condition_.Run(path_)) {
How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of DCHECK?

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

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:147:
if (condition_.Run(path_)) {
On 2013/05/23 06:45:09, hashimoto wrote:
> How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of
DCHECK?

(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?

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:842:
StartTest("transferFromSharedToDownloads");
On 2013/05/23 06:40:46, hirono wrote:
> Could you add ASSERT_NO_FATAL_FAILURE here?

Done.

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:866:
StartTest("transferFromRecentToDrive");
On 2013/05/23 06:38:44, hirono wrote:
> Could you add ASSERT_NO_FATAL_FAILURE here?

Done.

[hirono, 2013-05-23 07:11:32]

LGTM after solving the problem pointed by Hashimoto-san.

On 2013/05/23 07:07:32, mtomasz wrote:
>
https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
> File
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
>
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:147:
> if (condition_.Run(path_)) {
> On 2013/05/23 06:45:09, hashimoto wrote:
> > How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of
> DCHECK?
> 
> (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?
> 
>
https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
>
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:842:
> StartTest("transferFromSharedToDownloads");
> On 2013/05/23 06:40:46, hirono wrote:
> > Could you add ASSERT_NO_FATAL_FAILURE here?
> 
> Done.
> 
>
https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
>
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:866:
> StartTest("transferFromRecentToDrive");
> On 2013/05/23 06:38:44, hirono wrote:
> > Could you add ASSERT_NO_FATAL_FAILURE here?
> 
> Done.

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

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:147:
if (condition_.Run(path_)) {
On 2013/05/23 07:07:32, mtomasz wrote:
> On 2013/05/23 06:45:09, hashimoto wrote:
> > How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of
> DCHECK?
> 
> (1) Can't we just do failed_ = !watcher->Watch(...);
It also looks good.
> (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?
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:41]

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/15771005/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:147:
if (condition_.Run(path_)) {
On 2013/05/23 07:17:31, hashimoto wrote:
> On 2013/05/23 07:07:32, mtomasz wrote:
> > On 2013/05/23 06:45:09, hashimoto wrote:
> > > How about "if (!ok || condition_.Run(path_)) { failed_ = !ok" instead of
> > DCHECK?
> > 
> > (1) Can't we just do failed_ = !watcher->Watch(...);
> It also looks good.
> > (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?
> 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.

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.

[hashimoto, 2013-05-23 07:57:11]

lgtm thanks

[commit-bot: I haz the power, 2013-05-23 07:58:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15771005/13001

[commit-bot: I haz the power, 2013-05-23 19:50:16]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
  Hunk #3 succeeded at 632 (offset 3 lines).
  Hunk #4 succeeded at 662 (offset 9 lines).
  Hunk #5 succeeded at 682 (offset 9 lines).
  Hunk #6 FAILED at 774.
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc.rej

Patch:      
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
Index:
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
diff --git
a/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
b/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
index
d06a676c700b04925a37e82e999f58b1de328533..79fcbbd1d15d359a246b68367753b8264942c24b
100644
---
a/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
+++
b/chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
@@ -135,16 +135,16 @@ void TestFilePathWatcher::StartWatching() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
 
   watcher_.reset(new base::FilePathWatcher);
-  bool ok = watcher_->Watch(
+  failed_ = !watcher_->Watch(
       path_, false /*recursive*/,
       base::Bind(&TestFilePathWatcher::FilePathWatcherCallback,
                  base::Unretained(this)));
-  ASSERT_TRUE(ok);
 
-  // If the condition was already met before FilePathWatcher was launched,
+  // If failed to start the watcher, then quit the message loop immediately.
+  // Also, 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_)) {
+  if (failed_ || condition_.Run(path_)) {
     watcher_.reset();
     content::BrowserThread::PostTask(content::BrowserThread::UI,
                                      FROM_HERE,
@@ -276,7 +276,6 @@ class LocalTestVolume : public TestVolume {
   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").
@@ -630,7 +629,7 @@ void FileManagerBrowserTestBase::CreateTestEntries(
 
 void FileManagerBrowserTestBase::DoTestFileDisplay(TestVolume* volume) {
   ResultCatcher catcher;
-  StartTest("fileDisplay" + volume->GetName());
+  ASSERT_NO_FATAL_FAILURE(StartTest("fileDisplay" + volume->GetName()));
 
   ExtensionTestMessageListener listener("initial check done", true);
   ASSERT_TRUE(listener.WaitUntilSatisfied());
@@ -654,7 +653,7 @@ void
FileManagerBrowserTestBase::DoTestKeyboardCopy(TestVolume* volume) {
   ASSERT_FALSE(volume->PathExists(copy_path));
 
   ResultCatcher catcher;
-  StartTest("keyboardCopy" + volume->GetName());
+  ASSERT_NO_FATAL_FAILURE(StartTest("keyboardCopy" + volume->GetName()));
 
   const int64 kKeyboardTestFileSize = 59943;
 
@@ -674,7 +673,7 @@ void
FileManagerBrowserTestBase::DoTestKeyboardDelete(TestVolume* volume) {
   ASSERT_TRUE(volume->PathExists(delete_path));
 
   ResultCatcher catcher;
-  StartTest("keyboardDelete" + volume->GetName());
+  ASSERT_NO_FATAL_FAILURE(StartTest("keyboardDelete" + volume->GetName()));
 
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
   ASSERT_TRUE(volume->WaitUntilFileNotPresent(delete_path));
@@ -775,112 +774,114 @@ INSTANTIATE_TEST_CASE_P(InNonGuestMode,
                         ::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");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromSharedToDownloads"));
   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");
+  ASSERT_NO_FATAL_FAILURE(StartTest("transferFromRecentToDrive"));
   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

[commit-bot: I haz the power, 2013-05-24 03:21:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15771005/33001

[commit-bot: I haz the power, 2013-05-24 07:37:58]

Change committed as 202010