1002502 - Update launch end criteria for all B2G tests

Status: RESOLVED FIXED

(Testing Graveyard :: Eideticker, defect, P2)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

In most instances we currently wait 5 seconds after launching an application [1], however we may reach a stable state before (or even after) then. We should add finished state conditions for each app. If we then remove the 5 second sleep for tests that have their own wait_for_finish method, then we'll save time on processing the captured video for the times that we become stable before the 5 seconds elapse.

[1] https://github.com/mozilla/eideticker/blob/71beece88bec173d9d787c06a0aaf02a5dcd2873/src/eideticker/eideticker/test.py#L585

Comment 1

[William Lachance (:wlach)]

Yes, I think the way forward here is to define a default implementation for wait_for_finish which does the sleeping, then override that in each test with some custom logic when the test is done loading. Let's get to work on this.

Eventually we should just be able to wait for the 'content-ready' event to be sent from the app when it finishes loading (see bug 996038).

Comment 2

[William Lachance (:wlach)]

Attachment: Initial patch to do proper waiting in contacts + marketplace

Here's a patch which begins the work to refactor tests, etc. so this works properly. Currently works properly for marketplace and contacts, other tests fall back to old behaviour. We'll need to add a per-app wait_for_finish() method to them. 

Let me know if you want to do that, if you don't have time, I can. :)

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8415643 [details] [diff] [review]
Initial patch to do proper waiting in contacts + marketplace

Review of attachment 8415643 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking care of this. With the suggested changes this looks good to me. I'd be happy to work on wait_for_finish implementations for the remaining launch tests, or this might be a good mentored bug, depending on how quickly we want this done.

::: src/eideticker/eideticker/b2gtestmixins.py
@@ +32,5 @@
> +        contacts = Contacts(self.device.marionette)
> +
> +        Wait(self.device.marionette).until(
> +            lambda m: apps.displayed_app.name.lower() == 'contacts')
> +        apps.switch_to_displayed_app()

The contacts.launch() method should take care of a basic wait and switch to the app, so all we should need here is the wait for the first contact to be displayed.

@@ +38,4 @@
>          Wait(self.device.marionette, 120, ignored_exceptions=(
>              NoSuchElementException, ElementNotVisibleException)).until(
>              lambda m: m.find_element(
> +                    *contacts._contact_locator).is_displayed())

Nit: Why the change in indentation?

::: src/eideticker/eideticker/test.py
@@ +585,4 @@
>          self.execute_actions([['tap', tap_x, tap_y]],
>                               test_finished_after_actions=False)
>  
> +        self.wait_for_finish()

I was thinking about this, and do you think we should add a small sleep after wait_for_finish (if we're not already sleeping)? My concern is that if we immediately stop the capture there's potentially very few 'stable' frames after the final visible change. I would guess a single second would be plenty, but I haven't run any tests on this.

Comment 4

[William Lachance (:wlach)]

(In reply to Dave Hunt (:davehunt) from comment #3)
> Comment on attachment 8415643 [details] [diff] [review]
> Initial patch to do proper waiting in contacts + marketplace
> 
> Review of attachment 8415643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking care of this. With the suggested changes this looks good
> to me. I'd be happy to work on wait_for_finish implementations for the
> remaining launch tests, or this might be a good mentored bug, depending on
> how quickly we want this done.

I think this work is time critical enough that we probably just want to do it ourselves. It's also a bit complicated/involved to be a mentored bug IMO.

> ::: src/eideticker/eideticker/b2gtestmixins.py
> @@ +32,5 @@
> > +        contacts = Contacts(self.device.marionette)
> > +
> > +        Wait(self.device.marionette).until(
> > +            lambda m: apps.displayed_app.name.lower() == 'contacts')
> > +        apps.switch_to_displayed_app()
> 
> The contacts.launch() method should take care of a basic wait and switch to
> the app, so all we should need here is the wait for the first contact to be
> displayed.

Ah, but we aren't actually launching the app in the startup test -- we're simulating a "press" on the icon. So we do need this, in fact.

> @@ +38,4 @@
> >          Wait(self.device.marionette, 120, ignored_exceptions=(
> >              NoSuchElementException, ElementNotVisibleException)).until(
> >              lambda m: m.find_element(
> > +                    *contacts._contact_locator).is_displayed())
> 
> Nit: Why the change in indentation?

Will fix.

> ::: src/eideticker/eideticker/test.py
> @@ +585,4 @@
> >          self.execute_actions([['tap', tap_x, tap_y]],
> >                               test_finished_after_actions=False)
> >  
> > +        self.wait_for_finish()
> 
> I was thinking about this, and do you think we should add a small sleep
> after wait_for_finish (if we're not already sleeping)? My concern is that if
> we immediately stop the capture there's potentially very few 'stable' frames
> after the final visible change. I would guess a single second would be
> plenty, but I haven't run any tests on this.

Good point, will fix.

Comment 5

[William Lachance (:wlach)]

Pushed patch with suggested changes: https://github.com/mozilla/eideticker/commit/a62ddfb8f5588ba25c3fe516296afcd9dd7206e4

Now it's up to Dave Hunt to finish the rest. :)

Comment 6

[William Lachance (:wlach)]

Hey Dave, are you still working on this? I was reminded about this when I filed bug 1018334.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I have a patch, but it's blocked on bug 1017484 as it'll highly likely be branch specific.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Update launch end criteria for all B2G tests. v1.0

I tested this patch against gaia v1.3 using gaiatest-v1.3 and gaia master using gaiatest. All tests passed for me. I suspect at some point that we'll need to add a Gaia compatability layer for cross branch support, but for now we seem to get away without the need.

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8442696 [details] [diff] [review]
Update launch end criteria for all B2G tests. v1.0

Review of attachment 8442696 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for this! Just a few small comments.

::: src/eideticker/eideticker/b2gtestmixins.py
@@ +12,2 @@
>      def launch_app(self):
> +        from gaiatest.apps.contacts.app import Contacts

Could you put this back on top? I generally prefer to import things that way as it detects problems earlier.

@@ +22,5 @@
>      def prepare_app(self):
>          self.launch_app()
>  
> +    def wait_for_content_ready(self):
> +        from gaiatest.apps.contacts.app import Contacts

Likewise

::: src/tests/b2g/applaunching/browser-startup.ini
@@ +1,1 @@
> +[browser.py]

It would probably be good to just merge this into a single test manifest file, someday (doesn't need to be now). I seperated out various tests into their own ini files originally because I wanted to use the same program for multiple apps (which manifestdestiny can't handle), but we're not doing that anymore.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed in:
https://github.com/mozilla/eideticker/commit/43449aca0782b7d6a98705bf35842790243709e4