Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1908633002:
Introduce ScopedCommandLine (Closed)
|
Created:
4 years, 8 months ago by robliao Modified:
4 years, 7 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, felt, oshima Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ScopedCommandLine
A few tests other there have the need to manipulate the command line and
restore it after the test. ScopedCommandLine provides an easy way to
set the options you need and then restore them when the ScopedCommandLine
is out of scope.
This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer)
BUG=604961
Committed: https://crrev.com/48e31159a123a0f7e3f3e45b7e28a136408ddf3a
Cr-Commit-Position: refs/heads/master@{#390741}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Update CrOS Unit Test #
Total comments: 7
Patch Set 3 : Rename File #Patch Set 4 : Clean Up Other Tests #Messages
Total messages: 73 (29 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/20001
robliao@chromium.org changed reviewers: + phajdan.jr@chromium.org
phajdan.jr: Please review this CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Just a small comment. https://codereview.chromium.org/1908633002/diff/20001/base/test/command_line_... File base/test/command_line_test_util.cc (right): https://codereview.chromium.org/1908633002/diff/20001/base/test/command_line_... base/test/command_line_test_util.cc:10: ScopedCommandLine::ScopedCommandLine() We have a very similar class in base/test/test_suite.cc (TestClientInitializer). Could you deduplicate that? Other possible unit test candidates to use this (found using "git grep 'CommandLine::ForCurrentProcess() ='"): chrome/browser/chromeos/events/event_rewriter_unittest.cc chrome/browser/extensions/api/activity_log_private/activity_log_private_apitest.cc
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
https://codereview.chromium.org/1908633002/diff/20001/base/test/command_line_... File base/test/command_line_test_util.cc (right): https://codereview.chromium.org/1908633002/diff/20001/base/test/command_line_... base/test/command_line_test_util.cc:10: ScopedCommandLine::ScopedCommandLine() On 2016/04/21 11:55:50, Paweł Hajdan Jr. wrote: > We have a very similar class in base/test/test_suite.cc (TestClientInitializer). > > Could you deduplicate that? > > Other possible unit test candidates to use this (found using "git grep > 'CommandLine::ForCurrentProcess() ='"): > chrome/browser/chromeos/events/event_rewriter_unittest.cc > chrome/browser/extensions/api/activity_log_private/activity_log_private_apitest.cc I wanted to hold off on chrome/browser/extensions/api/activity_log_private/activity_log_private_apitest.cc since it's not a trivial remove + substitute. There's actually mutation and a strange (possibly erroneous) interaction where the command line gets stored off via a utility function. I'll need to do a bit of tracing to figure out what's going on here. event_rewriter_unittest is more trivial, so I've added that in.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please also deduplicate with base/test/test_suite.cc (read my last comment carefully, it has more details what needs to be done).
On 2016/04/21 20:55:55, Paweł Hajdan Jr. wrote: > Please also deduplicate with base/test/test_suite.cc (read my last comment > carefully, it has more details what needs to be done). Similarly, TestSuite has a non-contained treatment of command_line (TestClientInitializer doesn't actually change the command line itself). I would like to perform that change later.
LGTM
The CQ bit was checked by robliao@chromium.org
The CQ bit was unchecked by robliao@chromium.org
robliao@chromium.org changed reviewers: + danakj@chromium.org, oshima@chromium.org, rdevlin.cronin@chromium.org
OWNERs, please review changes under the path patterns under your email. Thanks! danakj@chromium.org: base/base.gyp rdevlin.cronin@chromium.org: c/b/extensions/* extensions/* oshima@chromium.org: chrome/browser/chromeos/*
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used to preserve a copy of the original command line. 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?
https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_... File base/test/command_line_test_util.h (right): https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_... base/test/command_line_test_util.h:13: class ScopedCommandLine final { Name this file after the class.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Patchset #3 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/40001
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used to preserve a copy of the original command line. 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.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/100001
https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_... File base/test/command_line_test_util.h (right): https://codereview.chromium.org/1908633002/diff/40001/base/test/command_line_... base/test/command_line_test_util.h:13: class ScopedCommandLine final { On 2016/04/22 18:57:10, danakj wrote: > Name this file after the class. Done.
base LGTM
https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used to preserve a copy of the original command line. 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".
https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used to preserve a copy of the original command line. 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.
https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used to preserve a copy of the original command line. 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.
On 2016/04/22 20:41:19, robliao wrote: > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (left): > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: // Used > to preserve a copy of the original command line. > 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. gfx_unittests does perform the cleanup, so maybe I'm misremembering here.
On Fri, Apr 22, 2016 at 1:48 PM, <robliao@chromium.org> wrote: > On 2016/04/22 20:41:19, robliao wrote: > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > (left): > > > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: > // > Used > > to preserve a copy of the original command line. > > 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. > > gfx_unittests does perform the cleanup, so maybe I'm misremembering here. > Are the tests in this CL not cleaning up? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/22 21:05:19, danakj wrote: > On Fri, Apr 22, 2016 at 1:48 PM, <mailto:robliao@chromium.org> wrote: > > > On 2016/04/22 20:41:19, robliao wrote: > > > > > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > (left): > > > > > > > > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: > > // > > Used > > > to preserve a copy of the original command line. > > > 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. > > > > gfx_unittests does perform the cleanup, so maybe I'm misremembering here. > > > > Are the tests in this CL not cleaning up? > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Yes they are. In other words, test classes don't need to have a ScopedCommandLine on their class. I will be adjusting those to remove and document based off of consensus here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/22 21:07:26, robliao wrote: > On 2016/04/22 21:05:19, danakj wrote: > > On Fri, Apr 22, 2016 at 1:48 PM, <mailto:robliao@chromium.org> wrote: > > > > > On 2016/04/22 20:41:19, robliao wrote: > > > > > > > > > > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > > (left): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1908633002/diff/40001/chrome/browser/extensio... > > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:529: > > > // > > > Used > > > > to preserve a copy of the original command line. > > > > 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. > > > > > > gfx_unittests does perform the cleanup, so maybe I'm misremembering here. > > > > > > > Are the tests in this CL not cleaning up? > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Yes they are. In other words, test classes don't need to have a > ScopedCommandLine on their class. > > I will be adjusting those to remove and document based off of consensus here. Any other feedback?
extensions lgtm
On 2016/04/25 17:28:35, Devlin wrote: > extensions lgtm My impression was that some were leaning towards implicit cleanup. If folks are fine with the explicit cleanup, I'm fine with that too.
On Mon, Apr 25, 2016 at 10:31 AM, <robliao@chromium.org> wrote: > On 2016/04/25 17:28:35, Devlin wrote: > > extensions lgtm > > My impression was that some were leaning towards implicit cleanup. If > folks are > fine with the explicit cleanup, I'm fine with that too. > If we're already cleaning up implicitly, no need to add the explicit one too. For cases where it's not implicit (scoped to part of a test) using this ScopedCommandLine sgtm. > > https://codereview.chromium.org/1908633002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A quick survey reveals that setting the command line and letting the test infra handle resetting it is a well established practice, so I've updated the tests to be in line with that convention. ScopedCommandLine remains in use for two files.
c/b/chromeos lgtm
Description was changed from ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. BUG=604961 ========== to ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer) BUG=604961 ==========
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, danakj@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1908633002/#ps120001 (title: "Clean Up Other Tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1908633002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1908633002/120001
Message was sent while issue was closed.
Description was changed from ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer) BUG=604961 ========== to ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer) BUG=604961 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/48e31159a123a0f7e3f3e45b7e28a136408ddf3a Cr-Commit-Position: refs/heads/master@{#390741}
Message was sent while issue was closed.
Description was changed from ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer) BUG=604961 ========== to ========== Introduce ScopedCommandLine A few tests other there have the need to manipulate the command line and restore it after the test. ScopedCommandLine provides an easy way to set the options you need and then restore them when the ScopedCommandLine is out of scope. This also cleans up some other tests that were storing away the command line. This is unnecessary as the test infrastructure cleans this up (see TestClientInitializer) BUG=604961 Committed: https://crrev.com/48e31159a123a0f7e3f3e45b7e28a136408ddf3a Cr-Commit-Position: refs/heads/master@{#390741} ========== |
