Closed Bug 1313312 Opened 8 years ago Closed 8 years ago

Core Puppeteer class should not be used as mixin

Categories

(Testing :: Firefox UI Tests, defect)

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

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1313312 - Refactor puppeteer module for UI base lib classes.
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1313312 - Separate out Puppeteer code into a core and mix-in class.
58 bytes, text/x-review-board-request
impossibus
: review+
Details 
Lets do the first step with transformation of Puppeteer and make it a true mixin class. I do not see the value of having it as a separate class outside of TestCase right now. Also because it would involve a lot of changes to tests to have something like self.puppeteer.*.

The patches which I will attach in a moment will do all the changes transparently to the tests. So whether Firefox ui tests nor any other tests depending on the Firefox ui harness should be affected.

Harnesses which directly make use of puppeteer might need an update. And as such I will bump the puppeteer version from 52.0.0 to 52.1.0.
Attachment #8805062 - Flags: review?(mjzffr)
Attachment #8805063 - Flags: review?(mjzffr)
Attachment #8805064 - Flags: review?(mjzffr)
Attachment #8805065 - Flags: review?(mjzffr)
Comment on attachment 8805062 [details]
Bug 1313312 - Refactor puppeteer module for UI base lib classes.

https://reviewboard.mozilla.org/r/88918/#review89098

Aside: could you squash the first two patches, please? The first commit needs to include a change to puppeteer's requirements (since a dependency on MarionetteTestCase is added), but it's an intermediate step toward your final goal anyway, so you may as well just squash the first two commits.

I have a couple of concerns with this patch series that I'd like to discuss. Some parts of the refactoring don't seem helpful, so I'd like to understand the motivation for them. 

Overall, in the whole commit series, I see that Puppeteer class is being moved into BaseFirefoxTestCase, and BaseFirefoxTestCase is being renamed to FirefoxPuppeteerMixin and moved, although its functionality is still the same and it still inherits from unittest.TestCase.
Attachment #8805062 - Flags: review?(mjzffr)
Comment on attachment 8805063 [details]
Bug 1313312 - Remove BaseFirefoxTestCase class from Puppeteer.

https://reviewboard.mozilla.org/r/88920/#review89104

::: dom/media/test/external/external_media_harness/testcase.py:21
(Diff revision 1)
>      VideoException,
>      VideoPuppeteer
>  )
>  
>  
> -class MediaTestCase(BaseFirefoxTestCase, MarionetteTestCase):
> +class MediaTestCase(MarionetteTestCase, FirefoxPuppeteerMixin):

What is the rationale behind changing the MRO here?

::: testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py
(Diff revision 1)
> -    """Base TestCase class for Firefox Desktop tests.
> -
> -    This is designed to enhance MarionetteTestCase by inserting the Puppeteer
> -    mixin class (so Firefox specific API modules are exposed to test scope) and
> -    providing common set-up and tear-down code for Firefox tests.
> -
> -    Child classes are expected to also subclass MarionetteTestCase such that
> -    MarionetteTestCase is inserted into the MRO after FirefoxTestCase but before
> -    unittest.TestCase.
> -
> -    example:
> -    `class AwesomeTestCase(FirefoxTestCase, MarionetteTestCase)`
> -
> -    The key role of MarionetteTestCase is to set self.marionette appropriately
> -    in `__init__`. Any TestCase class that satisfies this requirement is
> -    compatible with this class.
> -
> -    If you're extending the inheritance tree further to make specialized
> -    TestCases, favour the use of super() as opposed to explicit calls to a
> -    parent class.

I think you need to keep much of this comment under FirefoxPuppeteerMixin, especially the part the explains how it's intended to be used with MarionetteTestCase.
Attachment #8805063 - Flags: review?(mjzffr)
Comment on attachment 8805065 [details]
Bug 1313312 - Bump Firefox Puppeteer to version 52.1.0.

https://reviewboard.mozilla.org/r/88924/#review89110
Attachment #8805065 - Flags: review?(mjzffr)
Comment on attachment 8805064 [details]
Bug 1313312 - Separate out Puppeteer code into a core and mix-in class.

https://reviewboard.mozilla.org/r/88922/#review89108
Attachment #8805064 - Flags: review?(mjzffr)
Comment on attachment 8805062 [details]
Bug 1313312 - Refactor puppeteer module for UI base lib classes.

https://reviewboard.mozilla.org/r/88918/#review89112

::: testing/puppeteer/firefox/firefox_puppeteer/base.py:7
(Diff revision 1)
>  # 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/.
>  
>  
>  class BaseLib(object):
>      """A base class that handles lazily setting the "client" class attribute."""

This comment needs to be updated, especially  'lazily'.
Comment on attachment 8805062 [details]
Bug 1313312 - Refactor puppeteer module for UI base lib classes.

https://reviewboard.mozilla.org/r/88918/#review89098

Squashing should be fine. It would also remove some of the changes which would only exist for that single commit and make browsing/blaming easier in the future.

Regarding the other lines I think we should clarify everything about mixins first as already noted on bug 1259055.
Attachment #8805063 - Attachment is obsolete: true
Comment on attachment 8805063 [details]
Bug 1313312 - Remove BaseFirefoxTestCase class from Puppeteer.

https://reviewboard.mozilla.org/r/88920/#review89104

> What is the rationale behind changing the MRO here?

All Marionette based tests will have MarionetteTestCase as super class. We should not carry forward a BaseFirefoxTestCase class which simply mixes in Puppeteer, and is doing nothing else otherwise. Instead the tests which need Puppeteer will have to be setup like `class MyTestCase(MarionetteTestCase, FirefoxPuppeteerMixin)`.

> I think you need to keep much of this comment under FirefoxPuppeteerMixin, especially the part the explains how it's intended to be used with MarionetteTestCase.

Looks like it was indeed lost. I will get this added again.
Comment on attachment 8805063 [details]
Bug 1313312 - Remove BaseFirefoxTestCase class from Puppeteer.

https://reviewboard.mozilla.org/r/88920/#review89104

> All Marionette based tests will have MarionetteTestCase as super class. We should not carry forward a BaseFirefoxTestCase class which simply mixes in Puppeteer, and is doing nothing else otherwise. Instead the tests which need Puppeteer will have to be setup like `class MyTestCase(MarionetteTestCase, FirefoxPuppeteerMixin)`.

Let me clarify my question. Your patch renames BaseFirefoxTestCase to FirefoxPuppeteerMixin (with some refactoring, but the behaviour remains the same), and then in actual test cases the MRO is being changed by going from something like `MediaTestCase(FirefoxPuppeteerMixin, MarionetteTestCase)` to `MediaTestCase(MarionetteTestCase, FirefoxPuppeteerMixin)`. So before, the MRO is `<class 'firefox_ui_harness.testcases.FirefoxTestCase'>, <class 'firefox_puppeteer.mixins.FirefoxPuppeteerMixin'>, <class 'marionette.marionette_test.testcases.MarionetteTestCase'>, <class 'marionette.marionette_test.testcases.CommonTestCase'>, <class 'unittest.case.TestCase'>, <type 'object'>` and now, with this change, the MRO is `<class 'firefox_ui_harness.testcases.FirefoxTestCase'>, <class 'marionette.marionette_test.testcases.MarionetteTestCase'>, <class 'marionette.marionette_test.testcases.CommonTestCase'>, <class 'firefox_puppeteer.mixins.FirefoxPuppeteerMixin'>, <class 'unittest.case.TestCase'>, <type 'object'>`. So my question is why do you want to move FirefoxPuppeteerMixin further down the MRO list? If there's a reason, then it should be documented.

Although the new MRO doesn't break anything right now, I believe the change is counter-intuitive for consumers: if we inherit from FirefoxPuppeteerMixin, we want FirefoxPuppeteer.foo to override MarionetteTestCase.foo if super isn't called.
Comment on attachment 8805063 [details]
Bug 1313312 - Remove BaseFirefoxTestCase class from Puppeteer.

https://reviewboard.mozilla.org/r/88920/#review89104

> Let me clarify my question. Your patch renames BaseFirefoxTestCase to FirefoxPuppeteerMixin (with some refactoring, but the behaviour remains the same), and then in actual test cases the MRO is being changed by going from something like `MediaTestCase(FirefoxPuppeteerMixin, MarionetteTestCase)` to `MediaTestCase(MarionetteTestCase, FirefoxPuppeteerMixin)`. So before, the MRO is `<class 'firefox_ui_harness.testcases.FirefoxTestCase'>, <class 'firefox_puppeteer.mixins.FirefoxPuppeteerMixin'>, <class 'marionette.marionette_test.testcases.MarionetteTestCase'>, <class 'marionette.marionette_test.testcases.CommonTestCase'>, <class 'unittest.case.TestCase'>, <type 'object'>` and now, with this change, the MRO is `<class 'firefox_ui_harness.testcases.FirefoxTestCase'>, <class 'marionette.marionette_test.testcases.MarionetteTestCase'>, <class 'marionette.marionette_test.testcases.CommonTestCase'>, <class 'firefox_puppeteer.mixins.FirefoxPuppeteerMixin'>, <class 'unittest.case.TestCase'>, <type 'object'>`. So my question is why do you want to move FirefoxPuppeteerMixin further down the MRO list? If there's a reason, then it should be documented.
> 
> Although the new MRO doesn't break anything right now, I believe the change is counter-intuitive for consumers: if we inherit from FirefoxPuppeteerMixin, we want FirefoxPuppeteer.foo to override MarionetteTestCase.foo if super isn't called.

As said above there will be no `FirefoxTestCase` class in the future anymore with the firefox-ui-harness removed. Each test which doesn't need Firefox Puppeteer will depend on `MarionetteTestCase`. That's what we currently have in pure Marionette tests. Then only those tests which need extended support in handling the UI can opt-in for Puppeteer. A mixin class cannot be used as base class for inheritance and will be added as at least the second item in the class hierarchy.

Also I do not see a reason why Puppeteer has to override base Marionette methods. It's there to just bring in new features. See my WindowManagment mixin for Marionette unit tests. It's purely optional and can be added/removed at any time without changing the MRO of the test classes (except the addition and removal of itself). So maybe best we talk about this to figure out why you see this mixin as top class in the MRO list.
Comment on attachment 8805062 [details]
Bug 1313312 - Refactor puppeteer module for UI base lib classes.

Clearing all these r? because as I understand it Henrik plans to rewrite this whole patch using a different approach.
Attachment #8805062 - Flags: review?(mjzffr)
As Maja said, I will completely re-organize it again in a different way. As we chatted it will be better to have all core features of the Puppeteer class in a complete separate class which should not be used as a Mixin. This would correlate in how we build-up foxpuppet at the moment. 

On top of that we can still have mixin class for Puppeteer which takes care of the instantiation of puppeteer and makes it available by default.
Summary: Make Firefox Puppeteer a true mixin class → Core Puppeteer class should not be used as mixin
Attachment #8805065 - Attachment is obsolete: true
Comment on attachment 8805064 [details]
Bug 1313312 - Separate out Puppeteer code into a core and mix-in class.

https://reviewboard.mozilla.org/r/88922/#review90852

r+wc
Just some comments to fix.

::: testing/puppeteer/firefox/firefox_puppeteer/base.py:7
(Diff revision 4)
>  # 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/.
>  
>  
>  class BaseLib(object):
>      """A base class that handles lazily setting the "client" class attribute."""

Update this comment, esp. 'lazily'.

::: testing/puppeteer/firefox/firefox_puppeteer/puppeteer.py:11
(Diff revision 4)
> +    example:
> +    `class MyTestCase(MarionetteTestCase, Puppeteer)`

Remove this. Puppeteer in not intended to be used as a mixin with MarionetteTestCase, especially now that `__init__` just accepts a `marionette` argument. We have PuppeteerMixin for that.

::: testing/puppeteer/firefox/firefox_puppeteer/mixins.py:15
(Diff revision 4)
> -    MarionetteTestCase is inserted into the MRO after FirefoxTestCase but before
> -    unittest.TestCase.
> +    MarionetteTestCase is inserted into the MRO after PuppeteerMixin but before
> +    MarionetteTestCase.

Fix comment: "PuppeteerMixin inserted after MarionetteTestCase"?

::: testing/puppeteer/firefox/firefox_puppeteer/mixins.py:19
(Diff revision 4)
>      Child classes are expected to also subclass MarionetteTestCase such that
> -    MarionetteTestCase is inserted into the MRO after FirefoxTestCase but before
> -    unittest.TestCase.
> +    MarionetteTestCase is inserted into the MRO after PuppeteerMixin but before
> +    MarionetteTestCase.
>  
>      example:
> -    `class AwesomeTestCase(FirefoxTestCase, MarionetteTestCase)`
> +    `class MyTestCase(PuppeteerMixin, MarionetteTestCase)`

Aw, what's wrong with AwesomeTestCase? :P
Attachment #8805064 - Flags: review?(mjzffr) → review+
Comment on attachment 8805064 [details]
Bug 1313312 - Separate out Puppeteer code into a core and mix-in class.

https://reviewboard.mozilla.org/r/88922/#review90852

> Remove this. Puppeteer in not intended to be used as a mixin with MarionetteTestCase, especially now that `__init__` just accepts a `marionette` argument. We have PuppeteerMixin for that.

Ups, totally missed to update the docstring for this class after moving it to the new file.

> Fix comment: "PuppeteerMixin inserted after MarionetteTestCase"?

I think this sentence is a bit misleading. We talk about the order as written when creating a subclass, but on the other side also about the MRO. So I will restructure the comment. Please check how it looks now.

> Aw, what's wrong with AwesomeTestCase? :P

Too much awesome! The awesome bar was awesome enough. So long long story. :)
Comment on attachment 8805062 [details]
Bug 1313312 - Refactor puppeteer module for UI base lib classes.

https://reviewboard.mozilla.org/r/88918/#review90860
Attachment #8805062 - Flags: review?(mjzffr) → review+
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Comment on attachment 8805064 [details]
> Bug 1313312 - Separate out Puppeteer code into a core and mix-in class.
> 
> https://reviewboard.mozilla.org/r/88922/#review90852
> 
> > Remove this. Puppeteer in not intended to be used as a mixin with MarionetteTestCase, especially now that `__init__` just accepts a `marionette` argument. We have PuppeteerMixin for that.
> 
> Ups, totally missed to update the docstring for this class after moving it
> to the new file.
> 
> > Fix comment: "PuppeteerMixin inserted after MarionetteTestCase"?
> 
> I think this sentence is a bit misleading. We talk about the order as
> written when creating a subclass, but on the other side also about the MRO.
> So I will restructure the comment. Please check how it looks now.
> 
> > Aw, what's wrong with AwesomeTestCase? :P
> 
> Too much awesome! The awesome bar was awesome enough. So long long story. :)

Hi Maja. Mind having a look at those changes again before I push? Would like to do the right thing with no follow-ups necessary. :)
Flags: needinfo?(mjzffr)
Blocks: 1308902
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb9457561c1b
Refactor puppeteer module for UI base lib classes. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c78bef5d7961
Separate out Puppeteer code into a core and mix-in class. r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/eb9457561c1b
https://hg.mozilla.org/mozilla-central/rev/c78bef5d7961
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
New puppeteer release has been uploaded to pypi:

Uploading firefox_puppeteer-52.1.0-py2-none-any.whl
[================================] 51009/51009 - 00:00:03
Uploading firefox-puppeteer-52.1.0.tar.gz
[================================] 32768/32768 - 00:00:02
Whiteboard: [needs firefox_puppeteer release]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: