Simplify and extend commenting in test::ScopedCLDDynamicDataHarness

chrome/browser/translate/translate_browser_test_utils.cc

Index: chrome/browser/translate/translate_browser_test_utils.cc
diff --git a/chrome/browser/translate/translate_browser_test_utils.cc b/chrome/browser/translate/translate_browser_test_utils.cc
index 92494d2ef331c6f23589dfdefafcdbc5f58c1dba..85c4a44e9ebe8afa490e156b6946c9034523ded4 100644
--- a/chrome/browser/translate/translate_browser_test_utils.cc
+++ b/chrome/browser/translate/translate_browser_test_utils.cc
@@ -35,7 +35,7 @@ void GetTestDataSourceDirectory(base::FilePath* out_path) {
 
 #if defined(CLD2_DYNAMIC_MODE) && !defined(CLD2_IS_COMPONENT)
 void GetStandaloneDataFileSource(base::FilePath* out_path) {
-  ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(out_path));

[msw, 2014/05/22 04:26:48]

Why are you removing all the ASSERT_* calls?

[Andrew Hayden (chromium.org), 2014/05/22 10:33:45]

When I worked through your browser test in
https://codereview.chromium.org/292723003/ I had to iterate several times before
realizing that I needed to be using SetUpOnMainThread. As it turns out, any use
of the code prior to this point in the fixture setup is incorrect. The large
lists of failures weren't really helpful. What was helpful was the debug
printing that I added earlier, which revealed that the paths being retrieved by
these methods were empty. That was the underlying cause: the path utilities
hadn't been initialized yet. The return values from these methods were red
herrings and confusing.

The current set of asserts still ensures that the conditions under which I
observed that breakage will cause a test to fail, because copying to or from an
empty path is not supported. The debugging output pinpoints the error more
robustly than seeing that PathService returns false, IMO.

[msw, 2014/05/22 18:16:58]

I'm slightly worried that ignoring PathService::Get failures might let a test
continue, append something to the path, use some unintended directory, and cause
problems that are tough to diagnose without your debug printing. I would
probably leave those two ASSERT_TRUE checks in place, even if at the moment, the
tests happen to fail in another manner when PathService::Get fails.

+  GetTestDataSourceDirectory(out_path);
   *out_path = out_path->Append(FILE_PATH_LITERAL("_platform_specific"))
       .Append(FILE_PATH_LITERAL("all"))
       .Append(chrome::kCLDDataFilename);
@@ -45,7 +45,7 @@ void GetStandaloneDataFileSource(base::FilePath* out_path) {
 // important to test isolation. Each test gets its own USER_DATA_DIR, which
 // ensures proper isolation between test processes running in parallel.
 void GetStandaloneDataFileDestination(base::FilePath* out_path) {
-  ASSERT_TRUE(PathService::Get(chrome::DIR_USER_DATA, out_path));
+  PathService::Get(chrome::DIR_USER_DATA, out_path);
   *out_path = out_path->Append(chrome::kCLDDataFilename);
 }
 
@@ -59,11 +59,11 @@ void DeleteStandaloneDataFile() {
 void CopyStandaloneDataFile() {
   DeleteStandaloneDataFile();  // sanity: blow away any old copies.
   base::FilePath target_file;
-  ASSERT_NO_FATAL_FAILURE(GetStandaloneDataFileDestination(&target_file));
+  GetStandaloneDataFileDestination(&target_file);
   base::FilePath target_dir = target_file.DirName();
   ASSERT_TRUE(base::CreateDirectoryAndGetError(target_dir, NULL));
   base::FilePath source_file;
-  ASSERT_NO_FATAL_FAILURE(GetStandaloneDataFileSource(&source_file));
+  GetStandaloneDataFileSource(&source_file);
   DLOG(INFO) << "Copying CLD test data file from " << source_file.value()
              << " to " << target_file.value();
   ASSERT_TRUE(base::CopyFile(source_file, target_file));
@@ -76,11 +76,11 @@ void CopyStandaloneDataFile() {
 // like GetStandaloneDataFileDestination, this is safe to run in multiple
 // parallel test processes.
 void GetExtractedComponentDestination(base::FilePath* out_path) {
-  ASSERT_TRUE(PathService::Get(chrome::DIR_COMPONENT_CLD2, out_path));
+  PathService::Get(chrome::DIR_COMPONENT_CLD2, out_path);
 }
 
 void GetComponentDataFileDestination(base::FilePath* out_path) {
-  ASSERT_NO_FATAL_FAILURE(GetExtractedComponentDestination(out_path));
+  GetExtractedComponentDestination(out_path);
   *out_path = out_path->Append(kCrxVersion)
       .Append(FILE_PATH_LITERAL("_platform_specific"))
       .Append(FILE_PATH_LITERAL("all"))
@@ -89,7 +89,7 @@ void GetComponentDataFileDestination(base::FilePath* out_path) {
 
 void DeleteComponentTree() {
   base::FilePath tree_path;
-  ASSERT_NO_FATAL_FAILURE(GetExtractedComponentDestination(&tree_path));
+  GetExtractedComponentDestination(&tree_path);
   DLOG(INFO) << "Deleting CLD component test files from " << tree_path.value();
   base::DeleteFile(tree_path, true);
 }
@@ -97,16 +97,16 @@ void DeleteComponentTree() {
 void CopyComponentTree() {
   DeleteComponentTree();  // sanity: blow away any old copies.
   base::FilePath target_dir;
-  ASSERT_NO_FATAL_FAILURE(GetExtractedComponentDestination(&target_dir));
+  GetExtractedComponentDestination(&target_dir);
   base::FilePath source_dir;
+  GetTestDataSourceDirectory(&source_dir);
   DLOG(INFO) << "Copying CLD component test files from " << source_dir.value()
              << " to " << target_dir.value();
-  ASSERT_NO_FATAL_FAILURE(GetTestDataSourceDirectory(&source_dir));
   ASSERT_TRUE(base::CreateDirectoryAndGetError(target_dir, NULL));
   ASSERT_TRUE(base::CopyDirectory(source_dir, target_dir, true));
   ASSERT_TRUE(base::PathExists(target_dir));
   base::FilePath check_path;
-  ASSERT_NO_FATAL_FAILURE(GetComponentDataFileDestination(&check_path));
+  GetComponentDataFileDestination(&check_path);
   ASSERT_TRUE(base::PathExists(check_path));
 }
 #endif  // defined(CLD2_DYNAMIC_MODE) && defined(CLD2_IS_COMPONENT)
@@ -139,7 +139,7 @@ void ScopedCLDDynamicDataHarness::Init() {
   // Dynamic data mode is enabled and we are using the component updater.
   ASSERT_NO_FATAL_FAILURE(CopyComponentTree());
   base::FilePath data_file;
-  ASSERT_NO_FATAL_FAILURE(GetComponentDataFileDestination(&data_file));
+  GetComponentDataFileDestination(&data_file);
   component_updater::CldComponentInstallerTraits::SetLatestCldDataFile(
       data_file);
   base::FilePath result = component_updater::GetLatestCldDataFile();