Adds a command-line option for specifying a particular build to test and for ...

Adds a command-line option for specifying a particular build to test and for permitting the tests to be run regardless of the underlying platform. Also some minor fixes and cleanup, such as changing stable and dev versions to 3 and 4.
BUG=none
TEST=none

[kuchhal, 2009-10-08 21:34:32]

I have only looked at half the files yet. I would like to clarify the use of
-build flag first before I review the rest of change.

http://codereview.chromium.org/269016/diff/6001/6007
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/269016/diff/6001/6007#newcode5
Line 5: #ifndef CHROME_TEST_MINI_INSTALLER_TEST_CHROME_MINI_INSTALLER_H__
as long as you are touching this file, may be change the trailing two '_' to
just one '_'.

http://codereview.chromium.org/269016/diff/6001/6002
File chrome/test/mini_installer_test/run_all_unittests.cc (right):

http://codereview.chromium.org/269016/diff/6001/6002#newcode47
Line 47: printf("This test needs command line Arguments.\n");
small 'a'.

http://codereview.chromium.org/269016/diff/6001/6002#newcode48
Line 48: printf("Usage: mini_installer_tests.exe -{clean|backup}"
instead of mini_installer_tests.exe you can use command_line.program()

http://codereview.chromium.org/269016/diff/6001/6002#newcode50
Line 50: printf("Note: -clean arg will uninstall your chrome at all levels"
Remove 'Note:'

http://codereview.chromium.org/269016/diff/6001/6002#newcode79
Line 79: int mod_argc  = argc + 1;
extra space

http://codereview.chromium.org/269016/diff/6001/6002#newcode80
Line 80: char ** mod_argv = new char*[mod_argc];
remove space between char and **

http://codereview.chromium.org/269016/diff/6001/6002#newcode90
Line 90: char * normal_flag = "--gtest_filter=-MiniInstallTest.Cust*";
remove spaces between char and *

http://codereview.chromium.org/269016/diff/6001/6002#newcode92
Line 92: if (command_line.HasSwitch(L"build"))
so the help for build says you can say -build='dev' so shouldn't you check that
command_line.GetSwitch('build') != dev or stable.

http://codereview.chromium.org/269016/diff/6001/6003
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/269016/diff/6001/6003#newcode27
Line 27: static bool IS_CUSTOM_BUILD;
static variables aren't supposed to be all caps. Also i am not sure why these
are declared static. Why not just make regular member variables?

http://codereview.chromium.org/269016/diff/6001/6003#newcode32
Line 32: ChromeMiniInstaller* dev_user_inst_, *dev_sys_inst_;
Add a comment explaining what are these for. Also either break them on
individual lines and use Class* a; or use Class *a, *b;

http://codereview.chromium.org/269016/diff/6001/6003#newcode35
Line 35: 
remove blank line

http://codereview.chromium.org/269016/diff/6001/6003#newcode91
Line 91: dev_sys_inst_->SetBuildUnderTest(L"dev");
After seeing this I have a high level question. I am thinking  -build usage to
be like this:
-build='dev'
OR
-build='stable'
OR
-build='<custom path>'
so in one run mini_installer_tests.exe will only execute on type of tests. Why
not declare only one pair of *inst_ variables and use it?

http://codereview.chromium.org/269016/diff/6001/6003#newcode112
Line 112: PlatformThread::Sleep(2000);
why sleep on tear down?

[Ken Kania, 2009-10-08 22:42:56]

Originally, one run tested both the latest dev and stable channels.  This
change allows the user to specify one build in particular to test.  If the
user doesn't use the -build option, both the stable and dev tests would
run.  This is a bit messy to accomplish (therefore the hack in test.cc).  I
just talked with Anantha however, and she said it was no longer important to
run both the dev and stable channels in the same run.  So, I will clean
things up now and submit a patch.

Thanks,
Ken

On Thu, Oct 8, 2009 at 2:34 PM, <kuchhal@chromium.org> wrote:

> I have only looked at half the files yet. I would like to clarify the use
> of
> -build flag first before I review the rest of change.
>
>
> http://codereview.chromium.org/269016/diff/6001/6007
> File chrome/test/mini_installer_test/chrome_mini_installer.h (right):
>
> http://codereview.chromium.org/269016/diff/6001/6007#newcode5
> Line 5: #ifndef
> CHROME_TEST_MINI_INSTALLER_TEST_CHROME_MINI_INSTALLER_H__
> as long as you are touching this file, may be change the trailing two
> '_' to just one '_'.
>
> http://codereview.chromium.org/269016/diff/6001/6002
> File chrome/test/mini_installer_test/run_all_unittests.cc (right):
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode47
> Line 47: printf("This test needs command line Arguments.\n");
> small 'a'.
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode48
> Line 48: printf("Usage: mini_installer_tests.exe -{clean|backup}"
> instead of mini_installer_tests.exe you can use command_line.program()
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode50
> Line 50: printf("Note: -clean arg will uninstall your chrome at all
> levels"
> Remove 'Note:'
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode79
> Line 79: int mod_argc  = argc + 1;
> extra space
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode80
> Line 80: char ** mod_argv = new char*[mod_argc];
> remove space between char and **
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode90
> Line 90: char * normal_flag = "--gtest_filter=-MiniInstallTest.Cust*";
> remove spaces between char and *
>
> http://codereview.chromium.org/269016/diff/6001/6002#newcode92
> Line 92: if (command_line.HasSwitch(L"build"))
> so the help for build says you can say -build='dev' so shouldn't you
> check that command_line.GetSwitch('build') != dev or stable.
>
> http://codereview.chromium.org/269016/diff/6001/6003
> File chrome/test/mini_installer_test/test.cc (right):
>
> http://codereview.chromium.org/269016/diff/6001/6003#newcode27
> Line 27: static bool IS_CUSTOM_BUILD;
> static variables aren't supposed to be all caps. Also i am not sure why
> these are declared static. Why not just make regular member variables?
>
> http://codereview.chromium.org/269016/diff/6001/6003#newcode32
> Line 32: ChromeMiniInstaller* dev_user_inst_, *dev_sys_inst_;
> Add a comment explaining what are these for. Also either break them on
> individual lines and use Class* a; or use Class *a, *b;
>
> http://codereview.chromium.org/269016/diff/6001/6003#newcode35
> Line 35:
> remove blank line
>
> http://codereview.chromium.org/269016/diff/6001/6003#newcode91
> Line 91: dev_sys_inst_->SetBuildUnderTest(L"dev");
> After seeing this I have a high level question. I am thinking  -build
> usage to be like this:
> -build='dev'
> OR
> -build='stable'
> OR
> -build='<custom path>'
> so in one run mini_installer_tests.exe will only execute on type of
> tests. Why not declare only one pair of *inst_ variables and use it?
>
> http://codereview.chromium.org/269016/diff/6001/6003#newcode112
> Line 112: PlatformThread::Sleep(2000);
> why sleep on tear down?
>
>
> http://codereview.chromium.org/269016
>

[kkania, 2009-10-12 22:12:49]

Fixed reviewer's comments.

[kuchhal, 2009-10-13 18:53:09]

lgtm.

some nits below.

http://codereview.chromium.org/269016/diff/9001/9005
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/269016/diff/9001/9005#newcode113
Line 113: FAIL();
Not necessary to do in this change but at some time it would be good to get rid
of these redundant printf statements. A successful test should not print any
statements (if it passed that means everything worked). And in case of failure
the error message should be given to the FAIL method.

http://codereview.chromium.org/269016/diff/9001/9007
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/269016/diff/9001/9007#newcode68
Line 68: // The name of the browser/installation-type of this installer.
remove this comment as it is duplicate of next one.

http://codereview.chromium.org/269016/diff/9001/9003
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/269016/diff/9001/9003#newcode26
Line 26: ChromeMiniInstaller *user_inst_, *sys_inst_;
just use scoped_ptr to be more safe and remove the explicit delete statements

http://codereview.chromium.org/269016/diff/9001/9003#newcode38
Line 38: const CommandLine * cmd = CommandLine::ForCurrentProcess();
remove space before *

http://codereview.chromium.org/269016/diff/9001/9003#newcode41
Line 41: build = L"latest";
{ } not required.

http://codereview.chromium.org/269016/diff/9001/9003#newcode71
Line 71: printf("These tests don't run on this platform\n");
I now this was already there but this printf is redundant since the same error
message is getting printed in Setup(). By using scoped_ptr you can completely
remove this TearDown(0 method.

http://codereview.chromium.org/269016/diff/9001/9003#newcode79
Line 79: // previous installers accessible from our Google Chrome continuous
buildbot.
Since you are changing the define to GOOGLE_CHROME_BUILD, remove this comment?

http://codereview.chromium.org/269016/diff/9001/9003#newcode158
Line 158: if (!force_tests_)
why this additional check?