1212608 - Move parts of firefox-ui-tests harness into Marionette test runner

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Many elements of the current firefox-ui-tests harness are suitable for generic Firefox Desktop tests. They would fit nicely in Marionette runner as BaseDesktop(TestCase|Runner|Arguments).

I'll add more detail after I meet with David, tomorrow.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Agreed-upon changes:
* runner 
** Include a default set of browser prefs: those specific to desktop tests (in BaseDeskopRunner, say). Are there prefs that are useful for tests across all platforms?
** reset marionette before running tests
* arguments
** --e10s option: this should eventually be replaced with an option that deactivates e10s (e.g. --no-e10s) and we should enable e10s by default. 
** --workspace option: path to use for all temporary data
* testcase (BaseDesktopTestCase)
** setup/teardown: check and fix leaked window handles
** ability to restart browser 
** include firefox_puppeteer libraries


Notes:
* We won't add --installer, installer_path option into Marionette's runner: "Ideally Mozharness et al should set this up"
* Changes to the e10s flag and default behaviour will affect many test suites; any thoughts about coordinating that?
* Same for default prefs -- will any existing test suites break as a result?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Something else which I missed in our call is the specific behavior of our update tests. We have to call runtests() twice due to the direct and fallback updates. This is currently not really possible. The runner only expects a single run and as result we loose all logging to gecko.log (bug 1174766) and have to do some other workaround as can be seen here:

https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_ui_harness/runners/update.py#L53

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Some friendly questions about update tests:
- The direct and fallback update tests must be run against independent Fx binaries, hence different Marionette sessions, correct?
- Are the direct and fallback update tests independent? Must they run immediately one after the other, or in a particular order?
- Why run them one after the other in one call? Why not run them in separate jobs, separate workspaces to avoid overwriting gecko.log, say?
- Based on bug 1174766, if you set gecko_log to a directory, the session gets logged to |'gecko-%d.log' % time.time()| -- doesn't that address your issue sufficiently?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #3)
> Some friendly questions about update tests:
> - The direct and fallback update tests must be run against independent Fx
> binaries, hence different Marionette sessions, correct?

Once Firefox has been updated we cannot run the fallback update on the same binary. That's why we have to restore a formerly made backup to run another update test.

> - Are the direct and fallback update tests independent? Must they run
> immediately one after the other, or in a particular order?

No order. Both are independent tests.

> - Why run them one after the other in one call? Why not run them in separate
> jobs, separate workspaces to avoid overwriting gecko.log, say?

Similar to all the other harnesses we have a single invocation of the run. There might even more tests coming in the future. Also those tests have to be run fast due to the time limitation of update tests for releases, so having to setup new environments and such takes way longer as a single test run. We can discuss possible alternatives here.

> - Based on bug 1174766, if you set gecko_log to a directory, the session
> gets logged to |'gecko-%d.log' % time.time()| -- doesn't that address your
> issue sufficiently?

It's something I have to check next week. The directory option has been added later and I didn't had the time to check it.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I filed bug 1214372 for the firefox-ui-tests part. Both project can be moved independently into the tree once the necessary mixin classes are present in Marionette proper.

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

BaseDesktopTestCase will not use Puppeteer

Comment 7

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I've decided to drop the following from the initial proposal:
* --workspace option: This fits better in a mozharness script, especially given that mozharness already sets up a local workspace under ./build and can take care of installing Firefox. The "workspace path" currently set up with --workspace by firefox_ui_harness is used to keep copies for Fx binaries for update tests and it's clobbered by Jenkins before each new job.
* BaseDesktopTestCase: this was initially supposed hold some general-purpose features of FirefoxTestCase. As it turns out, all these ultimately rely on firefox_puppeteer. Instead, FirefoxTestCase is moving into firefox_puppeteer for easy use by other test suites (Bug 1217046).

A note about --e10s in Marionette test runner: bug 1217557

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I've tested locally, including with firefox-ui-tests.

Here's a idea of how things might look for firefox-ui-tests as a result: https://github.com/mozilla/firefox-ui-tests/compare/mozilla-central...mjzffr:new_runner

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52f202f4b01e

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: MozReview Request: Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?whimboo, r?automatedtester

* BaseDesktopTestRunner just sets default prefs for desktop tests
* BaseMarionetteRunner now resets self.marionette and self.tests with every
  call to run_tests()
* Refactor runtests.py to make it easier to extend/customize the harness
  for different test suites. Add more error handling.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://reviewboard.mozilla.org/r/23179/#review20711

I'm not an official peer so I cannot review your patches for Marionette. At least I can give feedback and that's what I did now. So in general I really like those changes which make Mariontte more flexible in some areas. There are some questions still unresolved and might need further investigation or discussion. Please also make sure to trigger try builds for your changes so that we can see that existent tests are not broken.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://reviewboard.mozilla.org/r/23179/#review20711

Not sure but all of my review comments are gone and not be part of this review. :( I will file a bug against mozreview, and re-do them all in a bit.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://reviewboard.mozilla.org/r/23179/#review20723

Lets hope that this works well now.

::: testing/marionette/client/marionette/__init__.py:14
(Diff revision 1)
> +        BaseDesktopTestRunner,

Please sort alphabetically.

::: testing/marionette/client/marionette/runner/__init__.py:9
(Diff revision 1)
> +    BaseDesktopTestRunner,

nit: sorting order again.

::: testing/marionette/client/marionette/runner/base.py:724
(Diff revision 1)
> -        if not self.marionette:
> +        # Ensure Marionette is always reset before starting tests

Does that ensure we clean-up everything? I mean if we have cyclic references we might leave memory behind.

::: testing/marionette/client/marionette/runner/base.py:1171
(Diff revision 1)
> +        self.prefs.update(self._default_browser_prefs)

Seeing this new runner type I wonder how we currently have Marionette unit tests as run on desktop setup. I assume they all use the basic marionette classes. Maybe its something which needs an update?

Also regarding the addition of all those prefs... I wonder again if we should make use of them via the testing prefs as referenced on bug 1123683.

::: testing/marionette/client/marionette/runtests.py:43
(Diff revision 1)
> +        runner = self._runner_class(**args_dict)

Do we really need this separate method? It's not that much code that it couldn't live inside of `run()`.

::: testing/marionette/client/marionette/runtests.py:47
(Diff revision 1)
> -def cli(runner_class=MarionetteTestRunner, parser_class=MarionetteArguments):
> +    def _parse_args(self, logger_defaults=None):

What if a subclass wants to override this method? It should not be done for private methods.

::: testing/marionette/client/marionette/runtests.py:68
(Diff revision 1)
> -    runner = startTestRunner(runner_class, args)
> +    def _process_args(self):

Same here regarding private method.

::: testing/marionette/client/marionette/runtests.py:79
(Diff revision 1)
> +                                   exc_info=True)

Does the logger object not have an `exception()` method which would automically care about the traceback?

::: testing/marionette/client/marionette/runtests.py:87
(Diff revision 1)
> -    cli()
> +    sys.exit(cli())

The `run()` method already calls `sys.exit()`. So this change is not necessary.

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/23179/#review20723

> Do we really need this separate method? It's not that much code that it couldn't live inside of `run()`.

I agree.

> Does the logger object not have an `exception()` method which would automically care about the traceback?

No `exception()` method.

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8678328 [details]
MozReview Request: Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester, jgriffin

* BaseDesktopTestRunner just sets default prefs for desktop tests
* BaseMarionetteRunner now resets self.marionette and self.tests with every
  call to run_tests()
* Refactor runtests.py to make it easier to extend/customize the harness
  for different test suites. Add more error handling.

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Thanks for the quick feedback, Henrik. I've addressed most of your comments -- a couple require some more thought on my part.

Comment 16

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8678328 [details]
MozReview Request: Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

https://reviewboard.mozilla.org/r/23181/#review20827

::: testing/marionette/client/marionette/runner/base.py:724
(Diff revision 2)
> -        if not self.marionette:
> +        # Ensure Marionette is always reset before starting tests

Like whimboo, I'm a bit concerned that this may have unintended consequences. Is there a particular type of state we're trying to clear here, which we might be able to address in a more specific way?

::: testing/marionette/client/marionette/runner/base.py:1128
(Diff revision 2)
> +    _default_browser_prefs = {

We've spent some time consolidating the profileration of test prefs across the tree, and it would be nice to avoid new profileration here. There are two spots where we might consolidate this: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py

or

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ah, geckoinstance.py I was looking for. Thanks Jonathan! So I agree that we should make use of that. I assume that this is for desktop or any kind of build? I won't go with mozprofile because that is also used in non-automation projects and could cause unexpected behavior to those tools. We even might have to remove some prefs from profile.py which we have added months back for Mozmill and are only automation related.

Comment 18

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/23179/#review20723

> Does that ensure we clean-up everything? I mean if we have cyclic references we might leave memory behind.

Consolidating this issue with that from :jgriffin's review. (mozreview weirdness)

> Seeing this new runner type I wonder how we currently have Marionette unit tests as run on desktop setup. I assume they all use the basic marionette classes. Maybe its something which needs an update?
> 
> Also regarding the addition of all those prefs... I wonder again if we should make use of them via the testing prefs as referenced on bug 1123683.

Consolidating this issue with that from :jgriffin's review. (mozreview weirdness)

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/23181/#review20827

> Like whimboo, I'm a bit concerned that this may have unintended consequences. Is there a particular type of state we're trying to clear here, which we might be able to address in a more specific way?

This change is really only intended to accommodate calling `run_tests()` several times, each time with a different binary: you don't want to reuse the same Marionette instance in that case. (This came up for Firefox Update Tests [1])

Maybe a better approach is to add an `update_binary(path)` method to the runner that also does the necessary Marionette clean-up. If someone wants to call `run_tests()` many times, it's up to them to call `update_binary()` in between as needed. 

As for how to properly clear the state, I see that the `BaseMarionetteTestRunner` accumulates `MarionetteTestResult` instances in `self.results`, and each `MarionetteTestResult` refers to the `Marionette` instance used in the tests that produced the result. So, if we use a new Marionette instance in each call to `run_tests()` (as in Firefox Update Tests), the previous Marionette instances remain (although the corresponding Marionette sessions are closed and cleaned up). It seems like the Marionette reference in `MarionetteTestResult` should be a weakref. The only usage I'm aware of is to check for crashes at the end of a test suite... any reason we might still need it after the test suite is done?

[1] https://github.com/mozilla/firefox-ui-tests/blob/acd30465aa9d1372edd2a402cd801ad20319ed4d/firefox_ui_harness/runners/update.py#L68

> We've spent some time consolidating the profileration of test prefs across the tree, and it would be nice to avoid new profileration here. There are two spots where we might consolidate this: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py
> 
> or
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/geckoinstance.py

`geckoinstance.py` makes sense. The only motivation for a distinct set of prefs in BaseDesktopTestRunner is the assumption that these prefs are only for Fx Desktop. The distinct set of prefs could still go in `geckoinstance.py` of course. Do we need to maintain different sets of prefs for different types of binaries?

Comment 20

[Jonathan Griffin (:jgriffin)]

In geckoinstance.py, we can probably subclass GeckoInstance for FxDesktop tests, and apply extra prefs there.

Re: run_tests, I think it makes sense to create a method which can be called explicitly if you're switching binaries, as you suggested.

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/23181/#review20827

> `geckoinstance.py` makes sense. The only motivation for a distinct set of prefs in BaseDesktopTestRunner is the assumption that these prefs are only for Fx Desktop. The distinct set of prefs could still go in `geckoinstance.py` of course. Do we need to maintain different sets of prefs for different types of binaries?

Filed bug 1219397

Comment 22

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8678328 [details]
MozReview Request: Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

* BaseMarionetteRunner: any change to self.bin resets self.marionette and
  self.tests, so you change binaries between calls to run_tests()
* Refactor runtests.py to make it easier to extend/customize the harness
  for different test suites. Add more error handling.

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Try run with latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a703221927be

Comment 24

[David Burns :automatedtester]

Comment on attachment 8678328 [details]
MozReview Request: Bug 1212608 - Add parts of firefox_ui_harness to Marionette runner; r?automatedtester

https://reviewboard.mozilla.org/r/23181/#review21351

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cb956020202

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1cb956020202

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Maja given that all? is done now, we could release new versions of the marionette packages, right? Or do we need more fixes/enhancements? Could you take care of the releases if its fine for now? Thanks.

Comment 28

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Henrik: All is done and I'll take care of the release soon. I'm waiting for a couple of patches to land, but we may be able to release a new version early next week.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Maja,

We got following error when running marionette-webapi:

ImportError: cannot import name startTestRunner

  File "/data/Projects/Builds/emulator/../../../../home/bevis/Projects/Builds/gecko/src/testing/marionette/mach_commands.py", line 109, in run_marionette_webapi
    topsrcdir=self.topsrcdir, **kwargs)
  File "/data/Projects/Builds/emulator/../../../../home/bevis/Projects/Builds/gecko/src/testing/marionette/mach_commands.py", line 39, in run_marionette
    from marionette.runtests import (

The test can be run after backout this bug locally.

Can you take a look at this?

Thanks!

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bevis, that method doesn't exist anymore. What you want is to use the MarionetteHarness class instead. See the patch as landed: https://hg.mozilla.org/mozilla-central/rev/1cb956020202#l2.12

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Henrik Skupin (:whimboo) from comment #30)
> Bevis, that method doesn't exist anymore. What you want is to use the
> MarionetteHarness class instead. See the patch as landed:
> https://hg.mozilla.org/mozilla-central/rev/1cb956020202#l2.12

Yes, that's the problem.
What I'd like to point out is that this patch caused the marionette-webapi in emulator failed to run.
I expect that the marionette-webapi in emulator shall be taken into account when landing this bug, even it's currently hidden in the treehelder. :(

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh I see. So yes, that was not ideal. Maybe it would be good to compile a list of test suites which make use of Marionette so we could limit the impact of such changes in the future. I think that would be good for a topic in tools mailing list, if you don't mind to create it.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

FYI the discussion can be found here: https://groups.google.com/forum/#!topic/mozilla.tools/8l3idMfPNmY