Add session object so that we have one class that control logged in session

Add session object so that we have one class that control logged in session

[sosa, 2010-03-25 23:05:03]

Moved common code for chrome os sessions into a class.  I'll modify screensaver
and other tests after this.

[rginda, 2010-03-25 23:19:50]

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

http://codereview.chromium.org/1363002/diff/1/2#newcode104
client/bin/site_login.py:104: class ChromeOSSession(object):
How about subclassing test.test instead of object, so that tests that need to
log in could just inherit from this class?

http://codereview.chromium.org/1363002/diff/1/2#newcode128
client/bin/site_login.py:128: def __del__(self):
http://docs.python.org/reference/datamodel.html#object.__del__

__del__ may not be the right place for this code.  It's only called when the
object is actually GC'd.  It might be ok for us since we happen to be quitting
the python interpreter after each test, but according to docs "exceptions ...
are ignored".

If this were a subclass of test.test, this code could live in test.cleanup(),
otherwise I think you'll need an explicit logout() method.

[sosa, 2010-03-25 23:24:47]

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

http://codereview.chromium.org/1363002/diff/1/2#newcode104
client/bin/site_login.py:104: class ChromeOSSession(object):
I was worried about subclassing test.test.  That was my original plan but I've
down the grapevine that this is filled with issues because of hardcoded
assumptions in the harness about tests

On 2010/03/25 23:19:51, rginda wrote:
> How about subclassing test.test instead of object, so that tests that need to
> log in could just inherit from this class?

http://codereview.chromium.org/1363002/diff/1/2#newcode128
client/bin/site_login.py:128: def __del__(self):
Yeah I've been going between a logout vs. del for a while.  The concern with an
explicit logout is that if the test fails you won't logout.  I guess I could do
something like have an explicit logout and have __del__ call that as a backup. 
Would that suffice?

On 2010/03/25 23:19:51, rginda wrote:
> http://docs.python.org/reference/datamodel.html#object.__del__
> 
> __del__ may not be the right place for this code.  It's only called when the
> object is actually GC'd.  It might be ok for us since we happen to be quitting
> the python interpreter after each test, but according to docs "exceptions ...
> are ignored".
> 
> If this were a subclass of test.test, this code could live in test.cleanup(),
> otherwise I think you'll need an explicit logout() method.

[sosa, 2010-03-25 23:28:33]

Actually hold off a sec.  Lemme see if I can create a site_ui_test
subclasses of test.test

On Thu, Mar 25, 2010 at 4:24 PM,  <sosa@chromium.org> wrote:
>
> http://codereview.chromium.org/1363002/diff/1/2
> File client/bin/site_login.py (right):
>
> http://codereview.chromium.org/1363002/diff/1/2#newcode104
> client/bin/site_login.py:104: class ChromeOSSession(object):
> I was worried about subclassing test.test.  That was my original plan
> but I've down the grapevine that this is filled with issues because of
> hardcoded assumptions in the harness about tests
>
> On 2010/03/25 23:19:51, rginda wrote:
>>
>> How about subclassing test.test instead of object, so that tests that
>
> need to
>>
>> log in could just inherit from this class?
>
> http://codereview.chromium.org/1363002/diff/1/2#newcode128
> client/bin/site_login.py:128: def __del__(self):
> Yeah I've been going between a logout vs. del for a while.  The concern
> with an explicit logout is that if the test fails you won't logout.  I
> guess I could do something like have an explicit logout and have __del__
> call that as a backup.  Would that suffice?
>
> On 2010/03/25 23:19:51, rginda wrote:
>>
>> http://docs.python.org/reference/datamodel.html#object.__del__
>
>> __del__ may not be the right place for this code.  It's only called
>
> when the
>>
>> object is actually GC'd.  It might be ok for us since we happen to be
>
> quitting
>>
>> the python interpreter after each test, but according to docs
>
> "exceptions ...
>>
>> are ignored".
>
>> If this were a subclass of test.test, this code could live in
>
> test.cleanup(),
>>
>> otherwise I think you'll need an explicit logout() method.
>
> http://codereview.chromium.org/1363002
>

[sosa, 2010-03-25 23:49:44]

Incorporated your changes and it seems to work great!  Much simpler too, thanks
rob

[rginda, 2010-03-26 00:09:00]

Cool, LGTM

On 2010/03/25 23:49:44, sosa wrote:
> Incorporated your changes and it seems to work great!  Much simpler too,
thanks
> rob

[Chris Masone, 2010-03-26 00:13:44]

lgtm

On 2010/03/26 00:09:00, rginda wrote:
> Cool, LGTM
> 
> On 2010/03/25 23:49:44, sosa wrote:
> > Incorporated your changes and it seems to work great!  Much simpler too,
> thanks
> > rob

[kmixter1, 2010-03-26 02:38:45]

http://codereview.chromium.org/1363002/diff/17001/18002
File client/bin/site_ui_test.py (right):

http://codereview.chromium.org/1363002/diff/17001/18002#newcode32
client/bin/site_ui_test.py:32: if not site_login.attempt_login(self, script):
would be nice if we waited for cryptohome here.  I would think every test would
want to be started when chrome really is up and ready.

http://codereview.chromium.org/1363002/diff/17001/18002#newcode41
client/bin/site_ui_test.py:41: raise error.TestFail('Could not logout at end of
session')
would be nice if we wait for the login manager here - just in case another
client test is starting soon after this that expects the login manager to up.

http://codereview.chromium.org/1363002/diff/17001/18004
File client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py (right):

http://codereview.chromium.org/1363002/diff/17001/18004#newcode13
client/site_tests/desktopui_DoLogin/desktopui_DoLogin.py:13: time.sleep(10) #
Local login is so fast, it needs to be slowed down.
Adding the blocking call to the constructor should remove the need for this.

[Chris Masone, 2010-03-26 02:42:19]

http://codereview.chromium.org/1363002/diff/17001/18002
File client/bin/site_ui_test.py (right):

http://codereview.chromium.org/1363002/diff/17001/18002#newcode32
client/bin/site_ui_test.py:32: if not site_login.attempt_login(self, script):
On 2010/03/26 02:38:45, kmixter1 wrote:
> would be nice if we waited for cryptohome here.  I would think every test
would
> want to be started when chrome really is up and ready.

waiting for cryptohome is no guarantee that Chrome is up and ready.  In fact, we
wait for a file that's output by an upstart job that runs when
start-user-session is emitted on Chrome's behalf -- something that happens
strictly after the cryptohome is mounted.