Closed Bug 1229730 Opened 8 years ago Closed 8 years ago

Move UpdateTestCase class into puppeteer package

Categories

(Testing :: Firefox UI Tests, defect)

defect
Not set
normal

Tracking

(firefox43 fixed, firefox44 fixed, firefox45 fixed, firefox-esr38 wontfix)

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

52 bytes, text/x-github-pull-request
impossibus
: review+
Details | Review
+++ This bug was initially created as a clone of Bug #1217046 +++

In bug 1217046 we moved the testcase classes from the firefox ui harness to puppeteer and firefox-ui-tests. While working to get all the pieces into mozilla-central I noticed some problems:

1. The puppeteer package does only need the FirefoxTestCase class for the unit tests, but not otherwise. This should not qualify to have the testcase class in that package.

2. The UpdateTestCase class is even in the firefox-ui-tests sub folder. This folder will no longer be present when everything is in tree. Tests will be located close to the integration code.

All in all the best place would still be the harness package to host the testcase classes. We don't expect that someone who is using our entry scripts will use a plain Marionette testcase but always our puppeteer package to access UI and API classes. So the harness has to provide those classes.

The changeset as landed on bug 1217046 might not be easily reverted due to changes happened in the meantime. So a new patch might be necessary.
So we can revert the changeset 50527c612f3c063ab52046d0d01f610b64f5f003 even now on mozilla-central. But a small follow-up patch is necessary for the test_enable_privilege.py test.
Attached file Github PR
Attachment #8694687 - Flags: review?(mjzffr)
firefox-media-tests (and potentially other test suites) need FirefoxTestCase, which is one of the reasons behind moving FirefoxTestCase into Puppeteer: it makes more sense for a suite like media-tests to depend on Puppeteer then on firefox-ui-harness. Based on my experience with media-tests, I think new test suites will not need anything else in firefox_ui_harness, and any harness code that is common to all test suites should live in MarionetteHarness anyway.

I propose we keep FirefoxTestCase in puppeteer, and suite-specific test cases (like MediaTestCase, UpdateTestCase) in suite-specific harness packages.
Discussion 02/12/15

 (AutomatedTester) I wasn't understanding what you were mentioning earlier...
22:08 (AutomatedTester) why does FirefoxTestCase live in puppeteer?
22:09 (AutomatedTester) to make it useable by other "firefox" based tests?
22:09 (whimboo) easier usable
22:09 (whimboo) without having to require firefox-ui-harness
22:10 (whimboo) my feeling is that testcase classes should be close to the harness
22:11 (AutomatedTester) yea...
22:11 (AutomatedTester) so my guy says "puppeteer" should be equivalent to web page objects
22:11 (AutomatedTester) a DSL to help write tests
22:12 (AutomatedTester) and anything testcase related should be in the harness
22:12 (AutomatedTester) if you are writing tests you need to build on a harness
22:12 (AutomatedTester) and to get to specific parts of the AUT you need to use the DSL
22:12 (AutomatedTester) but if you just want to drive the AUT to a specific thing then you only need the DSL
22:13 (AutomatedTester) that's how I would split them
22:14 (sydpolk) what is "DSL" and "AUT"?
22:14 (maja_zf) domain specific language?
22:14 (maja_zf) i dunno AUT
22:15 (AutomatedTester) application under test
22:15 (AutomatedTester) domain specific language
22:15 (whimboo) AutomatedTester: so a DSL is optional or fixed set and needs to be used?
22:15 (whimboo) i asked because we currently have the latter situation
22:15 (whimboo) class FirefoxTestCase(MarionetteTestCase, Puppeteer):
22:16 (AutomatedTester) let's get a concrete example of what I mean
22:16 (AutomatedTester) https://github.com/mozilla/Addon-Tests/
22:17 (AutomatedTester) the pages dir is what I see as puppeteer
22:17 (maja_zf) AutomatedTester: ok, but FirefoxTestCase is useful to many test suites, so that's why I was thinking in should not live in firefox-ui-harness. perhaps it should be in marionette_test.py for common use? but then marionette client has puppeteer as a dependency -- I'm not sure we want that.
22:18 (AutomatedTester) maja_zf: why does it need to have a reference to puppeteer?
22:18 mwobensmith has left IRC (Quit: Leaving.)
22:18 (whimboo) AutomatedTester: see my above line
22:18 (maja_zf) "FirefoxTestCase(MarionetteTestCase, Puppeteer)"
22:18 (whimboo) it has been setup that way by ahal or chmanchester initially
22:18 (AutomatedTester) that doesnt answer my question
22:18 (AutomatedTester) why "FirefoxTestCase(MarionetteTestCase, Puppeteer)"
22:18 (AutomatedTester) why not "FirefoxTestCase(MarionetteTestCase)"
22:19 (AutomatedTester) can we separate concerns
22:19 (whimboo) that is what i'm trying to solve now. if that would help to get rid of the location question, we would be closer to a solution
22:20 (whimboo) so that puppeteer could optionally be used
22:20 (whimboo) but is not required
22:20 (AutomatedTester) I dont see why location matters for my question
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
22:22 (whimboo) AutomatedTester: https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_puppeteer/testcases/base.py
My suggestion is that we should separare concerns here, which may/may not be a lot of work.

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. This should be nothing more than a DSL (Domain Specific Language) e.g. https://github.com/mozilla/Addon-Tests/blob/master/pages/desktop/addons_site.py

The harness is there to do the reporting and test discovery. We can extend it to starting the browser but personally I think the "tests" should be as self contained as possible. My ideal (and this might not be entirely possible currently) is to have tests looking like https://github.com/mozilla/Addon-Tests/blob/master/tests/desktop/test_reviews.py

Having things separate means we can try update things independently if need be and can try limit who would need to do updates.

HTH
So we spoke on IRC and for now we will move the UpdateTestCase also to the puppeteer package. Just to have them located right next to each other. A more in-depth refactoring as David pointed out above will happen later.
Attachment #8694687 - Flags: review?(mjzffr)
Summary: Revert "move testcases out of firefox_ui_harness" → Move UpdateTestCase class into puppeteer package
Pushed to mozilla-central:
https://github.com/mozilla/firefox-ui-tests/commit/823a9f0f3cf50093a54cc73cc6d8e27bc97f52f8

I will care of other branches on Monday.
Target Milestone: --- → Firefox 45
I backported the changes to beta and release but left esr38 out given that it has merge issues and I do not see a value in having the changes there.

https://github.com/mozilla/firefox-ui-tests/commit/9dab39b2ee8122def004608ed496780b10e37cba (beta)
https://github.com/mozilla/firefox-ui-tests/commit/0f7cd5b2c5b5b8bda24de63a6c4922057b7fcb90 (release)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: