Closed Bug 942826 Opened 7 years ago Closed 7 years ago

Move the reference workload population into the tests

Categories

(Testing Graveyard :: Eideticker, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: wlach)

References

Details

Attachments

(1 file, 3 obsolete files)

At the moment the tests rely on a reference workload being pushed to the device using b2gpopulate. Since version 0.10 b2gpopulate can be used as a Python module in addition to the command line, so we can move the population of the relevant workload into the tests themselves. This means we're only populating what we need for the current test, and anyone running the same version of the test will be using the same reference workload.
I have a work in progress patch, but I need to do some more testing before it's ready for review.
This patch changes things around a little, and allows tests to populate files and databases using b2gpopulate. I have tested various scripts successfully, but only for b2g. I don't expect any fallout for android.

One issue with this approach is that when we run scripts with --num-runs the entire test script is repeated including the population of the workload. Ideally we will only do this once. I'm interested to hear suggestions for how I could do this, or perhaps it could be addressed in a later patch.

I also fixed an issue in this patch when attempting to launch an application that has the same name as a everything.me category. We were sending a tap to a negative x-axis, which I would suggest orangutan should throw an exception rather than just fail silently.
Attachment #8341151 - Flags: feedback?(wlachance)
Comment on attachment 8341151 [details] [diff] [review]
Move the reference workload population into the tests. v1.0

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

Great stuff. I think we can address the "populate database multiple times" issue by breaking out a "prepare_test" method in runtest.py and putting that kind of logic there.

::: src/eideticker/eideticker/runtest.py
@@ +106,5 @@
> +            test.populate_databases()
> +
> +        device.start_b2g()
> +
> +        # TODO connect to network if test requires it

This will obviously need to be addressed before this lands. Perhaps we could add a "requiresNetwork" attribute to each test?

::: src/tests/b2g/applaunching/gallery.py
@@ +31,5 @@
> +                if len(items) == picture_count and not progress.is_displayed():
> +                    break
> +            except (NoSuchElementException, StaleElementException):
> +                pass
> +            time.sleep(0.5)

I would decrease this to 0.1 at least.
Attachment #8341151 - Flags: feedback?(wlachance) → feedback+
(In reply to William Lachance (:wlach) from comment #3)
> Comment on attachment 8341151 [details] [diff] [review]
> Move the reference workload population into the tests. v1.0
> 
> Review of attachment 8341151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great stuff. I think we can address the "populate database multiple times"
> issue by breaking out a "prepare_test" method in runtest.py and putting that
> kind of logic there.

Do you think this should be addressed in this patch, or in a follow-up?

> ::: src/eideticker/eideticker/runtest.py
> @@ +106,5 @@
> > +            test.populate_databases()
> > +
> > +        device.start_b2g()
> > +
> > +        # TODO connect to network if test requires it
> 
> This will obviously need to be addressed before this lands. Perhaps we could
> add a "requiresNetwork" attribute to each test?

There are no tests that currently require a network connection (are there?) so I'm not sure this blocks the patch. I would also avoid creating any tests that require a network as doing so will introduce latency based on the connection.

> ::: src/tests/b2g/applaunching/gallery.py
> @@ +31,5 @@
> > +                if len(items) == picture_count and not progress.is_displayed():
> > +                    break
> > +            except (NoSuchElementException, StaleElementException):
> > +                pass
> > +            time.sleep(0.5)
> 
> I would decrease this to 0.1 at least.

I was basing this on the default polling interval for explicit waits in WebDriver (and soon Marionette -- see bug 850881). At this point we're not recording video so I don't think the half-second will make much difference. I'm happy to change it though.
Flags: needinfo?(wlachance)
(In reply to Dave Hunt (:davehunt) from comment #4)
> (In reply to William Lachance (:wlach) from comment #3)
> > Comment on attachment 8341151 [details] [diff] [review]
> > Move the reference workload population into the tests. v1.0
> > 
> > Review of attachment 8341151 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Great stuff. I think we can address the "populate database multiple times"
> > issue by breaking out a "prepare_test" method in runtest.py and putting that
> > kind of logic there.
> 
> Do you think this should be addressed in this patch, or in a follow-up?

Well, I really want to get this patch in ASAP so it can go in a follow up if it helps get this done faster. Really I think this change should be fairly straightforward though.

> > ::: src/eideticker/eideticker/runtest.py
> > @@ +106,5 @@
> > > +            test.populate_databases()
> > > +
> > > +        device.start_b2g()
> > > +
> > > +        # TODO connect to network if test requires it
> > 
> > This will obviously need to be addressed before this lands. Perhaps we could
> > add a "requiresNetwork" attribute to each test?
> 
> There are no tests that currently require a network connection (are there?)
> so I'm not sure this blocks the patch. I would also avoid creating any tests
> that require a network as doing so will introduce latency based on the
> connection.

The web tests (which we don't currently run) do require a network connection. I guess as an interim measure you could check the class of the test here. Something like:

from eideticker.test import B2GWebTest # put this on top
if type(test) == eideticker.B2GWebTest:
  # .. setup network ..

> > ::: src/tests/b2g/applaunching/gallery.py
> > @@ +31,5 @@
> > > +                if len(items) == picture_count and not progress.is_displayed():
> > > +                    break
> > > +            except (NoSuchElementException, StaleElementException):
> > > +                pass
> > > +            time.sleep(0.5)
> > 
> > I would decrease this to 0.1 at least.
> 
> I was basing this on the default polling interval for explicit waits in
> WebDriver (and soon Marionette -- see bug 850881). At this point we're not
> recording video so I don't think the half-second will make much difference.
> I'm happy to change it though.

I suspect the WebDriver value is too high as well then. ;) It's more a "why wait longer than really needed" thing. 0.5 seconds here, 0.5 seconds there adds up to longer test cycles. I'm actually not sure if a wait is needed at all. Generally you only add those into a polling loop to prevent the CPU from spinning like crazy, which I'm not sure would be a problem here. 0.1 should be more than safe, anyway.
Flags: needinfo?(wlachance)
Here's a version of the patch I've been hacking on. I seperated out the logic to prepare the app for the test from running the test, and made it possible to optionally disable that via command-line argument (useful for testing). I also updated stuff to work with the latest version of gaiatest and did some other things to consolidate duplicated code, etc. Feedback appreciated. :)
Assignee: dave.hunt → wlachance
Attachment #8341151 - Attachment is obsolete: true
Attachment #8347526 - Flags: feedback?(dave.hunt)
Updated for bitrot
Attachment #8347526 - Attachment is obsolete: true
Attachment #8347526 - Flags: feedback?(dave.hunt)
Attachment #8348287 - Flags: feedback?(dave.hunt)
Comment on attachment 8348287 [details] [diff] [review]
0001-Bug-942826-Put-test-population-preparation-in-a-sepe.patch

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

Looks great, thanks for picking this up Will!

::: src/eideticker/eideticker/device.py
@@ +407,5 @@
>  
> +    def cleanup(self):
> +        self.removeDir('/data/local/storage/persistent')
> +        self.removeDir('/data/b2g/mozilla')
> +        self.shellCheckOutput(['rm', '-r', '/sdcard/*'])

This turned out to not be sufficient in gaiatest, we now list all files and remove them one at a time. See https://github.com/mozilla-b2g/gaia/blob/545aacf3feff6430140cc9ade757002df4895b77/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L827

Do you think it would be worth moving the cleanup_data and cleanup_sdcard to the GaiaDevice class, so they can be called from consumers of gaiatest?

::: src/eideticker/eideticker/runtest.py
@@ +26,5 @@
> +        device.stopB2G()
> +        device.cleanup()
> +        device.startB2G()
> +
> +        if hasattr(test, 'populate_databases'):

This should be done before starting B2G up.

::: src/eideticker/setup.py
@@ +3,4 @@
>      name = "eideticker",
>      version = "0.1.0",
>      packages = find_packages(),
> +    install_requires = [ 'mozdevice', 'BeautifulSoup', 'gaiatest', 'httplib2', 'b2gpopulate>=0.10' ]

We should probably pin gaiatest to the current latest.
Attachment #8348287 - Flags: feedback?(dave.hunt) → feedback+
This makes sure that the results between different runs of the harness are
consistent and not dependant on what state the device was in when the test
was run.

(based on an earlier patch by Dave Hunt)
Attachment #8355661 - Flags: review?(dave.hunt)
Attachment #8348287 - Attachment is obsolete: true
Uploaded a patch.

(In reply to Dave Hunt (:davehunt) from comment #8)
> ::: src/eideticker/eideticker/device.py
> @@ +407,5 @@
> >  
> > +    def cleanup(self):
> > +        self.removeDir('/data/local/storage/persistent')
> > +        self.removeDir('/data/b2g/mozilla')
> > +        self.shellCheckOutput(['rm', '-r', '/sdcard/*'])
> 
> This turned out to not be sufficient in gaiatest, we now list all files and
> remove them one at a time. See
> https://github.com/mozilla-b2g/gaia/blob/
> 545aacf3feff6430140cc9ade757002df4895b77/tests/python/gaia-ui-tests/gaiatest/
> gaia_test.py#L827

I did this, but why is this necessary? I would think an rm -r should take care of it, no? I traced the change back to the original bug but didn't see any mention of why it was made.

> Do you think it would be worth moving the cleanup_data and cleanup_sdcard to
> the GaiaDevice class, so they can be called from consumers of gaiatest?

I guess it makes sense to consolidate the code if we're using it in more than one place. To be honest I am not 100% sure about the idea of GaiaDevice as it seems to mix up a bunch of mozdevice and marionette stuff in one place, which I've had bad experiences with in the past with mozb2g. But I don't have an alternative proposal at present.

> ::: src/eideticker/eideticker/runtest.py
> @@ +26,5 @@
> > +        device.stopB2G()
> > +        device.cleanup()
> > +        device.startB2G()
> > +
> > +        if hasattr(test, 'populate_databases'):
> 
> This should be done before starting B2G up.

Fixed.

> ::: src/eideticker/setup.py
> @@ +3,4 @@
> >      name = "eideticker",
> >      version = "0.1.0",
> >      packages = find_packages(),
> > +    install_requires = [ 'mozdevice', 'BeautifulSoup', 'gaiatest', 'httplib2', 'b2gpopulate>=0.10' ]
> 
> We should probably pin gaiatest to the current latest.

I set up a >= requirement in setup.py. I'm not sure if pinning is the best idea since we won't pick up bugfixes that way, but I could be convinced otherwise.
Comment on attachment 8355661 [details] [diff] [review]
Move test population / preparation into Eideticker;r=davehunt

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

::: src/eideticker/eideticker/b2gtestmixins.py
@@ +8,5 @@
> +from marionette.errors import NoSuchElementException
> +from marionette.errors import StaleElementException
> +from marionette.errors import TimeoutException
> +
> +class B2GContactsTestMixin(object):

This appears to be unused.

::: src/eideticker/eideticker/device.py
@@ +408,5 @@
> +    def cleanup(self):
> +        self.removeDir('/data/local/storage/persistent')
> +        self.removeDir('/data/b2g/mozilla')
> +        for item in self.listFiles('/sdcard/'):
> +            self.device.manager.removeDir('/'.join(['/sdcard', item]))

This was needed to delete all hidden files and folders. When gallery is used, for example a .gallery folder is created for hidden generated thumbnails. Also, when the sdcard is already empty (as can be the case for emulator builds) the rm -r sdcard/* failed.

@@ +415,1 @@
>          #restart b2g so we start with a clean slate

Nit: We should probably update or remove this comment

::: src/eideticker/setup.py
@@ +2,5 @@
>  setup(
>      name = "eideticker",
>      version = "0.1.0",
>      packages = find_packages(),
> +    install_requires = [ 'mozdevice', 'BeautifulSoup', 'gaiatest>=0.21.2', 'httplib2', 'b2gpopulate>=0.10' ]

I think >= should be okay. Note that a new version of gaiatest may carry a new version of the Marionette client as a dependency, so it's possible a breaking change in either could cause issues with Eideticker.

::: src/tests/b2g/appscrolling/contacts.py
@@ +23,5 @@
> +        # launch the contacts app and wait for the contacts to be displayed,
> +        # the first launch after populating the data takes a long time.
> +        contacts = Contacts(self.device.marionette)
> +        contacts.launch()
> +        end_time = time.time() + 120

With the latest Marionette client we can now use waits:

from marionette.wait import Wait
Wait(self.device.marionette, 120, ignored_exceptions=(NoSuchElementException, ElementNotVisibleException)).until(lambda m: m.find_element(*contacts._contact_locator).is_displayed())

::: src/tests/b2g/appscrolling/messages.py
@@ +35,5 @@
> +        # launch the messages app and wait for the messages to be displayed,
> +        # the first launch after populating the data takes a long time.
> +        messages = Messages(self.device.marionette)
> +        messages.launch()
> +        end_time = time.time() + 120

As mentioned, we could now use a Marionette wait.
Attachment #8355661 - Flags: review?(dave.hunt) → review+
Ok pushed:

https://github.com/mozilla/eideticker/commit/8b4f59b560e746eb2b1dd80c67e7776e03d2f3f7
https://github.com/mozilla/eideticker/commit/55294ae734852d6f9cbc86ce194c0aeda7f586da

Note there were some changes that I forgot to add to the patch (hence the unused stuff in b2gtestmixins.py). There were also some minor problems with removing startB2G() which I only noticed in testing... b2gpopulate depends on a marionette instance, so we need to initialize marionette in b2g constructor even if we're not using it. I think we're good now.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.