Closed Bug 1238733 Opened 8 years ago Closed 7 years ago

Remove dependency between harness and firefox-puppeteer

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

Details

Attachments

(2 files, 1 obsolete file)

MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk
58 bytes, text/x-review-board-request
whimboo
: review+
sydpolk
: review+
Details
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk
58 bytes, text/x-review-board-request
whimboo
: review+
sydpolk
: review+
Details 
We want to separate concerns between firefox-puppeteer and the test runner.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1229730#c4

Excerpts:
"22:21 (whimboo) so FirefoxTestCase basically sets up some properties like
self.browser which is provided via puppeteer
22:21 (maja_zf) AutomatedTester: puppeteer is used by FirefoxTestCase setup and
teardown, managing window focus/prefs. Also seems to be handy for helper
methods like restart"

"I feel like we should remove any ties between harness and puppeteer so that we
can move things and grow them independently.

Puppeteer can have a dependency on marionette_driver and only be concerned in
getting things to the place where we expect it in Firefox."
Assignee: nobody → mjzffr
FirefoxTestCase inherits from unittest.TestCase and takes
advantage of super to serve as a cooperative base class
for children of MarionetteTestCase.

Review commit: https://reviewboard.mozilla.org/r/40113/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40113/
Attachment #8730716 - Flags: review?(hskupin)
Attachment #8730716 - Flags: review?(dburns)
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

https://reviewboard.mozilla.org/r/40117/#review36613

Looks good!
Attachment #8730719 - Flags: review?(spolk) → review+
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

https://reviewboard.mozilla.org/r/40115/#review36731

::: testing/firefox-ui/harness/firefox_ui_harness/runners/__init__.py:5
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +from firefox_ui_harness.runners.testcases import (

We should put testcases under `firefox_ui_harness.testcases`. I don't actually see the relationship to the runner here.

Also why the renaming to `FunctionalTestCase`? This is really the base testcase class for Firefox. Functional tests are just a type of tests, and other types could use the same testcase class. I don't think that we want to create different testcases for each type of test.

::: testing/firefox-ui/harness/firefox_ui_harness/runners/testcases.py:17
(Diff revision 1)
>  from firefox_puppeteer.api.software_update import SoftwareUpdate
>  from firefox_puppeteer.testcases import FirefoxTestCase
>  from firefox_puppeteer.ui.update_wizard import UpdateWizardDialog
>  
>  
> -class UpdateTestCase(FirefoxTestCase):
> +class FunctionalTestCase(FirefoxTestCase, MarionetteTestCase):

IMO what we mixin from Puppeteer should be a class called `FirefoxPuppeteer`. It's not a testcase.
Attachment #8730718 - Flags: review?(hskupin)
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

https://reviewboard.mozilla.org/r/40113/#review36729

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:25
(Diff revision 1)
> +
> +    example:
> +    `class AwesomeTestCase(FirefoxTestCase, MarionetteTestCase)`
> +
> +    If you're extending the inheritance tree further to make custom TestCases,
> +    favour the use of super() as opposed to explicit calls to a parent class.

Hm, I really thought that we indented to move both testcase classes to the harness and only keep the logic / helpers which are based on MarionetteTestCase in there. Puppeteer can then add its own logic, but only if explicitly added to the class inheritance in the test.

Here an example: Take the `restart()` method. Most of it would perfectly work without puppeteer. Only if puppeteer gets added to the list of classes, it would mixin its logic for `self.browser` by overriding the `restart()` method.

So including puppeteer should never be necessary for a FirefoxTestCase if the test doesn't want to use its properties and helper methods.
Attachment #8730716 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/40113/#review36729

> Hm, I really thought that we indented to move both testcase classes to the harness and only keep the logic / helpers which are based on MarionetteTestCase in there. Puppeteer can then add its own logic, but only if explicitly added to the class inheritance in the test.
> 
> Here an example: Take the `restart()` method. Most of it would perfectly work without puppeteer. Only if puppeteer gets added to the list of classes, it would mixin its logic for `self.browser` by overriding the `restart()` method.
> 
> So including puppeteer should never be necessary for a FirefoxTestCase if the test doesn't want to use its properties and helper methods.

[To be clear, when I say 'FirefoxTestCase', I'm talking about a testcase that is used for many different Firefox desktop tests and takes advantage of Puppeteer. In my view, a vanilla, "Puppeteer-free" testcase for desktop tests already exists -- it's MarionetteTestCase.]

I see what you're saying, Henrik, but here is my understanding of the constraints to satisfy:

* There is a FirefoxTestCase now in firefox-puppeteer because it's _not_ only useful to firefox-ui tests (see external-media-tests). We expect FirefoxTestCase to be useful to other specialized Firefox test suites. It's not only the helper methods - we don't want custom runners to repeat the same or similar setup and teardown code that uses Puppeteer; that code should be reusable instead. We also don't want these custom runners to depend on firefox-ui-harness.
* At the same time we don't want Marionette client (a.k.a. driver) or runner (a.k.a harness) to depend on or include firefox-puppeteer, so we're not going to keep FirefoxTestCase there either.
* And of course we don't want firefox-puppeteer to depend on the Marionette Runner.

So this patch proposes a way to provide a base TestCase that takes advantage of Puppeteer while satisfying the above.

Regarding your example of `restart()` - this patch series works perfectly with what you suggest: if there was a puppeteer-free `restart()` in MarionetteTestCase, say, then the FirefoxTestCase in firefox-puppeteer would override it with its fancy `restart()` whenever it is inserted into the MRO. In addition, it provides fancy setUp and tearDown.
https://reviewboard.mozilla.org/r/40115/#review36731

> We should put testcases under `firefox_ui_harness.testcases`. I don't actually see the relationship to the runner here.
> 
> Also why the renaming to `FunctionalTestCase`? This is really the base testcase class for Firefox. Functional tests are just a type of tests, and other types could use the same testcase class. I don't think that we want to create different testcases for each type of test.

The rationale is that testcases are part of the runner as a whole, but I will put them wherever you want and name them whatever you want. 

I need to distinguish between the testcase in firefox-puppeteer and the one in firefox-ui, so how about we keep the one in firefox-ui as 'FirefoxTestCase' and the one in firefox-puppeteer as 'BaseFirefoxTestCase'?

> IMO what we mixin from Puppeteer should be a class called `FirefoxPuppeteer`. It's not a testcase.

See my previous comment. Strictly speaking, the \*TestCase in firefox-puppeteer is a testcase that uses the Puppeteer mixin; it is not itself a mixin.
https://reviewboard.mozilla.org/r/40115/#review36731

> The rationale is that testcases are part of the runner as a whole, but I will put them wherever you want and name them whatever you want. 
> 
> I need to distinguish between the testcase in firefox-puppeteer and the one in firefox-ui, so how about we keep the one in firefox-ui as 'FirefoxTestCase' and the one in firefox-puppeteer as 'BaseFirefoxTestCase'?

The testcases are not part of the runner code. You can see this for Marionette too:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/marionette_test.py vs.
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner

> See my previous comment. Strictly speaking, the \*TestCase in firefox-puppeteer is a testcase that uses the Puppeteer mixin; it is not itself a mixin.

Then we have to change that. If you won't have the time for that maybe I can continue here the remaining days of this month. Just let me know.
https://reviewboard.mozilla.org/r/40115/#review36731

> The testcases are not part of the runner code. You can see this for Marionette too:
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/marionette_test.py vs.
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner

Yeah, Marionette is a terminology nightmare. :) The words "harness" and "runner" are often used interchangeably.

> Then we have to change that. If you won't have the time for that maybe I can continue here the remaining days of this month. Just let me know.

As dicussed, we need to check what's in `setUp` and `tearDown` in FirefoxTestCase to decide whether to keep them at all.

Some points about the MRO.
* If `setUp` and `tearDown` are included just in the Puppeteer mixin, they will either override or be overriden by methods in TestCase/CommonTestCase/MarionetteTestCase. Putting `setUp` and `tearDown` in a "base" FirefoxTestCase that inherits from `unittest.TestCase` (as in this patch) allows them to call `super()` safely (in the sense of type-safety) and will distinguish them from the DSL provided by Puppeteer.
* The MRO we have in m-c now is maintained by this patch:
```
<class 'firefox_puppeteer.testcases.base.FirefoxTestCase'>
<class 'marionette.marionette_test.MarionetteTestCase'>
<class 'marionette.marionette_test.CommonTestCase'>
<class 'unittest.case.TestCase'>
<class 'firefox_puppeteer.Puppeteer'>
<type 'object'>
```
https://reviewboard.mozilla.org/r/40113/#review37419

So we talked about that briefly via Vidyo and agreed on the current approach. It's a wonderful start for the refactoring and can be continued once we have a generic deskop testcase in Marionette. As spoken I will add some more comments about things which are unclear for me or when something is missing.

::: testing/puppeteer/firefox/firefox_puppeteer/__init__.py:12
(Diff revision 1)
>  from marionette_driver.marionette import HTMLElement
>  
>  from decorators import use_class_as_property
>  
>  
> -__version__ = '3.2.0'
> +__version__ = '3.3.0'

Lets make it a major version bump given that we also got the Firefox version bumped since the last release of the puppeteer package.

Also update firefox_ui_harness for it, and especially uncomment the line in its requirements.txt.

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:80
(Diff revision 1)
>  
>      def setUp(self, *args, **kwargs):
> -        MarionetteTestCase.setUp(self, *args, **kwargs)
> -        Puppeteer.set_marionette(self, self.marionette)
> +        try:
> +            super(FirefoxTestCase, self).setUp(*args, **kwargs)
> +        except (AttributeError, TypeError):
> +            unittest.TestCase.setUp(self)

Can you explain why we need this fallback?

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py:81
(Diff revision 1)
>      def setUp(self, *args, **kwargs):
> -        MarionetteTestCase.setUp(self, *args, **kwargs)
> -        Puppeteer.set_marionette(self, self.marionette)
> +        try:
> +            super(FirefoxTestCase, self).setUp(*args, **kwargs)
> +        except (AttributeError, TypeError):
> +            unittest.TestCase.setUp(self)
> +        super(FirefoxTestCase, self).set_marionette(self.marionette)

So how does this line set the marionette object of the Puppeteer class?
https://reviewboard.mozilla.org/r/40115/#review37421

One more comment here. The other opened issues will remain from the last reviews, e.g. the location of the testcase classes in the harness.

::: testing/firefox-ui/harness/firefox_ui_harness/runners/testcases.py:17
(Diff revision 1)
>  from firefox_puppeteer.api.software_update import SoftwareUpdate
>  from firefox_puppeteer.testcases import FirefoxTestCase
>  from firefox_puppeteer.ui.update_wizard import UpdateWizardDialog
>  
>  
> -class UpdateTestCase(FirefoxTestCase):
> +class FunctionalTestCase(FirefoxTestCase, MarionetteTestCase):

Given that you moved the testcase class to the harness, the version bump of the puppeteer package should be part of the last commit.

If possible I would also not add a new testcase class for the functional tests given that this type of test not necessarily would always require this kind of testcase.
Attachment #8730716 - Attachment description: MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?AutomatedTester;whimboo → MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo
Attachment #8730716 - Flags: review?(dburns) → review?(hskupin)
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/1-2/
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/1-2/
Attachment #8730718 - Attachment description: MozReview Request: Bug 1238733 - Update Firefox UI and Update tests with new FirefoxTestCase; r?whimboo;AutomatedTester → MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in Firefox UI and Update tests; r?whimboo
Attachment #8730718 - Flags: review?(dburns) → review?(hskupin)
Attachment #8730719 - Attachment description: MozReview Request: Bug 1238733 - Update external media tests with new FirefoxTestCase; r?sydpolk → MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/1-2/
Attachment #8730719 - Flags: review+ → review?(spolk)
https://reviewboard.mozilla.org/r/40113/#review37419

> Can you explain why we need this fallback?

Ugh, I'm ambivalent about keeping this fallback. The initial motivation was in case this class is used with a normal TestCase instead of a MarionetteTestCase, but that doesn't really make sense. I could definitely remove it. What do you think?

> So how does this line set the marionette object of the Puppeteer class?

FirefoxTestCase inherits from Puppeteer, so the call to super follows the MRO until it finds `set_marionette` defined in Puppeteer. 

However, this line is a no-op, by definition, since self.marionette is already set correctly. Indeed, if I remove this line, ./mach fireofox-ui-functional, etc. still work. In practice, MarionetteTestCase takes care of setting self.marionette in `__init__`; any class that wants to inherit from Puppeteer needs to take care of setting the Marionette instance appropriately. (I'll add that to the docs.) In fact, `Puppeteer.set_marionette` can be removed entirely -- it's not used anywhere else.
https://reviewboard.mozilla.org/r/40115/#review36731

> Yeah, Marionette is a terminology nightmare. :) The words "harness" and "runner" are often used interchangeably.

Done. 
Now we have firefox_puppeteer.testcass.base.BaseFirefoxTestCase.
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/2-3/
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/2-3/
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/2-3/
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

https://reviewboard.mozilla.org/r/40117/#review37519

Straightforward refactoring looks good.
Attachment #8730719 - Flags: review?(spolk) → review+
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

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

As it turns out, the version bump caused bustage. The mozharness script was installing firefox-puppeteer 4.0.0 from the tests package, then uninstalling it and replacing it with 3.2.0 from pypi mirror. I have pushed a fix.
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/4-5/
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/5-6/
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/3-4/
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/3-4/
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/6-7/
https://reviewboard.mozilla.org/r/40113/#review37419

> Ugh, I'm ambivalent about keeping this fallback. The initial motivation was in case this class is used with a normal TestCase instead of a MarionetteTestCase, but that doesn't really make sense. I could definitely remove it. What do you think?

Yeah, please remove. The base of our testcases should be the MarionetteTestCase nothing below.

> FirefoxTestCase inherits from Puppeteer, so the call to super follows the MRO until it finds `set_marionette` defined in Puppeteer. 
> 
> However, this line is a no-op, by definition, since self.marionette is already set correctly. Indeed, if I remove this line, ./mach fireofox-ui-functional, etc. still work. In practice, MarionetteTestCase takes care of setting self.marionette in `__init__`; any class that wants to inherit from Puppeteer needs to take care of setting the Marionette instance appropriately. (I'll add that to the docs.) In fact, `Puppeteer.set_marionette` can be removed entirely -- it's not used anywhere else.

If it's not needed lets remove it too. Puppeteer is dependent on Marionette, so as mentioned above the lowest base is MarionetteTestCase.
https://reviewboard.mozilla.org/r/40113/#review36729

> [To be clear, when I say 'FirefoxTestCase', I'm talking about a testcase that is used for many different Firefox desktop tests and takes advantage of Puppeteer. In my view, a vanilla, "Puppeteer-free" testcase for desktop tests already exists -- it's MarionetteTestCase.]
> 
> I see what you're saying, Henrik, but here is my understanding of the constraints to satisfy:
> 
> * There is a FirefoxTestCase now in firefox-puppeteer because it's _not_ only useful to firefox-ui tests (see external-media-tests). We expect FirefoxTestCase to be useful to other specialized Firefox test suites. It's not only the helper methods - we don't want custom runners to repeat the same or similar setup and teardown code that uses Puppeteer; that code should be reusable instead. We also don't want these custom runners to depend on firefox-ui-harness.
> * At the same time we don't want Marionette client (a.k.a. driver) or runner (a.k.a harness) to depend on or include firefox-puppeteer, so we're not going to keep FirefoxTestCase there either.
> * And of course we don't want firefox-puppeteer to depend on the Marionette Runner.
> 
> So this patch proposes a way to provide a base TestCase that takes advantage of Puppeteer while satisfying the above.
> 
> Regarding your example of `restart()` - this patch series works perfectly with what you suggest: if there was a puppeteer-free `restart()` in MarionetteTestCase, say, then the FirefoxTestCase in firefox-puppeteer would override it with its fancy `restart()` whenever it is inserted into the MRO. In addition, it provides fancy setUp and tearDown.

> [To be clear, when I say 'FirefoxTestCase', I'm talking about a testcase
> that is used for many different Firefox desktop tests and takes advantage of
> Puppeteer. In my view, a vanilla, "Puppeteer-free" testcase for desktop
> tests already exists -- it's MarionetteTestCase.]

I do not agree on this. At least not with the current state of MarionetteTestCase. It's not created for running tests in chrome scope by default, handling restarts and such. So if we want to have it as a base testcase class for desktop, we should definitely enhance it by moving code over there.

> * There is a FirefoxTestCase now in firefox-puppeteer because it's _not_
> only useful to firefox-ui tests (see external-media-tests). We expect
> FirefoxTestCase to be useful to other specialized Firefox test suites. It's
> not only the helper methods - we don't want custom runners to repeat the
> same or similar setup and teardown code that uses Puppeteer; that code
> should be reusable instead. We also don't want these custom runners to
> depend on firefox-ui-harness.

So what I would propose is that we maybe move the plain FirefoxTestCase to Marionette itself as a MarionetteDesktopTestCase, which contains all the basics you need and as mentioned above. Then there would be no need to have this testcase class in firefox-ui-harness, and media tests won't have to depent on it.

> * At the same time we don't want Marionette client (a.k.a. driver) or runner
> (a.k.a harness) to depend on or include firefox-puppeteer, so we're not
> going to keep FirefoxTestCase there either.

That is true because puppeteer is not a real testcase class. It only mixes in additional helpers and properties which are of help for Firefox test cases and could be used.

> * And of course we don't want firefox-puppeteer to depend on the Marionette
> Runner.

We do not need that at all, right.

> So this patch proposes a way to provide a base TestCase that takes advantage
> of Puppeteer while satisfying the above.
> 
> Regarding your example of `restart()` - this patch series works perfectly
> with what you suggest: if there was a puppeteer-free `restart()` in
> MarionetteTestCase, say, then the FirefoxTestCase in firefox-puppeteer would
> override it with its fancy `restart()` whenever it is inserted into the MRO.
> In addition, it provides fancy setUp and tearDown.

Well, I'm still on the point that we do not want any testcase class in puppeteer. Any of those should be in the harness. But the technique you point out here is fine with me. It's just about where things are located.
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

https://reviewboard.mozilla.org/r/40115/#review37915

This looks fine to me. Please check and maybe address my comments.

::: testing/firefox-ui/tests/puppeteer/test_windows.py:11
(Diff revision 4)
>  from marionette_driver.errors import NoSuchWindowException, TimeoutException
>  
>  import firefox_puppeteer.errors as errors
>  
> -from firefox_puppeteer.testcases import FirefoxTestCase
> +from firefox_ui_harness.testcases import FirefoxTestCase
> +from firefox_ui_harness.testcases import FirefoxTestCase

nit: this line should be below the puppeteer import

::: testing/firefox-ui/harness/firefox_ui_harness/testcases.py:9
(Diff revision 4)
>  
>  import pprint
>  from datetime import datetime
>  
>  from marionette_driver import Wait
> +from marionette import MarionetteTestCase

nit: please move before the `marionette_driver` import.

::: testing/firefox-ui/harness/firefox_ui_harness/testcases.py:22
(Diff revision 4)
> -class UpdateTestCase(FirefoxTestCase):
> +class FirefoxTestCase(BaseFirefoxTestCase, MarionetteTestCase):
> +    """ Integrate MarionetteTestCase with BaseFirefoxTestCase by reordering MRO """
> +    pass
> +
> +
> +class UpdateTestCase(FirefoxTestCase, MarionetteTestCase):

Needs `MarionetteTestCase` to be listed here? There is no other mixin, and I think the MRU should be set by FirefoxTestCase.
Attachment #8730718 - Flags: review?(hskupin) → review+
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/4-5/
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/4-5/
Comment on attachment 8730719 [details]
MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in external media tests; r?sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40117/diff/7-8/
Thanks for the reviews! I believe I've addressed all the comments; just missing an r+ for the first patch from whimboo. 

I intend to squash the commits.
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

https://reviewboard.mozilla.org/r/40113/#review38103

Looks great. Would you mind to move out the version bump to an extra commit? Just in case we have to revert. In those cases we cannot go back to the old versions.
Attachment #8730716 - Flags: review?(hskupin) → review+
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/5-6/
Attachment #8730716 - Attachment description: MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo → MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?wh
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/5-6/
Attachment #8730718 - Attachment description: MozReview Request: Bug 1238733 - Use BaseFirefoxTestCase in Firefox UI and Update tests; r?whimboo → MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk
Attachment #8730718 - Flags: review?(spolk)
Attachment #8730719 - Attachment is obsolete: true
Comment on attachment 8730716 [details]
MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40113/diff/6-7/
Attachment #8730716 - Attachment description: MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?wh → MozReview Request: Bug 1238733 - Remove dependency on Marionette harness in firefox-puppeteer; r?whimboo;sydpolk
Attachment #8730716 - Flags: review?(spolk)
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/6-7/
https://reviewboard.mozilla.org/r/40115/#review38161

::: testing/firefox-ui/harness/requirements.txt:5
(Diff revision 6)
>  marionette-client >= 2.2.0
>  mozfile >= 1.2
>  mozinfo >= 0.8
>  mozinstall >= 1.12
> -
> +firefox-puppeteer == 4.0.0

nit: please sort alphabetical. Also maybe want to allow >=4.0.0 and <5.0.0.
Attachment #8730718 - Flags: review?(spolk) → review+
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

https://reviewboard.mozilla.org/r/40115/#review38215
Comment on attachment 8730718 [details]
MozReview Request: Bug 1238733 - Bump version for firefox-puppeteer and update dependencies; r?whimboo;sydpolk

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40115/diff/7-8/
Attachment #8730716 - Flags: review?(spolk) → review+
Adding leave-open keyword so that we can release the new versions to pypi.
Keywords: leave-open
Both packages have been released:

Uploading distributions to https://pypi.python.org/pypi
Uploading firefox_puppeteer-4.0.0-py2-none-any.whl
Uploading firefox-puppeteer-4.0.0.tar.gz

Uploading distributions to https://pypi.python.org/pypi
Uploading firefox_ui_harness-1.3.0-py2-none-any.whl
Uploading firefox-ui-harness-1.3.0.tar.gz

Thanks Maja!
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox48: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.