Add a test for installing Chrome at system level.

Add a test for installing Chrome at system level.

NOTRY=True
BUG=264859
TEST=
1) Uninstall Chrome (if it's installed.) 
2) Build Chrome with Release mode and make sure that mini_installer.exe is created. 
3) Go to src\chrome\test\mini_installer 
4) Run "python test_installer.py config\config.config --build-dir=<build-dir> --target=Release" where <build-dir> is the path to main build directory (the parent of the Release directory). The test should pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222067

[sukolsak, 2013-09-05 22:52:12]

Could you please take a look at this?

[robertshield, 2013-09-05 23:20:11]

lgtm

https://codereview.chromium.org/24007002/diff/1/chrome/test/mini_installer/co...
File chrome/test/mini_installer/config/config.config (right):

https://codereview.chromium.org/24007002/diff/1/chrome/test/mini_installer/co...
chrome/test/mini_installer/config/config.config:45:
"install_chrome_at_user_level", "chrome_user_installed_not_inuse",
nit: mild preference to name these actions as
"install_chrome_user"/"install_chrome_system",
"launch_chrome_user"/"launch_chrome_system" etc.

[grt (UTC plus 2), 2013-09-06 02:18:48]

this is totally rad. lgtm.

[gab, 2013-09-06 18:02:20]

Very very nice :)! This is so exciting!

LGTM!
Gab

https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
File chrome/test/mini_installer/config/config.config (right):

https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
chrome/test/mini_installer/config/config.config:43: [
This makes me think it would be great to be able to give a description to these
tests here (as we get more tests it will be tedious to have to read the sequence
of actions to know which test is which...).

Doesn't need to be in this CL.

I know we can't have comments in JSON, but we could perhaps add a "name" and
"description" field to each test? That would also help when reporting an error
(I think right now we only report the state that failed, but not which test it
is...).

[grt (UTC plus 2), 2013-09-06 18:49:01]

On 2013/09/06 18:02:20, gab wrote:
> Very very nice :)! This is so exciting!
> 
> LGTM!
> Gab
> 
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> File chrome/test/mini_installer/config/config.config (right):
> 
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> chrome/test/mini_installer/config/config.config:43: [
> This makes me think it would be great to be able to give a description to
these
> tests here (as we get more tests it will be tedious to have to read the
sequence
> of actions to know which test is which...).
> 
> Doesn't need to be in this CL.
> 
> I know we can't have comments in JSON, but we could perhaps add a "name" and
> "description" field to each test? That would also help when reporting an error
> (I think right now we only report the state that failed, but not which test it
> is...).

+1 (https://codereview.chromium.org/23060002/#msg3 ;-) )

[sukolsak, 2013-09-06 21:07:59]

On 2013/09/06 18:49:01, grt wrote:
> On 2013/09/06 18:02:20, gab wrote:
> > Very very nice :)! This is so exciting!
> > 
> > LGTM!
> > Gab
> > 
> >
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> > File chrome/test/mini_installer/config/config.config (right):
> > 
> >
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> > chrome/test/mini_installer/config/config.config:43: [
> > This makes me think it would be great to be able to give a description to
> these
> > tests here (as we get more tests it will be tedious to have to read the
> sequence
> > of actions to know which test is which...).
> > 
> > Doesn't need to be in this CL.
> > 
> > I know we can't have comments in JSON, but we could perhaps add a "name" and
> > "description" field to each test? That would also help when reporting an
error
> > (I think right now we only report the state that failed, but not which test
it
> > is...).
> 
> +1 (https://codereview.chromium.org/23060002/#msg3 ;-) )

Any preference on what the comments should look like?

Descriptive:

"tests": [
  {
    "name": "Install/uninstall Chrome at user level",
    "description": ["clean", "install_chrome_at_user_level", ...]
  },
  {
    "name": "Install/uninstall Chrome at system level",
    "description": [ ... ]
  },
  {
    "name": "Upgrade Chrome at system level",
    "description": [ ... ]
  },
  ...
]

Minimalist:

"tests": [
  [
    "Install/uninstall Chrome at user level",
    ["clean", "install_chrome_at_user_level", ...]
  ],
  [
    "Install/uninstall Chrome at system level",
    [ ... ]
  ],
  [
    "Upgrade Chrome at user level",
    [ ... ]
  ],
  ...
]

?

The first format might also be better if we want to add some parameters to the
tests in the future.

[gab, 2013-09-09 13:17:15]

On 2013/09/06 21:07:59, sukolsak wrote:
> On 2013/09/06 18:49:01, grt wrote:
> > On 2013/09/06 18:02:20, gab wrote:
> > > Very very nice :)! This is so exciting!
> > > 
> > > LGTM!
> > > Gab
> > > 
> > >
> >
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> > > File chrome/test/mini_installer/config/config.config (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/24007002/diff/7001/chrome/test/mini_installer...
> > > chrome/test/mini_installer/config/config.config:43: [
> > > This makes me think it would be great to be able to give a description to
> > these
> > > tests here (as we get more tests it will be tedious to have to read the
> > sequence
> > > of actions to know which test is which...).
> > > 
> > > Doesn't need to be in this CL.
> > > 
> > > I know we can't have comments in JSON, but we could perhaps add a "name"
and
> > > "description" field to each test? That would also help when reporting an
> error
> > > (I think right now we only report the state that failed, but not which
test
> it
> > > is...).
> > 
> > +1 (https://codereview.chromium.org/23060002/#msg3 ;-) )
> 
> Any preference on what the comments should look like?
> 
> Descriptive:
> 
> "tests": [
>   {
>     "name": "Install/uninstall Chrome at user level",
>     "description": ["clean", "install_chrome_at_user_level", ...]
>   },
>   {
>     "name": "Install/uninstall Chrome at system level",
>     "description": [ ... ]
>   },
>   {
>     "name": "Upgrade Chrome at system level",
>     "description": [ ... ]
>   },
>   ...
> ]
> 
> Minimalist:
> 
> "tests": [
>   [
>     "Install/uninstall Chrome at user level",
>     ["clean", "install_chrome_at_user_level", ...]
>   ],
>   [
>     "Install/uninstall Chrome at system level",
>     [ ... ]
>   ],
>   [
>     "Upgrade Chrome at user level",
>     [ ... ]
>   ],
>   ...
> ]
> 
> ?
> 
> The first format might also be better if we want to add some parameters to the
> tests in the future.

I prefer the descriptive version.
How about having:
"name": A short name to be printed when the test fails. (e.g., User-level)
"description": A more detailed description of exactly which scenario this tests.
(e.g., Installs Chrome at user-level.)
"test": The current list of state/actions.

Perhaps name and description say the same thing for now, but I can imagine
something like "Installs system-level Chrome over user-level Chrome and makes
sure shared values are taken over by system-level Chrome." (at which point
having a simple name System-level over user-level" is nice for reporting errors;
a developer can then look at the description to know what this does in more
details).

[gab, 2013-09-09 13:18:28]

Once again; I'm fine with committing this CL as-is and following up with the
proposed change.

(re-LGTM)

[sukolsak, 2013-09-09 18:27:53]

On 2013/09/09 13:18:28, gab wrote:
> Once again; I'm fine with committing this CL as-is and following up with the
> proposed change.
> 
> (re-LGTM)

Thank you.

Just FYI, there is a bug in the installer that leaves
HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts in the registry
when we install/uninstall Chrome at system level (see
https://codereview.chromium.org/23453032/#msg5). This patch will make the
mini_installer test slave fail until that bug is fixed.

[commit-bot: I haz the power, 2013-09-09 18:59:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sukolsak@chromium.org/24007002/7001

[commit-bot: I haz the power, 2013-09-09 19:00:06]

Change committed as 222067

[gab, 2013-09-10 17:13:15]

On Sep 9, 2013 2:27 PM, <sukolsak@chromium.org> wrote:
>
> On 2013/09/09 13:18:28, gab wrote:
>>
>> Once again; I'm fine with committing this CL as-is and following up with
the
>> proposed change.
>
>
>> (re-LGTM)
>
>
> Thank you.
>
> Just FYI, there is a bug in the installer that leaves
> HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts in the
registry
> when we install/uninstall Chrome at system level (see
> https://codereview.chromium.org/23453032/#msg5). This patch will make the
> mini_installer test slave fail until that bug is fixed.

Interesting, I thought we were only checking a single/simple key in the
registry for now, no?

>
> https://codereview.chromium.org/24007002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sukolsak, 2013-09-10 21:34:02]

On 2013/09/10 17:13:15, gab wrote:
> On Sep 9, 2013 2:27 PM, <mailto:sukolsak@chromium.org> wrote:
> >
> > On 2013/09/09 13:18:28, gab wrote:
> >>
> >> Once again; I'm fine with committing this CL as-is and following up with
> the
> >> proposed change.
> >
> >
> >> (re-LGTM)
> >
> >
> > Thank you.
> >
> > Just FYI, there is a bug in the installer that leaves
> > HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts in the
> registry
> > when we install/uninstall Chrome at system level (see
> > https://codereview.chromium.org/23453032/#msg5). This patch will make the
> > mini_installer test slave fail until that bug is fixed.
> 
> Interesting, I thought we were only checking a single/simple key in the
> registry for now, no?

In chrome_user_not_installed.prop, we check that
HKEY_CURRENT_USER\$CHROME_UPDATE_REGISTRY_SUBKEY (which is resolved to
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}
or HKEY_CURRENT_USER\Software\Chromium) doesn't exist.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/mini_i...

[gab, 2013-09-10 21:54:01]

On 2013/09/10 21:34:02, sukolsak wrote:
> On 2013/09/10 17:13:15, gab wrote:
> > On Sep 9, 2013 2:27 PM, <mailto:sukolsak@chromium.org> wrote:
> > >
> > > On 2013/09/09 13:18:28, gab wrote:
> > >>
> > >> Once again; I'm fine with committing this CL as-is and following up with
> > the
> > >> proposed change.
> > >
> > >
> > >> (re-LGTM)
> > >
> > >
> > > Thank you.
> > >
> > > Just FYI, there is a bug in the installer that leaves
> > > HKEY_CURRENT_USER\Software\Chromium\BrowserCrashDumpAttempts in the
> > registry
> > > when we install/uninstall Chrome at system level (see
> > > https://codereview.chromium.org/23453032/#msg5). This patch will make the
> > > mini_installer test slave fail until that bug is fixed.
> > 
> > Interesting, I thought we were only checking a single/simple key in the
> > registry for now, no?
> 
> In chrome_user_not_installed.prop, we check that
> HKEY_CURRENT_USER\$CHROME_UPDATE_REGISTRY_SUBKEY (which is resolved to
>
HKEY_CURRENT_USER\Software\Google\Update\Clients\{8A69D345-D564-463c-AFF1-A69D9E530F96}
> or HKEY_CURRENT_USER\Software\Chromium) doesn't exist.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/mini_i...

Ah nice, well there you go, this test framework just caught it's first real
product bug :)! Congrats!