Simplify and extend commenting in test::ScopedCLDDynamicDataHarness

chrome/browser/translate/translate_browser_test_utils.h

 // Copyright 2014 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.
 
 #ifndef CHROME_BROWSER_TRANSLATE_TRANSLATE_BROWSER_TEST_UTILS_H_
 #define CHROME_BROWSER_TRANSLATE_TRANSLATE_BROWSER_TEST_UTILS_H_
 
 #include "base/macros.h"
 
 namespace test {
 
 // A utility class that sets up CLD dynamic data upon calling Init() and cleans
 // it up when destroyed.
+// NB: Test data lives under: src/chrome/test/data/cld2_component

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

nit: what does "NB:" itself add to this comment? It's a pet peeve of mine :)

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

Funny thing, I first encountered "NB" about 3 years ago... somehow I managed to
avoid seeing it for about 10 years prior, the it started showing up. I hated it
initially, but it has slowly crept into my lexicon. I agree, it should die.
Removed.

 //
 // This class is intended to be instantiated within IN_PROC_BROWSER_TEST_F
 // test fixtures; it uses ASSERT macros for correctness, so that tests will
 // fail gracefully in error conditions. Sample use:
 //
-//   #include "chrome/browser/translate/translate_browser_test_utils.h"
-//
 //   IN_PROC_BROWSER_TEST_F(BrowserTest, PageLanguageDetection) {
 //     test::ScopedCLDDynamicDataHarness dynamic_data_scope;
 //     ASSERT_NO_FATAL_FAILURE(dynamic_data_scope.Init());
 //     // ... your code that depends on language detection goes here
 //   }
 //
 // If you have a lot of tests that need language translation features, you can
 // add an instance of the ScopedCLDDynamicDataHarness to your test class'
-// private member variables and add the call to Init() into your Setup method.
+// private member variables and add the call to Init() into SetUpOnMainThread.

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

I'm not sure the example here adds much, in fact I'd do away with it and the
example above altogether. As long as the pattern is demonstrated by existing use
in the codebase, it'll tend to be followed (copied) elsewhere.

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

That's fair, I suppose. "git gs ScopedCLD" would get you what you need. I think
the first example is still important, because it illustrates the use of
ASSERT_NO_FATAL_FAILURE, which is useful.

+// Sample use:
 //
-// NB: Test data lives under src/chrome/test/data/cld2_component
+//   class MyTestClass : public InProcessBrowserTest {
+//    public:

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

nit: remove lines not directly related to init; I'd axe lines 32-34 and 41-46.
(maybe add ellipses lines if you think readers would be confused otherwise)

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

Done.

+//     virtual MyTestClass() {}
+//     virtual ~MyTestclass() {}
+//     virtual void SetUpOnMainThread() OVERRIDE {
+//       dynamic_data_scope.Init();

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

If this doesn't need to ASSERT_NO_FATAL_FAILURE(...) as you note elsewhere,
perhaps those other comments should be qualified or removed?

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

Thorny question. It doesn't make sense to ASSERT_NO_FATAL_FAILURE here because
doing so will cause the test not to run, because SetUpOnMainThread needs to be
called. If there IS a fatal failure, the test itself will fail because that
error will have been recorded in the same thread. You could call
HasFatalFailure() at the start of every test, but that would defeat much of the
good of moving this into the setup code to begin with.

So: In general, the assert is useful... but not here.

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

Thanks for clarifying, I suppose what you have is the best we can do for now.

+//       InProcessBrowserTest::SetUpOnMainThread();
+//     }
+//    private:
+//     test::ScopedCLDDynamicDataHarness dynamic_data_scope;
+//     DISALLOW_COPY_AND_ASSIGN(MyTestClass);
+//   };
+//
+//  IN_PROC_BROWSER_TEST_F(MyTestClass, MyTest) {
+//    (your code here)
+//  }
 class ScopedCLDDynamicDataHarness {
  public:
   // Constructs the object, but does nothing. Call Init() to prepare the
   // harness, and enclose that call in ASSERT_NO_FATAL_FAILURE(...).
   ScopedCLDDynamicDataHarness();
 
   // Reverses the work done by the constructor: any files and/or directories
   // that would be created by the constructor are immediately and irrevocably
   // deleted.
   // If dynamic data is not currently available for any reason, this method has
   // no net effect on the runtime.
   ~ScopedCLDDynamicDataHarness();
 
   // Call this method, wrapping it in ASSERT_NO_FATAL_FAILURE, to initialize
   // the harness and trigger test failure of initialization fails.
   void Init();
 
  private:
   void ClearStandaloneDataFileState();
 
   DISALLOW_COPY_AND_ASSIGN(ScopedCLDDynamicDataHarness);
 };
 
 }  // namespace test
 
 #endif  // CHROME_BROWSER_TRANSLATE_TRANSLATE_BROWSER_TEST_UTILS_H_