Implement the window verifier in the Automated Installer Testing Framework.

Implement the window verifier in the Automated Installer Testing Framework.

NOTRY=True 
BUG=264859
TEST= 
1) Uninstall Chrome. 
2) Put mini_installer.exe in the same folder as test_installer.py. 
3) Run "python test_installer.py config\config.config". 
4) The script will install Chrome and then uninstall Chrome. At each state, it will check that the window "Welcome to Chrome" exists (or doesn't exist). You should see output similar to the following: 

"Test: clean -> install chrome -> chrome_installed -> uninstall chrome -> clean ... ok 

---------------------------------------------------------------------- 
Ran 1 test in 12.345s"

[sukolsak, 2013-08-13 13:22:34]

Could you please take a look?

[gab, 2013-08-13 17:49:26]

looks good

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

https://codereview.chromium.org/23060002/diff/1/chrome/test/mini_installer/co...
chrome/test/mini_installer/config/chrome_installed.prop:7: "Welcome to Chrome":
{"exists": true}
The installer launches chrome.exe and exits shortly after without waiting for it
to actually launch.

Asserting that the window is up immediately after the install completes will
result in a flaky test as it relies on chrome.exe having been scheduled by the
OS on the machine and having made it far enough to have a window with the right
title before this verifier kicks in.

Instead I think we should add --do-not-launch-chrome to the install command
being tested.

Then add another command which runs chrome.exe and then verifies another state
made of chrome_installed.prop + chrome_running.prop.

The command that runs chrome.exe should also be made so that it only returns
when the window is up (essentially using the verifier you wrote to probe for the
Chrome window in a loop before returning).

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

https://codereview.chromium.org/23060002/diff/1/chrome/test/mini_installer/co...
chrome/test/mini_installer/config/chrome_not_installed.prop:7: "Welcome to
Chrome": {"exists": false}
You will also need a command to kill the running chrome.exe to go back to
chrome_installed state before you can even run the uninstall command.

https://codereview.chromium.org/23060002/diff/1/chrome/test/mini_installer/wi...
File chrome/test/mini_installer/window_verifier.py (right):

https://codereview.chromium.org/23060002/diff/1/chrome/test/mini_installer/wi...
chrome/test/mini_installer/window_verifier.py:20: assert expectation['exists'],
"Window '%s' exists" % window_title
Isn't there a way to test both cases in 1 line, something like:

assertequal expectation['exists'], window_title in titles

?

[grt (UTC plus 2), 2013-08-16 02:39:07]

i think it would help to have some commentary somewhere that explains what the
entries in the prop files are verifying and why. one way to do this would be to
add some sort of doc string to the JSON objects. for example, the dict that
holds "exists": true could also have "comment": "verifies that no goats were
harmed when the polarity of the neutron flux was reversed." these same comments
could be included in the output of the test to make it easier to diagnose
failures.

relating to the window verifier, it's not clear to me why a "Welcome to Chrome"
window is being looked for. is this to ensure that Chrome is launched by the
installer? is it to ensure that something specific happens on first-run? or
something else? if it's testing first-run behavior, it seems that this should be
tested elsewhere since it isn't related to installation. if it's testing that
chrome is launched by the installer, then perhaps there's a more robust way to
do it.

[gab, 2013-08-16 13:50:15]

On 2013/08/16 02:39:07, grt wrote:
> i think it would help to have some commentary somewhere that explains what the
> entries in the prop files are verifying and why. one way to do this would be
to
> add some sort of doc string to the JSON objects. for example, the dict that
> holds "exists": true could also have "comment": "verifies that no goats were
> harmed when the polarity of the neutron flux was reversed." these same
comments
> could be included in the output of the test to make it easier to diagnose
> failures.

That could be useful indeed, but not a blocker to get the first basic test up
and running imo.

> 
> relating to the window verifier, it's not clear to me why a "Welcome to
Chrome"
> window is being looked for. is this to ensure that Chrome is launched by the
> installer? is it to ensure that something specific happens on first-run? or
> something else? if it's testing first-run behavior, it seems that this should
be
> tested elsewhere since it isn't related to installation. if it's testing that
> chrome is launched by the installer, then perhaps there's a more robust way to
> do it.

We had discussed this in our first meeting with Carlos. He was arguing to we'd
like to do more than just check that chrome.exe is running (which we also do).
Making sure a browser window is up is likely the best way; and I disagree that
making sure we get the first run behaviour post install is not installation
related (although what the title of the window is on first run is probably
beyond the scope of this test...).

Can we know the parent process when enumerating windows? If so, it might be
better to look for a window owned by our chrome.exe instead of by title.

Otherwise, this design as much evolved over offline discussions between
sukolsak, robertshield, and myself over the last 2 days; let's try to sync up.

Cheers,
Gab

[grt (UTC plus 2), 2013-08-16 15:36:54]

On 2013/08/16 13:50:15, gab wrote:
> On 2013/08/16 02:39:07, grt wrote:
> > i think it would help to have some commentary somewhere that explains what
the
> > entries in the prop files are verifying and why. one way to do this would be
> to
> > add some sort of doc string to the JSON objects. for example, the dict that
> > holds "exists": true could also have "comment": "verifies that no goats were
> > harmed when the polarity of the neutron flux was reversed." these same
> comments
> > could be included in the output of the test to make it easier to diagnose
> > failures.
> 
> That could be useful indeed, but not a blocker to get the first basic test up
> and running imo.
> 
> > 
> > relating to the window verifier, it's not clear to me why a "Welcome to
> Chrome"
> > window is being looked for. is this to ensure that Chrome is launched by the
> > installer? is it to ensure that something specific happens on first-run? or
> > something else? if it's testing first-run behavior, it seems that this
should
> be
> > tested elsewhere since it isn't related to installation. if it's testing
that
> > chrome is launched by the installer, then perhaps there's a more robust way
to
> > do it.
> 
> We had discussed this in our first meeting with Carlos. He was arguing to we'd
> like to do more than just check that chrome.exe is running (which we also do).
> Making sure a browser window is up is likely the best way;

Makes sense (although on Windows 8 the first thing that pops up is the "You're
almost done" dialog).

> and I disagree that
> making sure we get the first run behaviour post install is not installation
> related (although what the title of the window is on first run is probably
> beyond the scope of this test...).

It makes sense to me that an installer test would verify that first-run is
triggered on a new install. I don't think it should test any of the first-run
behavior, though. Since the exact actions and behaviors of first-run are a
browser feather rather than an installer feature, tests for them belong down
with the browser rather than up with the installer (in other words, Layer 5
should test Layer 5 and that it triggers something in Layer 4. Testing the
details of that "something" in Layer 4 belongs with the general Layer 4 tests).

> Can we know the parent process when enumerating windows? If so, it might be
> better to look for a window owned by our chrome.exe instead of by title.

Good idea. GetWindowThreadProcessId is the trick here.

[gab, 2013-09-06 18:27:38]

So what's up with this verifier? Are we doing it? Not saying it's high priority
just wondering?

[sukolsak, 2013-09-06 20:04:44]

On 2013/09/06 18:27:38, gab wrote:
> So what's up with this verifier? Are we doing it? Not saying it's high
priority
> just wondering?

I'm just not really sure what we want to verify (class names?) and what the
format to this verifier would look like. Should it be something like

{
  "$PATH\\TO\\chrome.exe": {"exists": true", classes": ["Chrome_WidgetWin_1",
"Chrome_WidgetWin_2"]}
}

or

{
  "$PATH\\TO\\chrome.exe|Chrome_WidgetWin_1": {"exists": true"},
  "$PATH\\TO\\chrome.exe|Chrome_WidgetWin_2": {"exists": true"}
}
?

Each choice will have different merging behaviors.

This is unrelated, but there is also a problem in merging in the file verifier.
Currently, if we merge

{
  "Files": {
    "$LOCAL_APPDATA\\$CHROME_DIR\\Application": {"exists": false}
  }
}

with

{
  "Files": {
    "$LOCAL_APPDATA\\$CHROME_DIR\\Application\\chrome.exe": {"exists": true}
  }
}

we get

{
  "Files": {
    "$LOCAL_APPDATA\\$CHROME_DIR\\Application": {"exists": false},
    "$LOCAL_APPDATA\\$CHROME_DIR\\Application\\chrome.exe": {"exists": true}
  }
}

which is an impossible state. It seems that we might have to have different
merge rules for different verifiers.

[gab, 2013-09-09 16:06:38]

On 2013/09/06 20:04:44, sukolsak wrote:
> On 2013/09/06 18:27:38, gab wrote:
> > So what's up with this verifier? Are we doing it? Not saying it's high
> priority
> > just wondering?
> 
> I'm just not really sure what we want to verify (class names?) and what the
> format to this verifier would look like. Should it be something like
> 
> {
>   "$PATH\\TO\\chrome.exe": {"exists": true", classes": ["Chrome_WidgetWin_1",
> "Chrome_WidgetWin_2"]}
> }
> 
> or
> 
> {
>   "$PATH\\TO\\chrome.exe|Chrome_WidgetWin_1": {"exists": true"},
>   "$PATH\\TO\\chrome.exe|Chrome_WidgetWin_2": {"exists": true"}
> }
> ?

Classes would be nice, but I'd also be fine with simply a verifier that checks
that "a window" owned by the installed chrome.exe is up (out of all visible
windows); i.e., the input could be the same as the ProcessVerifier imo.

> 
> Each choice will have different merging behaviors.
> 
> This is unrelated, but there is also a problem in merging in the file
verifier.
> Currently, if we merge
> 
> {
>   "Files": {
>     "$LOCAL_APPDATA\\$CHROME_DIR\\Application": {"exists": false}
>   }
> }
> 
> with
> 
> {
>   "Files": {
>     "$LOCAL_APPDATA\\$CHROME_DIR\\Application\\chrome.exe": {"exists": true}
>   }
> }
> 
> we get
> 
> {
>   "Files": {
>     "$LOCAL_APPDATA\\$CHROME_DIR\\Application": {"exists": false},
>     "$LOCAL_APPDATA\\$CHROME_DIR\\Application\\chrome.exe": {"exists": true}
>   }
> }
> 
> which is an impossible state. It seems that we might have to have different
> merge rules for different verifiers.

That's fine. It's up to the writer to be smart enough to not have this scenario
imo. It will be caught easily by a test error anyways.

[gab, 2013-11-19 13:07:15]

I don't a window verifier will be implemented in the short term; it is already
implicitly checked (one of the steps of launching chrome is to wait until its
window is up to return).

Closing this issue.

Cheers,
Gab