1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

I have seen that we run some of our unit tests like preferences, restart, and similar on platforms where Marionette doesn't control the application instance. It means that we completely fail there.

We should wait for bug 1323770 and then have a new skip decorator for self.marionette.instance.

The fix will green up a lot of unit tests for Fennec.

Comment 1

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

Attachment: Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

Review commit: https://reviewboard.mozilla.org/r/101976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101976/

Comment 2

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101976/diff/1-2/

Comment 3

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101976/diff/2-3/

Comment 4

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

Try build of latest update look fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea9dc34d7f57c974dc3b498e936ff35b83d08d72&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=66224400

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

Generally it’s not clear to be why we need a new decorator `run_if_manage_instance` if there is complete overlap between the tests it’s being used for and the tests we can’t run on Fennec.  Could the tese tests not simply be ignored for Fennec?

::: testing/marionette/driver.js:2673
(Diff revision 3)
>  /**
>   * Quits Firefox with the provided flags and tears down the current
>   * session.
>   */
>  GeckoDriver.prototype.quitApplication = function (cmd, resp) {
> -  assert.firefox()
> +  assert.firefox("Bug 1298921 - In app initiated quit only supported in Firefox yet")

s/yet// for more correct English.

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:72
(Diff revision 3)
> +def run_if_manage_instance(reason):
> +    """Decorator which runs a test if e10s mode is active."""

Copied documentation is wrong.

::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:48
(Diff revision 3)
>          self.assertEqual(self.caps["moz:processID"], self.appinfo["processID"])
>          self.assertEqual(self.marionette.process_id, self.appinfo["processID"])
>  
> +        if self.marionette.instance is not None:
> -        current_profile = self.marionette.instance.runner.profile.profile
> +            current_profile = self.marionette.instance.runner.profile.profile
> -        self.assertIn("moz:profile", self.caps)
> +            self.assertIn("moz:profile", self.caps)

We still want this test when the browser instance isn’t managed.

::: testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py:25
(Diff revision 3)
>              func=self.test_correct_test_name.__name__,
>          )
>  
>          self.assertEqual(self.marionette.test_name, expected_test_name)
>  
> +    @run_if_manage_instance("Only runnable if Marionette manages the instance")

The string here is pretty self-explanatory from the decorator’s name, but OK.

Comment 6

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

This is not only a Fennec issue. It happens with all applications if Marionette doesn't control its life-cycle.

> s/yet// for more correct English.

I used 'yet' to indicate that this is a feature we have to activate. I don't see why it should not be supported in other applications. All of them have the appropriate Gecko APIs available to do so. So I updated the comment to the style we use for other exceptions in driver.js.

> We still want this test when the browser instance isn’t managed.

Ups, you are right. I will move it up before the if condition.

Comment 7

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101976/diff/3-4/

Comment 8

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103206

r+wc

::: testing/marionette/harness/marionette_harness/tests/unit/test_shadow_dom.py:18
(Diff revision 4)
> -        MarionetteTestCase.setUp(self)
> -        self.marionette.enforce_gecko_prefs({"dom.webcomponents.enabled": True})
> +        super(TestShadowDom, self).setUp()
> +        self.marionette.set_pref("dom.webcomponents.enabled", True)
>          self.marionette.navigate(self.marionette.absolute_url("test_shadow_dom.html"))
>  
>          self.host = self.marionette.find_element(By.ID, "host")
>          self.marionette.switch_to_shadow_root(self.host)
>          self.button = self.marionette.find_element(By.ID, "button")
>  
> +    def tearDown(self):
> +        self.marionette.clear_pref("dom.webcomponents.enabled")
> +        super(TestShadowDom, self).tearDown()
> +

Is this from a rebase? If not, could you put these changes in a separate logical commit, please?

Comment 9

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103206

> Is this from a rebase? If not, could you put these changes in a separate logical commit, please?

No, it's not from a rebase but I would have to skip this test if those changes aren't made. There is no need to restart Firefox by enforcing this preference. I can surely make it a separate commit, which needs to be added before this one.

Comment 10

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

Attachment: Bug 1326047 - Remove usage of enforce_gecko_prefs() from test_shadow_dom.py.

Flipping the preference "dom.webcomponents.enabled" doesn't require a restart.
As such there is no need to use enforce_gecko_prefs().

Review commit: https://reviewboard.mozilla.org/r/102862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102862/

Comment 11

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101976/diff/4-5/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

> I used 'yet' to indicate that this is a feature we have to activate. I don't see why it should not be supported in other applications. All of them have the appropriate Gecko APIs available to do so. So I updated the comment to the style we use for other exceptions in driver.js.

Alright.

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

But there are only two applications: Firefox and Fennec, and the test harness always manages the Firefox instance for Firefox.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8824344 [details]
Bug 1326047 - Remove usage of enforce_gecko_prefs() from test_shadow_dom.py.

https://reviewboard.mozilla.org/r/102862/#review103408

r+, but there are some test failures.

Comment 15

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

It's by default yes, but you can always start Firefox yourself and let Marionette connect to it. This is how WPT and other harnesses work. From time to time I would to be able to run our tests that way too, to check that all is still working. Maybe we should consider to have once run automated even.

Comment 16

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

Comment on attachment 8824344 [details]
Bug 1326047 - Remove usage of enforce_gecko_prefs() from test_shadow_dom.py.

https://reviewboard.mozilla.org/r/102862/#review103408

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1326047#c4. Mozreview doesn't show the latest try results.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12ef9e562881
Remove usage of enforce_gecko_prefs() from test_shadow_dom.py. r=ato
https://hg.mozilla.org/integration/autoland/rev/732e83a0de14
Skip unit tests which rely on an instance but Marionette doesn't manage the process. r=ato,maja_zf

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

But WPT isn’t using the Marionette test harness.  I’m not saying you need to change this, I’m just trying to understand the complexity it is introducing.

Comment 19

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

Comment on attachment 8823450 [details]
Bug 1326047 - Skip unit tests which rely on an instance but Marionette doesn't manage the process.

https://reviewboard.mozilla.org/r/101976/#review103010

It may have been a bad example, but other harnesses (mochitest, reftest) match it. Given that those are all using Marionette to install add-ons.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12ef9e562881
https://hg.mozilla.org/mozilla-central/rev/732e83a0de14

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1c83035673d0
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e8559ebed8f