chrome/browser/extensions/activity_log/counting_policy_unittest.cc - Issue 1908633002: Introduce ScopedCommandLine

Unified Diff: chrome/browser/extensions/activity_log/counting_policy_unittest.cc

Issue 1908633002: Introduce ScopedCommandLine (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Update CrOS Unit Test Created 4 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« base/test/command_line_test_util.h ('K') | « chrome/browser/extensions/activity_log/activity_log_enabled_unittest.cc ('k') | chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/extensions/activity_log/counting_policy_unittest.cc

diff --git a/chrome/browser/extensions/activity_log/counting_policy_unittest.cc b/chrome/browser/extensions/activity_log/counting_policy_unittest.cc

index 8b08a4abf519791ef0503dcaef8133ffc4b31d36..b0aa2dee085dd039d470bf3b6311a5a455cae49e 100644

--- a/chrome/browser/extensions/activity_log/counting_policy_unittest.cc

+++ b/chrome/browser/extensions/activity_log/counting_policy_unittest.cc

@@ -19,6 +19,7 @@

#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"

@@ -48,16 +49,14 @@ 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

- base::CommandLine command_line(base::CommandLine::NO_PROGRAM);

- saved_cmdline_ = *base::CommandLine::ForCurrentProcess();

profile_.reset(new TestingProfile());

- base::CommandLine::ForCurrentProcess()->AppendSwitch(

- switches::kEnableExtensionActivityLogging);

+ scoped_command_line_.GetProcessCommandLine()->

+ AppendSwitch(switches::kEnableExtensionActivityLogging);

+ base::CommandLine command_line(base::CommandLine::NO_PROGRAM);

extension_service_ = static_cast<TestExtensionSystem*>(

ExtensionSystem::Get(profile_.get()))->CreateExtensionService

(&command_line, base::FilePath(), false);

@@ -70,8 +69,6 @@ class CountingPolicyTest : public testing::Test {

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.

@@ -526,11 +523,7 @@ class CountingPolicyTest : public testing::Test {

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 TearDo

robliao 2016/04/22 19:28:30 Looks like CountingPolicyTest never had a TearDown

Devlin 2016/04/22 20:35:21 Personally, I'm in favor of just having the test i

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 th

On 2016/04/22 20:35:21, Devlin wrote: > On 2016/04/22 19:28:30, robliao wrote: > > On 2016/04/22 17:22:33, Devlin wrote: > > > 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? > > > > 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. > > 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".

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 TestClientInitialize

On 2016/04/22 20:37:52, danakj wrote: > On 2016/04/22 20:35:21, Devlin wrote: > > On 2016/04/22 19:28:30, robliao wrote: > > > On 2016/04/22 17:22:33, Devlin wrote: > > > > 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? > > > > > > 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. > > > > 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". > > 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.

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_;