Adding simple Chrome install testing using multi-install and cleaning up a bit.

Adding simple Chrome install testing using multi-install and cleaning up a bit.




BUG=none
TEST=none

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

[Huyen, 2011-09-23 22:47:12]

kkania, please take look when you have a chance. In this change, I added simple
test of Chrome install using multi-install. The bulk of the change has to do
with cleaning up common functions like launching the installer, launching
Chrome, checking registry, and etc. Thanks!

[kkania, 2011-09-26 20:00:53]

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:58: :process_id(pid) {}
4 space indent here, and 1 space between : and process_id; you have it right
below

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:61: return process_id
== entry.pid();
two space indent here

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:145:
InstallMiniInstaller(false, mini_installer_);
not sure why this passes mini_installer_ if its accessible by the called func.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:204: << system_install_
<< " (1 means system, otherwise user)";
use the ternary operator here to print out 'system' or 'user'

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:216:
args.AppendArg("--multi-install");
use
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util...
instead of duplicating the constant value; maybe same for below

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:228: EXPECT_NE(0,
how about just EXPECT_TRUE(type & ....); same below

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:355: if
(GetChromeVersionFromRegistry(&version)) {
This function fails silently in a lot of places. In some cases we want to be
sure that the uninstall was successful. To do that, I think we should make this
func expect chrome is installed and should fail if uninstall is unsuccessful.
Make another that removes chrome if its installed.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:375:
cmd.AppendArg("--uninstall");
same here

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:427:
cmd.AppendArg("--uninstall");
don't hardcode these

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:684:
installer.AppendArg("--do-not-create-shortcuts");
same here

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:706:
MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
what's better about this new way?

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:747: } else if
((!system_install_) &&
one line, don't need all these parens

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:34: void
ChromeMiniInstaller::InstallUsingMultiInstall();
You don't need the ChromeMiniInstaller:: part.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:36: void
VerifyMultiInstall();
This doesn't need to be public. It doesn't really need to be a separate func,
since it is only called in one place.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:113: void
LaunchChrome(const bool kill);
the style guide says don't use const with a primitive type.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:126: void
LaunchInstaller(const FilePath& path, const CommandLine& args);
any reason not to just pass a CommandLine, since it can hold the path to the
exe?

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/run_all_unittests.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/run_all_unittests.cc:21: ChromeMiniInstaller
installer(false,
single line

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:35:
cmd->HasSwitch(installer::switches::kChromeFrame));
can't you replace this with false? same below?

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:56: user_inst_.reset(new
ChromeMiniInstaller(false,
collapse on one line; same below

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:181:
user_inst_->InstallUsingMultiInstall();
do these work when you do --chrome-frame too?

[Huyen, 2011-09-27 23:00:24]

kkania, please take another look when you have a chance. Thanks!

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:58: :process_id(pid) {}
On 2011/09/26 20:00:53, kkania wrote:
> 4 space indent here, and 1 space between : and process_id; you have it right
> below

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:61: return process_id
== entry.pid();
On 2011/09/26 20:00:53, kkania wrote:
> two space indent here

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:145:
InstallMiniInstaller(false, mini_installer_);
On 2011/09/26 20:00:53, kkania wrote:
> not sure why this passes mini_installer_ if its accessible by the called func.

Because sometime we use diff installer or other type of installer. We can pass
an installer type instead of the actual installer but that doesn't make this
better.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:204: << system_install_
<< " (1 means system, otherwise user)";
On 2011/09/26 20:00:53, kkania wrote:
> use the ternary operator here to print out 'system' or 'user'

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:216:
args.AppendArg("--multi-install");
On 2011/09/26 20:00:53, kkania wrote:
> use
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/installer/util...
> instead of duplicating the constant value; maybe same for below

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:228: EXPECT_NE(0,
On 2011/09/26 20:00:53, kkania wrote:
> how about just EXPECT_TRUE(type & ....); same below

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:355: if
(GetChromeVersionFromRegistry(&version)) {
On 2011/09/26 20:00:53, kkania wrote:
> This function fails silently in a lot of places. In some cases we want to be
> sure that the uninstall was successful. To do that, I think we should make
this
> func expect chrome is installed and should fail if uninstall is unsuccessful.
> Make another that removes chrome if its installed.
This seems to be used by the setup and teardown steps only. In these cases if
Chrome is not install then we just continue, there doesn't seem to be a need to
fail.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:375:
cmd.AppendArg("--uninstall");
On 2011/09/26 20:00:53, kkania wrote:
> same here

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:427:
cmd.AppendArg("--uninstall");
On 2011/09/26 20:00:53, kkania wrote:
> don't hardcode these

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:684:
installer.AppendArg("--do-not-create-shortcuts");
On 2011/09/26 20:00:53, kkania wrote:
> same here

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:706:
MiniInstallerTestUtil::CloseProcesses(installer::kChromeExe);
On 2011/09/26 20:00:53, kkania wrote:
> what's better about this new way?
I thought this wasn't really needed either. Considered killing only Chrome
started by the installer but see no value in that. Good catch, removing this.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:747: } else if
((!system_install_) &&
On 2011/09/26 20:00:53, kkania wrote:
> one line, don't need all these parens

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:34: void
ChromeMiniInstaller::InstallUsingMultiInstall();
On 2011/09/26 20:00:53, kkania wrote:
> You don't need the ChromeMiniInstaller:: part.

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:36: void
VerifyMultiInstall();
On 2011/09/26 20:00:53, kkania wrote:
> This doesn't need to be public. It doesn't really need to be a separate func,
> since it is only called in one place.

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:113: void
LaunchChrome(const bool kill);
On 2011/09/26 20:00:53, kkania wrote:
> the style guide says don't use const with a primitive type.

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:126: void
LaunchInstaller(const FilePath& path, const CommandLine& args);
On 2011/09/26 20:00:53, kkania wrote:
> any reason not to just pass a CommandLine, since it can hold the path to the
> exe?
Good point. Now using CommandLine.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/run_all_unittests.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/run_all_unittests.cc:21: ChromeMiniInstaller
installer(false,
On 2011/09/26 20:00:53, kkania wrote:
> single line

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:35:
cmd->HasSwitch(installer::switches::kChromeFrame));
On 2011/09/26 20:00:53, kkania wrote:
> can't you replace this with false? same below?

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:56: user_inst_.reset(new
ChromeMiniInstaller(false,
On 2011/09/26 20:00:53, kkania wrote:
> collapse on one line; same below

Done.

http://codereview.chromium.org/8037004/diff/1002/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:181:
user_inst_->InstallUsingMultiInstall();
On 2011/09/26 20:00:53, kkania wrote:
> do these work when you do --chrome-frame too?
I haven't try this scenario yet but it is a test that will be added.

[kkania, 2011-09-28 18:02:43]

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:56: class
ChromeProcessFilter : public base::ProcessFilter {
this isn't used, remove

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:257: CommandLine
installer(FilePath::FromWStringHack(
just do FilePath(...) instead of FromWStringHack

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:351:
ASSERT_FALSE(cmd.GetCommandLineString().empty())
don't need this

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:400: if
(cmd.GetCommandLineString().empty()) {
if (cmd.GetProgram().empty())

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:675: LOG(ERROR) <<
"Installer failed.";
this should fail the test, not log. Remove the log. same below

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:675: LOG(ERROR) <<
"Installer failed.";
macros can expand to multiple lines, so to be safe, you should change the if to
be
if (...) {
  MACRO...
}

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:694: LOG(ERROR) <<
"Could not launch process: " << exe.value();
change to error

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:123: // Launch installer
using provided |command|.
Ideally this should be changed to RunInstaller, since it doesn't just launch the
process, it waits for it to run, right? If you don't want to change the name,
just mention it in the comment.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:30: ChromeMiniInstaller
systeminstall(true, true);
this can be simplified by just ChromeMiniInstaller(true, true).UnInstall(); same
below

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:35: ChromeMiniInstaller
systeminstall(true,false);
space

[Huyen, 2011-09-28 18:55:47]

Thanks!

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.cc (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:56: class
ChromeProcessFilter : public base::ProcessFilter {
On 2011/09/28 18:02:43, kkania wrote:
> this isn't used, remove

Done.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:257: CommandLine
installer(FilePath::FromWStringHack(
On 2011/09/28 18:02:43, kkania wrote:
> just do FilePath(...) instead of FromWStringHack
FilePath doesn't have a constructor for wchar_t.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:351:
ASSERT_FALSE(cmd.GetCommandLineString().empty())
On 2011/09/28 18:02:43, kkania wrote:
> don't need this

Done.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:400: if
(cmd.GetCommandLineString().empty()) {
On 2011/09/28 18:02:43, kkania wrote:
> if (cmd.GetProgram().empty())

Done.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:675: LOG(ERROR) <<
"Installer failed.";
On 2011/09/28 18:02:43, kkania wrote:
> this should fail the test, not log. Remove the log. same below

Done.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.cc:694: LOG(ERROR) <<
"Could not launch process: " << exe.value();
On 2011/09/28 18:02:43, kkania wrote:
> change to error

you mean change to an assert?

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/chrome_mini_installer.h (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/chrome_mini_installer.h:123: // Launch installer
using provided |command|.
On 2011/09/28 18:02:43, kkania wrote:
> Ideally this should be changed to RunInstaller, since it doesn't just launch
the
> process, it waits for it to run, right? If you don't want to change the name,
> just mention it in the comment.

Done.

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/8037004/diff/8001/chrome/test/mini_installer_t...
chrome/test/mini_installer_test/test.cc:30: ChromeMiniInstaller
systeminstall(true, true);
On 2011/09/28 18:02:43, kkania wrote:
> this can be simplified by just ChromeMiniInstaller(true, true).UnInstall();
same
> below

Done.

[kkania, 2011-09-28 21:52:06]

LGTM, assuming all the tests pass!

http://codereview.chromium.org/8037004/diff/14001/chrome/test/mini_installer_...
File chrome/test/mini_installer_test/test.cc (right):

http://codereview.chromium.org/8037004/diff/14001/chrome/test/mini_installer_...
chrome/test/mini_installer_test/test.cc:33:
ChromeMiniInstaller(true,false).UnInstall();
missed a space