switch to autox.py and robustify login/logout code

switch to autox.py and robustify login/logout code

This CL switches from using the autox binary to using autox.py.  This includes backwards
incompatible changes to site_login.py, and I haven't had a chance to fix the affected callsites
yet.

I've also made the login/logout code a little more robust.  Now it'll make sure that the login
manager is running under a NEW pid, that X is running, and that at a window is visible before
assuming we're ready to log in.

All of the wait loops have been refactored into wait_for(...), which spits out logging.info()
messages that could be parsed later to determine how long the operations are actually taking.
Perhaps this could make it into a different, non-login specific library soonish.

I'm out of the office for the next few days, but wanted to get this out there before the
trunk totally passed it by.  I'll return on Wednesday to finish the job.

[Chris Masone, 2010-03-28 06:50:09]

LGTM, with changes below

http://codereview.chromium.org/1534001/diff/1/3
File client/bin/site_login.py (right):

http://codereview.chromium.org/1534001/diff/1/3#newcode35
client/bin/site_login.py:35: Returns True when the session manager appears under
a pid other than
It's more that it returns true _if_ the session_manager _has appeared_

The comment seems to imply that it loops and waits til this condition is true.

http://codereview.chromium.org/1534001/diff/1/3#newcode105
client/bin/site_login.py:105: desc="session_manager recycle")
If we're going to cycle the session manager anyway, it might be better to
SIGTERM session_manager.  That also causes a graceful exit, and it'll SIGTERM
chrome in the process anyway.

[sosa, 2010-03-29 18:02:09]

Before you commit this, re-run the Build Verify suite.  By changing SiteUI
you'll already be breaking other tests because you changed the interface to
initialize

[Daniel Erat, 2010-03-29 18:33:50]

On 2010/03/29 18:02:09, sosa wrote:
> Before you commit this, re-run the Build Verify suite.  By changing SiteUI
> you'll already be breaking other tests because you changed the interface to
> initialize

Note that I have a change that has some overlap with this that I'm hoping to
check in soon, at http://codereview.chromium.org/1565001/show .  Sorry, I didn't
see this change until cmasone mentioned it this morning. :-(

Mine adds a poll_for_condition() function that's pretty similar to what you're
adding, with the change that it takes an Exception and throws it if the timeout
is reached instead of returning True/False -- this is nice because it saves a
bunch of duplicated code within the tests themselves.  It's also inside of a new
site_utils library, so that tests can call it directly.  (I didn't do anything
related to swapping out the old autox for autox.py, though.)

I've updated a bunch of tests to deal my CL's changes.  Robert, are you
completely offline until Wednesday (if so, answer with silence :-) ) or just
unable to finish this change and check it in before then?  Let me know if you
want to discuss the best way to merge these.

[Daniel Erat, 2010-03-30 00:21:48]

On 2010/03/29 18:33:50, Daniel Erat wrote:
> On 2010/03/29 18:02:09, sosa wrote:
> > Before you commit this, re-run the Build Verify suite.  By changing SiteUI
> > you'll already be breaking other tests because you changed the interface to
> > initialize
> 
> Note that I have a change that has some overlap with this that I'm hoping to
> check in soon, at http://codereview.chromium.org/1565001/show .  Sorry, I
didn't
> see this change until cmasone mentioned it this morning. :-(
> 
> Mine adds a poll_for_condition() function that's pretty similar to what you're
> adding, with the change that it takes an Exception and throws it if the
timeout
> is reached instead of returning True/False -- this is nice because it saves a
> bunch of duplicated code within the tests themselves.  It's also inside of a
new
> site_utils library, so that tests can call it directly.  (I didn't do anything
> related to swapping out the old autox for autox.py, though.)
> 
> I've updated a bunch of tests to deal my CL's changes.  Robert, are you
> completely offline until Wednesday (if so, answer with silence :-) ) or just
> unable to finish this change and check it in before then?  Let me know if you
> want to discuss the best way to merge these.

I checked my change in as 3e3f7f4041a9b0d4f2afc0c80f7fefe03ef5563e.  Let me know
if there's anything that I can do to help with the merge.

[rginda, 2010-04-02 18:01:38]

Here's a new patch set post 1565001.  I've run the tests that were modified as
part of the patch, and they seem to pass except for some issues related to a
stale image on my test hardware.  I've run into GPT issues updating the
hardware, so it'll be a little longer until I can run all the tests against the
tip.

[Daniel Erat, 2010-04-03 00:54:38]

This is a nice cleanup.  Thanks for doing it!

Can client/deps/autox be deleted too now?

http://codereview.chromium.org/1534001/diff/7001/8002
File client/bin/site_login.py (right):

http://codereview.chromium.org/1534001/diff/7001/8002#newcode32
client/bin/site_login.py:32: if ary:
this can all just be:

  return int(ary[0]) if ary else None

(feel free to ignore if it looks too weird)

http://codereview.chromium.org/1534001/diff/7001/8002#newcode57
client/bin/site_login.py:57: wid = ax.get_top_window_id_at_point(0, 0)
What's the reason for the a-window-is-displayed check?

http://codereview.chromium.org/1534001/diff/7001/8002#newcode88
client/bin/site_login.py:88: raise UnexpectedCondition("Login manager is not
running")
s/Login/Session/ ?  Do we need some other way to check that we're at the login
prompt?  The session manager is running even when the user is already logged in.
 (Unfortunately, Chrome is still running with a --login-manager argument at this
point, even though it's no longer functioning as a login manager.)

http://codereview.chromium.org/1534001/diff/7001/8002#newcode95
client/bin/site_login.py:95: ax.send_hotkey("Ctrl+Alt+L")
So this locks the screen first in case the user is already logged in?  The
comment should be updated, at least.

http://codereview.chromium.org/1534001/diff/7001/8002#newcode107
client/bin/site_login.py:107: logged_in, TimeoutError('Timed out waiting for
login'),
Whoops, nice catch re the unnecessary lambda.  I suck at Python. :-(

http://codereview.chromium.org/1534001/diff/7001/8002#newcode126
client/bin/site_login.py:126: # Gracefully exiting chrome causes the user's
session to end.
Update the comment, since you're killing the session manager rather than Chrome
now?

http://codereview.chromium.org/1534001/diff/7001/8003
File client/bin/site_ui_test.py (right):

http://codereview.chromium.org/1534001/diff/7001/8003#newcode36
client/bin/site_ui_test.py:36: def __is_screensaver(self, status):
add docstring describing 'status' (I think you can probably switch the command
line to use egrep instead so you don't need to backslash-escape pipes)

http://codereview.chromium.org/1534001/diff/7001/8003#newcode83
client/bin/site_ui_test.py:83: If self.auto_login is True, this will
automatically log in using the
nit: the style guide shows the order of stuff in the docstring as:

  one-line description

  detailed description

  args

  returns

  raises

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

http://codereview.chromium.org/1534001/diff/7001/8012
File client/site_tests/login_CryptohomeUnmounted/login_CryptohomeUnmounted.py
(right):

http://codereview.chromium.org/1534001/diff/7001/8012#newcode12
client/site_tests/login_CryptohomeUnmounted/login_CryptohomeUnmounted.py:12: def
run_once(self, is_control = False):
not your fault, but the last arg should just be "is_control=False", with no
spaces

[rginda, 2010-04-05 16:44:28]

http://codereview.chromium.org/1534001/diff/7001/8002
File client/bin/site_login.py (right):

http://codereview.chromium.org/1534001/diff/7001/8002#newcode32
client/bin/site_login.py:32: if ary:
On 2010/04/03 00:54:39, Daniel Erat wrote:
> this can all just be:
> 
>   return int(ary[0]) if ary else None
> 
> (feel free to ignore if it looks too weird)

It does look a little wonky, but I'll give it a shot.

http://codereview.chromium.org/1534001/diff/7001/8002#newcode57
client/bin/site_login.py:57: wid = ax.get_top_window_id_at_point(0, 0)
On 2010/04/03 00:54:39, Daniel Erat wrote:
> What's the reason for the a-window-is-displayed check?

When the session manager starts up there is a moment where we can make a
connection with autox, but there is no window displayed.  If we start sending
keystrokes at this point they get lost.  If we wait for this window to show up,
things go much smoother.

(Added that comment to the code.)

http://codereview.chromium.org/1534001/diff/7001/8002#newcode88
client/bin/site_login.py:88: raise UnexpectedCondition("Login manager is not
running")
On 2010/04/03 00:54:39, Daniel Erat wrote:
> s/Login/Session/ ?  Do we need some other way to check that we're at the login
> prompt?  The session manager is running even when the user is already logged
in.
>  (Unfortunately, Chrome is still running with a --login-manager argument at
this
> point, even though it's no longer functioning as a login manager.)

Something more definitive might be nice, but I think I added this in as a sanity
check.  Before __session_manager_restarted() got so careful I had to ensure that
the session manager wasn't in the middle of a restart.

http://codereview.chromium.org/1534001/diff/7001/8002#newcode95
client/bin/site_login.py:95: ax.send_hotkey("Ctrl+Alt+L")
On 2010/04/03 00:54:39, Daniel Erat wrote:
> So this locks the screen first in case the user is already logged in?  The
> comment should be updated, at least.

Actually this makes sure that the session manager is at the username/password
screen.  http://codereview.chromium.org/1390003 this accelerator and the
alt+u,alt+p so that this code could be sure it was driving the correct screen.

http://codereview.chromium.org/1534001/diff/7001/8002#newcode126
client/bin/site_login.py:126: # Gracefully exiting chrome causes the user's
session to end.
On 2010/04/03 00:54:39, Daniel Erat wrote:
> Update the comment, since you're killing the session manager rather than
Chrome
> now?

Done.

http://codereview.chromium.org/1534001/diff/7001/8003
File client/bin/site_ui_test.py (right):

http://codereview.chromium.org/1534001/diff/7001/8003#newcode36
client/bin/site_ui_test.py:36: def __is_screensaver(self, status):
On 2010/04/03 00:54:39, Daniel Erat wrote:
> add docstring describing 'status' (I think you can probably switch the command
> line to use egrep instead so you don't need to backslash-escape pipes)

Done.

http://codereview.chromium.org/1534001/diff/7001/8003#newcode83
client/bin/site_ui_test.py:83: If self.auto_login is True, this will
automatically log in using the
On 2010/04/03 00:54:39, Daniel Erat wrote:
> nit: the style guide shows the order of stuff in the docstring as:
> 
>   one-line description
> 
>   detailed description
> 
>   args
> 
>   returns
> 
>   raises
> 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...

Done.

http://codereview.chromium.org/1534001/diff/7001/8012
File client/site_tests/login_CryptohomeUnmounted/login_CryptohomeUnmounted.py
(right):

http://codereview.chromium.org/1534001/diff/7001/8012#newcode12
client/site_tests/login_CryptohomeUnmounted/login_CryptohomeUnmounted.py:12: def
run_once(self, is_control = False):
On 2010/04/03 00:54:39, Daniel Erat wrote:
> not your fault, but the last arg should just be "is_control=False", with no
> spaces

Done.

[stevenjb, 2010-04-05 21:16:53]

The following tests passed with the changes in Patch 4 on my netbook:

./run_remote_tests.sh --remote=172.22.70.34 desktopui_DoLogin
./run_remote_tests.sh --remote=172.22.70.34 desktopui_FailedLogin
./run_remote_tests.sh --remote=172.22.70.34 desktopui_ScreenSaverUnlock
./run_remote_tests.sh --remote=172.22.70.34 desktopui_WindowManagerHotkeys
./run_remote_tests.sh --remote=172.22.70.34 login_CryptohomeUnmounted
./run_remote_tests.sh --remote=172.22.70.34 platform_ProcessPrivileges

The following failed both with the patch and without (i.e. master fails):
./run_remote_tests.sh --remote=172.22.70.34 login_ChromeProfileSanitary
./run_remote_tests.sh --remote=172.22.70.34 login_LogoutProcessCleanup

[Daniel Erat, 2010-04-05 22:15:45]

LGTM if everything passes now, but please also see my question from the last
round ("Can client/deps/autox be deleted too now?").

http://codereview.chromium.org/1534001/diff/19001/20007
File client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py (right):

http://codereview.chromium.org/1534001/diff/19001/20007#newcode12
client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py:12: def run_once(self):
it's possible (likely) that rietveld is just messing up, but it seems to be
displaying this line as having a bunch of trailing spaces

http://codereview.chromium.org/1534001/diff/19001/20011
File
client/site_tests/login_ChromeProfileSanitary/login_ChromeProfileSanitary.py
(right):

http://codereview.chromium.org/1534001/diff/19001/20011#newcode52
client/site_tests/login_ChromeProfileSanitary/login_ChromeProfileSanitary.py:52:
def run_once(self, timeout = 10):
please fix the spacing here as well (should be "timeout=10")

[rginda, 2010-04-05 23:16:42]

On 2010/04/05 22:15:45, Daniel Erat wrote:
> LGTM if everything passes now, but please also see my question from the last
> round ("Can client/deps/autox be deleted too now?").
> 

Oops, I forgot about that one.  Yeah, we can remove that now.

http://codereview.chromium.org/1534001/diff/19001/20007
File client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py (right):

http://codereview.chromium.org/1534001/diff/19001/20007#newcode12
client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py:12: def run_once(self):
On 2010/04/05 22:15:45, Daniel Erat wrote:
> it's possible (likely) that rietveld is just messing up, but it seems to be
> displaying this line as having a bunch of trailing spaces

Not sure what that's about, my source file doesn't have any trailing space here.
 Maybe I removed some trailing space?

[rginda, 2010-04-07 20:56:00]

I've finally got a working build again and was able to rerun the tests. 
ChromeProfieSanitary still fails with a "Never received callback from browser",
but I think it's a problem with the way the testcase works and not with the
autotest code.

"LogoutProcessCleanup" also fails because it finds two pids that are owned by
chronos but not parented by the session manager.  This also appears to be a
problem with the testcase, or an actual bug in the image, but not an issue with
autox.

My previous patchset had a small issue in ChromeProfileSanitary with leftover
reference to a 'script' variable.

I also had to add an additional check to __session_manager_restarted, since
keystrokes were being sent before the session manager was really ready to hear
them.

I'm running the full suite_BuildVerify now, and assuming I don't find any more
regressions I'll push the CL tonight.

[Daniel Erat, 2010-04-07 21:29:04]

Sounds great.  Thanks for pushing this through!

On Wed, Apr 7, 2010 at 1:56 PM,  <rginda@chromium.org> wrote:
> I've finally got a working build again and was able to rerun the tests.
> ChromeProfieSanitary still fails with a "Never received callback from
> browser",
> but I think it's a problem with the way the testcase works and not with the
> autotest code.
>
> "LogoutProcessCleanup" also fails because it finds two pids that are owned
> by
> chronos but not parented by the session manager.  This also appears to be a
> problem with the testcase, or an actual bug in the image, but not an issue
> with
> autox.
>
> My previous patchset had a small issue in ChromeProfileSanitary with
> leftover
> reference to a 'script' variable.
>
> I also had to add an additional check to __session_manager_restarted, since
> keystrokes were being sent before the session manager was really ready to
> hear
> them.
>
> I'm running the full suite_BuildVerify now, and assuming I don't find any
> more
> regressions I'll push the CL tonight.
>
> http://codereview.chromium.org/1534001/show
>

[rginda, 2010-04-07 21:56:45]

No problem, thanks for the reviews.

buildverify looked good, I've landed the CL.

On 2010/04/07 21:29:04, Daniel Erat wrote:
> Sounds great.  Thanks for pushing this through!
> 
> On Wed, Apr 7, 2010 at 1:56 PM,  <mailto:rginda@chromium.org> wrote:
> > I've finally got a working build again and was able to rerun the tests.
> > ChromeProfieSanitary still fails with a "Never received callback from
> > browser",
> > but I think it's a problem with the way the testcase works and not with the
> > autotest code.
> >
> > "LogoutProcessCleanup" also fails because it finds two pids that are owned
> > by
> > chronos but not parented by the session manager. &nbsp;This also appears to
be a
> > problem with the testcase, or an actual bug in the image, but not an issue
> > with
> > autox.
> >
> > My previous patchset had a small issue in ChromeProfileSanitary with
> > leftover
> > reference to a 'script' variable.
> >
> > I also had to add an additional check to __session_manager_restarted, since
> > keystrokes were being sent before the session manager was really ready to
> > hear
> > them.
> >
> > I'm running the full suite_BuildVerify now, and assuming I don't find any
> > more
> > regressions I'll push the CL tonight.
> >
> > http://codereview.chromium.org/1534001/show
> >
>