Replace ScopedCommandLineOverride with TestSuite listener.

Replace ScopedCommandLineOverride with TestSuite listener.

This was split out of http://codereview.chromium.org/8588067/.

BUG=none
TEST=none, but this does make testing easier.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111057

[Paweł Hajdan Jr., 2011-11-21 12:14:55]

Drive-by (and see chrome/test/OWNERS).

I think there's much better way. Add a test listener to base/test/test_suite,
similar to one from content/test/content_test_suite, to take care of resetting
the CommandLine.

This is actually a great thing to do, just the way I suggested should work even
if people forget about it.

[asargent_no_longer_on_chrome, 2011-11-21 17:14:23]

http://codereview.chromium.org/8561031/diff/1/chrome/test/base/scoped_command...
File chrome/test/base/scoped_command_line_override.h (right):

http://codereview.chromium.org/8561031/diff/1/chrome/test/base/scoped_command...
chrome/test/base/scoped_command_line_override.h:27: explicit
ScopedCommandLineOverride(CommandLine& old_command_line);
nit: const CommandLine&

http://codereview.chromium.org/8561031/diff/1/chrome/test/base/scoped_command...
chrome/test/base/scoped_command_line_override.h:30: // constructing a vector,
and va_list won't work with references.
It might be better to group the 3 constructors that take strings together, eg.

explicit ScopedCommandLineOverride(const std::string& new_switch);
ScopedCommandLineOverride(const std::string& new_switch1,
                          const std::string& switch2);
...

and then

explicit ScopedCommandLineOverride(CommandLine& old_command_line);

[miket_OOO, 2011-11-21 18:51:07]

> I think there's much better way. Add a test listener to base/test/test_suite,
> similar to one from content/test/content_test_suite, to take care of resetting
> the CommandLine.

OK, done, thanks for the great suggestion. Much simpler now. Please re-review.

[asargent_no_longer_on_chrome, 2011-11-21 19:19:50]

lgtm

http://codereview.chromium.org/8561031/diff/6001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/8561031/diff/6001/base/test/test_suite.cc#newc...
base/test/test_suite.cc:53: : old_command_line_(CommandLine::NO_PROGRAM) {
I guess it doesn't really matter since you set it in the OnTestStart, but would
it be better to use CommandLine::ForCurrentProcess() rather than
CommandLine::NO_PROGRAM here?

[Paweł Hajdan Jr., 2011-11-21 19:59:32]

LGTM with a nit, thanks.

http://codereview.chromium.org/8561031/diff/6001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

http://codereview.chromium.org/8561031/diff/6001/base/test/test_suite.cc#newc...
base/test/test_suite.cc:253: testing::TestEventListeners& listeners =
nit: It's slightly inconsistent to add the MAYBE test listeners in a function
and this one inline. Please do those two on the same abstraction level. I
slightly prefer doing both of them inline.

[commit-bot: I haz the power, 2011-11-21 22:18:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8561031/6005

[commit-bot: I haz the power, 2011-11-21 22:18:43]

Presubmit check for 8561031-6005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

Presubmit checks took 17.5s to calculate.

[miket_OOO, 2011-11-21 22:22:55]

+sky

[miket_OOO, 2011-11-21 22:22:56]

+sky

[sky, 2011-11-21 22:29:50]

LGTM

[commit-bot: I haz the power, 2011-11-21 22:45:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8561031/6005

[commit-bot: I haz the power, 2011-11-21 22:45:27]

Presubmit check for 8561031-6005 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

Presubmit checks took 1.7s to calculate.

[miket_OOO, 2011-11-21 23:01:52]

+zea

[Nicolas Zea, 2011-11-21 23:04:03]

sync LGTM

[commit-bot: I haz the power, 2011-11-21 23:06:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8561031/6005

[commit-bot: I haz the power, 2011-11-22 00:44:41]

Change committed as 111057

[Aaron Boodman, 2011-11-30 01:11:09]

http://codereview.chromium.org/8561031/diff/6005/chrome/common/extensions/ext...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/8561031/diff/6005/chrome/common/extensions/ext...
chrome/common/extensions/extension_manifests_unittest.cc:381:
switches::kEnablePlatformApps);
This doesn't seem as good. Now all the manifest tests below have this flag
turned on.

Without ScopedCommandLineOverride, we should move the tests that need this flag
to a separate test.

[miket_OOO, 2011-12-06 22:23:42]

http://codereview.chromium.org/8561031/diff/6005/chrome/common/extensions/ext...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/8561031/diff/6005/chrome/common/extensions/ext...
chrome/common/extensions/extension_manifests_unittest.cc:381:
switches::kEnablePlatformApps);
On 2011/11/30 01:11:09, Aaron Boodman wrote:
> This doesn't seem as good. Now all the manifest tests below have this flag
> turned on.
> 
> Without ScopedCommandLineOverride, we should move the tests that need this
flag
> to a separate test.

Review for this change is on its way.