Introduce ScopedCommandLine

chrome/browser/extensions/activity_log/counting_policy_unittest.cc

 // Copyright 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.
 
 #include "chrome/browser/extensions/activity_log/counting_policy.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/cancelable_callback.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/command_line_test_util.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/test_timeouts.h"
 #include "base/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/common/extension_builder.h"
 #include "sql/statement.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/login/users/scoped_test_user_manager.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #endif
 
 using content::BrowserThread;
 
 namespace extensions {
 
 class CountingPolicyTest : public testing::Test {
  public:
   CountingPolicyTest()
-      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-        saved_cmdline_(base::CommandLine::NO_PROGRAM) {
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {
 #if defined OS_CHROMEOS
     test_user_manager_.reset(new chromeos::ScopedTestUserManager());
 #endif
+    profile_.reset(new TestingProfile());
+    scoped_command_line_.GetProcessCommandLine()->
+        AppendSwitch(switches::kEnableExtensionActivityLogging);
     base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
-    saved_cmdline_ = *base::CommandLine::ForCurrentProcess();
-    profile_.reset(new TestingProfile());
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableExtensionActivityLogging);
     extension_service_ = static_cast<TestExtensionSystem*>(
         ExtensionSystem::Get(profile_.get()))->CreateExtensionService
             (&command_line, base::FilePath(), false);
   }
 
   ~CountingPolicyTest() override {
 #if defined OS_CHROMEOS
     test_user_manager_.reset();
 #endif
     base::RunLoop().RunUntilIdle();
     profile_.reset(NULL);
     base::RunLoop().RunUntilIdle();
-    // Restore the original command line and undo the affects of SetUp().
-    *base::CommandLine::ForCurrentProcess() = saved_cmdline_;
   }
 
   // Wait for the task queue for the specified thread to empty.
   void WaitOnThread(const BrowserThread::ID& thread) {
     BrowserThread::PostTaskAndReply(
         thread, FROM_HERE, base::Bind(&base::DoNothing),
         base::MessageLoop::current()->QuitWhenIdleClosure());
     base::MessageLoop::current()->Run();
   }
 
@@ skipping 434 matching lines @@
                 "Google1",
                 "http://www.args-url1.com/",
                 2);
     ASSERT_EQ(1, actions->at(0)->action_id());
   }
 
  protected:
   ExtensionService* extension_service_;
   std::unique_ptr<TestingProfile> profile_;
   content::TestBrowserThreadBundle thread_bundle_;
-  // Used to preserve a copy of the original command line.

[Devlin, 2016/04/22 17:22:33]

I don't understand this comment (there's no TearDown method here).  Is it still
valid?  Or can we just rely on the test framework to reset the commandline after
all?

[robliao, 2016/04/22 19:28:30]

Looks like CountingPolicyTest never had a TearDown method so this comment never
really applied.

The test infrastructure, however, does clean up the command line for you via
TestClientInitializer::OnTestEnd, which leads to the following philosophical
discussion:

Most Chromium devs know that the CommandLine belongs to global state. They also
know that whatever you change in SetUp you generally clean up in TearDown.

I suspect most Chromium devs (including myself until digging into test_suite.cc)
do not know that the Command Line is handled specially by the testing framework
to automatically restore at the end of a test (as opposed to test case which
groups all tests together).

Do we want developers to clean up global state that they changed or should they
know to rely on the testing infrastructure to clean this specific bit up? [The
impact of this is a lot of CommandLine state saving will naturally go away if it
covers the scope of a single test]

PRO for self contained cleanup: It's trivially obvious to see that the test
restores state.
CON for self contained cleanup: It's redundant code.

PRO for omitting the cleanup: The infrastructure will clean it up.
CON for omitting the cleanup: It's not obvious that it will do that.

I'm leaning towards self-contained cleanup as a reviewing sustainability note
and at the same time keeping the test infrastructure cleanup for robustness. In
another approach, the test infrastructure could instead DCHECK when cleanup did
not occur.

[Devlin, 2016/04/22 20:35:21]

Personally, I'm in favor of just having the test infrastructure clean this up -
it's a lot easier, and it's hard to track all the places we might modify a
command line in the course of running a test (since it's not necessarily
confined to the test code itself).  In other words, I think we need to have the
infrastructure clean up the commandline anyway, so I'd rather not add extra work
for people writing tests if we don't have to.

As to people not being aware of it, I thought most people did know this, but
perhaps not.  Maybe though another FYI to chromium-dev and/or updating docs if
they don't reflect this?

In either case, that's not to say that this change isn't worthwhile - there are
tests that need to modify, restore, and modify again, or pass around copies for
mutation, etc.  I just think in cases like this particular one, it might be best
to say "test infrastructure cleans up".

[danakj, 2016/04/22 20:37:52]

Which infrastructure? TestSuite could do it but that's per suite not per test. I
think you might have to change gtest to do this, but maybe someone can point out
a way.

[robliao, 2016/04/22 20:41:19]

The current infrastructure is TestClientInitializer, which gets added to the
Test event listeners and is notified on ever test start and test end. 

https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sui...

Added to the listener chain here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sui...

I do recall getting burned by this when I had a test in gfx_unittests and
restoring the command line fixed things up.

-  // The test framework will do this itself as well. However, by then,
-  // it is too late to call ActivityLog::RecomputeLoggingIsEnabled() in
-  // TearDown().
-  base::CommandLine saved_cmdline_;
+  base::test::ScopedCommandLine scoped_command_line_;
 
 #if defined OS_CHROMEOS
   chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
   chromeos::ScopedTestCrosSettings test_cros_settings_;
   std::unique_ptr<chromeos::ScopedTestUserManager> test_user_manager_;
 #endif
 };
 
 TEST_F(CountingPolicyTest, Construct) {
   ActivityLogDatabasePolicy* policy = new CountingPolicy(profile_.get());
@@ skipping 877 matching lines @@
 
   action_ids.push_back(2);
   CheckRemoveActions(
       policy, action_ids, base::Bind(&CountingPolicyTest::Action2Deleted));
   action_ids.clear();
 
   policy->Close();
 }
 
 }  // namespace extensions