Group commandline settings in UI test and in process browser test.

Group cmdline settings in UI test and in_process_browser_test.


BUG=none
TEST=all UI tests and browser tests stays green.


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

[lzheng, 2010-11-10 18:14:33]

This will disable safebrowsing autoupdate in UI/browser tests.

I also refactored the commandline setups in UI test and browser tests a bit so
it becomes more clear how the commandlines are configured in each test.

[Scott Hess - ex-Googler, 2010-11-10 18:27:05]

http://codereview.chromium.org/4724004/diff/1/chrome/test/in_process_browser_...
File chrome/test/in_process_browser_test.cc (right):

http://codereview.chromium.org/4724004/diff/1/chrome/test/in_process_browser_...
chrome/test/in_process_browser_test.cc:147:
RenderProcessHost::set_run_renderer_in_process(true);
Previous code stored the original value in original_single_process_ before
setting it.

http://codereview.chromium.org/4724004/diff/1/chrome/test/test_launcher_utils.cc
File chrome/test/test_launcher_utils.cc (right):

http://codereview.chromium.org/4724004/diff/1/chrome/test/test_launcher_utils...
chrome/test/test_launcher_utils.cc:45:
command_line.AppendSwitch(switches::kSbDisableAutoUpdate);
Surrounding code uses ->.

http://codereview.chromium.org/4724004/diff/1/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/4724004/diff/1/chrome/test/ui/ui_test.h#newcod...
chrome/test/ui/ui_test.h:427: void PrepareCommonUITestCommandline(CommandLine*
arguments);
Is this missing implementation?

[Paweł Hajdan Jr., 2010-11-10 21:01:22]

I think it would be cleaner to do move and cleanup in separate CLs. It seems
this is missing ui_test.cc.

[Scott Hess - ex-Googler, 2010-11-10 23:29:31]

lgtm.

http://codereview.chromium.org/4724004/diff/34001/chrome/test/in_process_brow...
File chrome/test/in_process_browser_test.cc (right):

http://codereview.chromium.org/4724004/diff/34001/chrome/test/in_process_brow...
chrome/test/in_process_browser_test.cc:241:
subprocess_path.Append(chrome::kHelperProcessExecutablePath);
Wow, this Mac-specific code is messed up.

[Paweł Hajdan Jr., 2010-11-11 10:15:25]

http://codereview.chromium.org/4724004/diff/40001/chrome/test/in_process_brow...
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/4724004/diff/40001/chrome/test/in_process_brow...
chrome/test/in_process_browser_test.h:153: void
PrepareCommonInProcessBrowserTestCommandLine(CommandLine* arguments);
nit: InProcessBrowserTest is already the class name. No need to repeat it in the
method name. I'd suggest that PrepareTestCommandLine, which would call the
common test_launcher_utils code, and then add browser_tests-specific flags.

http://codereview.chromium.org/4724004/diff/40001/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/4724004/diff/40001/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:427: void PrepareCommonUITestCommandline(CommandLine*
arguments);
nit: Same here.

[lzheng1, 2010-11-12 06:29:20]

On Thu, Nov 11, 2010 at 2:15 AM, <phajdan.jr@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/4724004/diff/40001/chrome/test/in_process_brow...
> File chrome/test/in_process_browser_test.h (right):
>
>
>
http://codereview.chromium.org/4724004/diff/40001/chrome/test/in_process_brow...
> chrome/test/in_process_browser_test.h:153: void
> PrepareCommonInProcessBrowserTestCommandLine(CommandLine* arguments);
> nit: InProcessBrowserTest is already the class name. No need to repeat
> it in the method name. I'd suggest that PrepareTestCommandLine, which
> would call the common test_launcher_utils code, and then add
> browser_tests-specific flags.
>
> My intention was to group the setting of browser test commandlines into one
function, but not include the test_launcher_util in that function. Call
these two functions separately in LaunchBrowserHelper so it is clear that
there are two groups of commandline settings. If anyone wants to add a new
commandline setting, it is easier to tell which group it should be added by
reading the code and function name. Mixing them in one function would be
hard to notice the test_launcher_util call. Thoughts?



> http://codereview.chromium.org/4724004/diff/40001/chrome/test/ui/ui_test.h
> File chrome/test/ui/ui_test.h (right):
>
>
>
http://codereview.chromium.org/4724004/diff/40001/chrome/test/ui/ui_test.h#ne...
> chrome/test/ui/ui_test.h:427: void
> PrepareCommonUITestCommandline(CommandLine* arguments);
> nit: Same here.
>
>
> http://codereview.chromium.org/4724004/
>

[Paweł Hajdan Jr., 2010-11-12 09:11:07]

On Fri, Nov 12, 2010 at 07:29, Lei Zheng <lzheng@google.com> wrote:

> My intention was to group the setting of browser test commandlines into one
> function, but not include the test_launcher_util in that function. Call
> these two functions separately in LaunchBrowserHelper so it is clear that
> there are two groups of commandline settings.
>

It's not obvious to me what's the advantage of that.


> If anyone wants to add a new commandline setting, it is easier to tell
> which group it should be added by reading the code and function name. Mixing
> them in one function would be hard to notice the test_launcher_util call.
>

Arguably, having a call to test_launcher_utils in different place than the
test harness specific command-line flags makes the test_launcher_utils call
harder to notice. I think I'm still in favor of having both in the same
method, and not repeating the class name in the method name.

[lzheng, 2010-11-12 19:08:14]

I guess different people have different taste. We all agree that we could move
the part that setting commandline out as a subroutine. I can be flexible as long
as we are making things better. 
Take another look.

On 2010/11/12 09:11:07, Paweł Hajdan Jr. wrote:
> On Fri, Nov 12, 2010 at 07:29, Lei Zheng <mailto:lzheng@google.com> wrote:
> 
> > My intention was to group the setting of browser test commandlines into one
> > function, but not include the test_launcher_util in that function. Call
> > these two functions separately in LaunchBrowserHelper so it is clear that
> > there are two groups of commandline settings.
> >
> 
> It's not obvious to me what's the advantage of that.
> 
> 
> > If anyone wants to add a new commandline setting, it is easier to tell
> > which group it should be added by reading the code and function name. Mixing
> > them in one function would be hard to notice the test_launcher_util call.
> >
> 
> Arguably, having a call to test_launcher_utils in different place than the
> test harness specific command-line flags makes the test_launcher_utils call
> harder to notice. I think I'm still in favor of having both in the same
> method, and not repeating the class name in the method name.

[Paweł Hajdan Jr., 2010-11-13 11:55:08]

LGTM with comments.

http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
File chrome/test/in_process_browser_test.cc (right):

http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
chrome/test/in_process_browser_test.cc:209: // Propagate commandline settings
from test_launcher_utils which should be
nit: Remove "which should be used by browser".

http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
chrome/test/in_process_browser_test.h:152: // Config commandlines that are used
in InProcessBrowserTests.
nit: How about: prepare command line that will be used to launch the child
browser process with an in-process test?

http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
chrome/test/in_process_browser_test.h:153: void
PrepareTestCommandLine(CommandLine* arguments);
nit: arguments -> command_line for accuracy

http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:665: // Propagate commandline settings from
test_launcher_utils which should be
nit: Same comment as in in_process_browser_test.cc

http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.h#ne...
chrome/test/ui/ui_test.h:426: // Config commandlines that are used in UI tests.
nit: Same comment as in in_process_borwser_test.h.

[lzheng, 2010-11-16 00:39:27]

All done and committed.

Thanks for your review!

Lei


On 2010/11/13 11:55:08, Paweł Hajdan Jr. wrote:
> LGTM with comments.
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
> File chrome/test/in_process_browser_test.cc (right):
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
> chrome/test/in_process_browser_test.cc:209: // Propagate commandline settings
> from test_launcher_utils which should be
> nit: Remove "which should be used by browser".
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
> File chrome/test/in_process_browser_test.h (right):
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
> chrome/test/in_process_browser_test.h:152: // Config commandlines that are
used
> in InProcessBrowserTests.
> nit: How about: prepare command line that will be used to launch the child
> browser process with an in-process test?
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/in_process_brow...
> chrome/test/in_process_browser_test.h:153: void
> PrepareTestCommandLine(CommandLine* arguments);
> nit: arguments -> command_line for accuracy
> 
> http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.cc
> File chrome/test/ui/ui_test.cc (right):
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.cc#n...
> chrome/test/ui/ui_test.cc:665: // Propagate commandline settings from
> test_launcher_utils which should be
> nit: Same comment as in in_process_browser_test.cc
> 
> http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.h
> File chrome/test/ui/ui_test.h (right):
> 
>
http://codereview.chromium.org/4724004/diff/48001/chrome/test/ui/ui_test.h#ne...
> chrome/test/ui/ui_test.h:426: // Config commandlines that are used in UI
tests.
> nit: Same comment as in in_process_borwser_test.h.