Closed Bug 1320073 Opened 8 years ago Closed 7 years ago

Rename marionette_client package to marionette_harness

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(2 files)

Bug 1320073 - Release marionette-driver 2.2.0 and marionette-client 3.3.0.
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.
58 bytes, text/x-review-board-request
bryce
: review+
gps
: review+
impossibus
: review+
mtseng
: review+
Details 
It's been a while since the rename of the folders under testing/marionette for the client and driver happened. Now we have a mixture of the following:

> testing/marionette/client -> marionette_driver
> testing/marionette/harness -> marionette_client

And as you may have noticed there is lots of confusion now. So I propose that we finally start to rename the packages. It won't be that easy because of the name clashes between those two packages. 

So my proposal is that we start to rename marionette_client into marionette_harness, and indicate on PYPI and if possible in setup.py that the marionette_client package is continued and has the successor marionette_harness. This should land for Firefox 52 which is the current ESR. Then we have to wait some time - I would suggest until the next ESR release - and also rename marionette_driver to marionette_client. This should give our customers enough time to notice this former package change and to update their code.

Andreas and David, what do you think? Does that sound fine with you?
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
If you agree here is what needs to be done:

1. Release a new version based on the old name and update setup.py so we can add an EOL message, tag, or whatever.
2. Do the rename and update all in-tree references
3. Release a new version under the new name.
This is going huge breaking change but hopefully there are near zero clients that are affected by this change.

I don't mind but this has the potential to cause issues if we want to do uplifts but if you are willing to do that work for us then sure.
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
(In reply to David Burns :automatedtester from comment #2)
> This is going huge breaking change but hopefully there are near zero clients
> that are affected by this change.

This actually shouldn't cause any trouble for us in-tree given that the packages are available via `import marionette` and not `import marionette_client`. So the only necessary change here is for the package name and docs:

https://dxr.mozilla.org/mozilla-central/search?q=marionette_client&=mozilla-central

Renaming the marionette_driver package to marionette_client will be a mess. That's true.
I’m in favour of adopting correct names for the Python packages.  Though presumably the marionette_client has a limited number of users.  I don’t think we should remove this package from PyPI but deprecate it.  Also bumping major version numbers here is a must.
Totally! We have to leave the old packages around for that long on PyPI until we can replace it with the current driver package.

Dave Hunt gave me an example in how to deprecate a module:
https://github.com/mozilla/pytest-mozwebqa/commit/baf0bf27d593414c3fa96b9e0859a1723e767aa9
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Remaining features/fixes I would like to see in the next releases for driver and client are auto-detection of the GeckoInstance class, and the updated default preferences. Maybe also the move of the docs for the current driver to marionette/client. I will mark all as blocking this bug.
Depends on: 1318644, 1319024, 1320099
The patches for bug 1103196 got backed out, so we also want to wait for this being re-landed.
Depends on: 1103196
Depends on: 1320643
Something else I would propose to do at the same time when renaming the package is that we also rename the `marionette` subfolder to `marionette_harness`. Reason is that the harness is rarely used, and the `marionette` package should be later used by marionette_client:

now:
> from marionette_driver.marionette import Marionette

proposed:
> from marionette import Marionette

If we don't do a rename like that now, we won't have another chance to do it again without breaking lots of code again.

Could you all agree on that?
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Something else I would propose to do at the same time when renaming the
> package is that we also rename the `marionette` subfolder to
> `marionette_harness`. 

Which sub-folder?

> Reason is that the harness is rarely used, and the
> `marionette` package should be later used by marionette_client:
> 
> now:
> > from marionette_driver.marionette import Marionette
> 
> proposed:
> > from marionette import Marionette
> 

I agree the extra dot is not needed but because people conflate marionette driver, harness and server all the time having an explicit name would be better than having it just be marionette and then people complaining that it doesnt have the harness code there when they needed it.
Depends on: 1320380
Blocks: 1316984
Depends on: 1321625
The last try build went pretty good, except for the Marionette tests on Fennec, which are currently broken and will be fixed via bug 1321625.

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

Given that the patch is so huge, I'm going to split it up for now into build, mozharness, and marionette buckets. It will also ease the review of those changes.
Attachment #8815859 - Flags: review?(mjzffr)
Attachment #8815859 - Flags: review?(mtseng)
Attachment #8815859 - Flags: review?(mjzffr)
Attachment #8815859 - Flags: review?(james)
Attachment #8815859 - Flags: review?(gps)
Attachment #8815859 - Flags: review?(bvandyk)
If you are a reviewer of the second patch please be aware that this patch is huge. Given that it is a monolithic change, I cannot split it into different buckets which would cause broken commits.

For details about which parts you should review please see here:

* gps - build/
* SingingTree - dom/media/test/external/
* mtseng - layout/base/tests/marionette/
* maja_zf - testing/marionette/
* jgraham - testing/web-platform/

Thanks.
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98104

r+ with a couple of comments:

- PuppeteerMixin (firefox-puppeteer/mixins.py) has had its setUp() broken. It has the line `self.marionette.navigate(self.puppeteer.prefs.get_pref('browser.newtab.url'))` which tries to fetch old pref. It looks like something was guarding against this and returning `about:newtab`. With these changes this is no longer the case (get a None and stuff falls over). If instead the line is updated to `self.marionette.navigate('about:newtab')` the correct behaviour is maintained (and we no longer use the old pref).
- The requirements.txt for the external media tests looks to need another version bump for mozrunner. I've added an inline comment.

::: dom/media/test/external/requirements.txt:14
(Diff revision 9)
>  mozInstall==1.12
>  mozlog==3.3
>  moznetwork==0.27
>  mozprocess==0.23
>  mozprofile==0.28
>  mozrunner==6.12

I believe this needs to be bumped to 6.13 to be in line with the changes to marionette.
Attachment #8815859 - Flags: review?(bvandyk) → review+
Comment on attachment 8815858 [details]
Bug 1320073 - Release marionette-driver 2.2.0 and marionette-client 3.3.0.

https://reviewboard.mozilla.org/r/96630/#review98172
Attachment #8815858 - Flags: review?(mjzffr) → review+
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98174
Attachment #8815859 - Flags: review?(mjzffr) → review+
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98200
Attachment #8815859 - Flags: review?(mtseng) → review+
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98104

Puppeteer.mixins.py has been fixed for the initial page load of about:newtab on bug 1319024 about 2 weeks ago.

Regarding the requirements.txt file it seems to be kinda overbloated. Your code doesn't use mozrunner directly so I do not see why we need this extra line in the file. Marionette will take care of the correct version itself. I will go ahead and remove all the listed dependencies which are not imported in the external media test harness. I hope that is ok for you.
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98244

::: testing/web-platform/harness/wptrunner/executors/executormarionette.py:49
(Diff revision 10)
>      global errors, marionette
>  
>      # Marionette client used to be called marionette, recently it changed
>      # to marionette_driver for unfathomable reasons
>      try:
> -        import marionette
> +        import marionette_harness

This only works by accident (or, rather becase we still have code from last time there was a rename here).

We shouldn't import anything from marionette_harness   here, just from marionette_driver, or whatever it's called these days.

I *think* that means you don't need to bump anything in the requirements_firefox.txt file. However you do need to upstream the patch to https://github.com/w3c/wptrunner
Attachment #8815859 - Flags: review?(james) → review-
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98104

Cool bananas. Is there a version of firefox-puppeteer with the fix available? I'm getting my dep resolved to 52.1.0, which is resulting in a crash when I try to test this.

Sounds good to me regarding the bloat trimming.
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98244

> This only works by accident (or, rather becase we still have code from last time there was a rename here).
> 
> We shouldn't import anything from marionette_harness   here, just from marionette_driver, or whatever it's called these days.
> 
> I *think* that means you don't need to bump anything in the requirements_firefox.txt file. However you do need to upstream the patch to https://github.com/w3c/wptrunner

Oh! You are right. This all only relies on the marionette_driver package. So this change was by accident, and not only works as such. It looks like this is fallback code for a very early marionette client, which may no longer be supported. It should be updated.

In case of my Marionette harness changes I will revert those lines for web-platform tests, so that it won't be affected at all by this changes. Thanks for noticing!
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98104

There is no public release on PyPI yet because I'm waiting for this rename + release and bug 1310632. Are your tests still running outside of the tree? If yes, I would really propose that you make use of the test packages everywhere.

Also what kind of crash are you seeing? Can you please hand me over some more details? Would be good to know if that is related to my changes or just an issue on your machine only. Thanks.
Blocks: 1323048
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98398
Attachment #8815859 - Flags: review?(gps) → review+
Comment on attachment 8815859 [details]
Bug 1320073 - Rename marionette-client to marionette-harness and release version 4.0.0.

https://reviewboard.mozilla.org/r/96632/#review98104

The tests are now running intree for automation purposes. I believe they all install from intree, so should be fine, as in the above try run. This issue ocurrs when testing these changes locally.

Steps to repro:
- Create a venv: `virtualenv venv`
- Activate `source venv/<Scripts/bin>/activate`
- Install new versions of driver from `testing/marionette/client` and harness from `/testing/marionette/harness` to avoid missing packages
- Navigate to `dom/media/test/external`
- Setup external tests `python setup develop` (will need to have removed inconsistent req from the external tests)
- Run the tests ` external-media-tests --binary <FF Binary>

Stack trace:
```
Traceback (most recent call last):
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\marionette_harness-4.0.0-py2.7.egg\marionette_harness\marionette_test\testcases.py", line 147, in run
    self.setUp()
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\firefox_puppeteer\mixins.py", line 85, in setUp
    self.marionette.navigate(self.puppeteer.prefs.get_pref('browser.newtab.url'))
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\marionette_driver-2.2.0-py2.7.egg\marionette_driver\marionette.py", line 1637, in navigate
    self._send_message("get", {"url": url})
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\marionette_driver-2.2.0-py2.7.egg\marionette_driver\decorators.py", line 23, in _
    return func(*args, **kwargs)
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\marionette_driver-2.2.0-py2.7.egg\marionette_driver\marionette.py", line 718, in _send_message
    self._handle_error(err)
  File "c:\projects\mozilla-central\dom\media\test\external\venv2\lib\site-packages\marionette_driver-2.2.0-py2.7.egg\marionette_driver\marionette.py", line 751, in _handle_error
    raise errors.lookup(error)(message, stacktrace=stacktrace)
InvalidArgumentException: Malformed URL: null is not a valid URL.
```

I appreciate this isn't the normal route by which the tests are run (and that some are coming from in tree and others from PyPi). If all of these packages are released at the same time to PyPi, or if the new firefox-puppeteer is released before these then we should be fine. Is there a particular rlease order you anticipate? However, if these versions are bumped and packages released before firefox-puppeteer then local runs will be busted, which I'd prefer to avoid.
(In reply to Bryce Van Dyk (:SingingTree) from comment #40)
> The tests are now running intree for automation purposes. I believe they all
> install from intree, so should be fine, as in the above try run. This issue
> ocurrs when testing these changes locally.
> 
> Steps to repro:
> - Create a venv: `virtualenv venv`
> - Activate `source venv/<Scripts/bin>/activate`
> - Install new versions of driver from `testing/marionette/client` and
> harness from `/testing/marionette/harness` to avoid missing packages
> - Navigate to `dom/media/test/external`
> - Setup external tests `python setup develop` (will need to have removed
> inconsistent req from the external tests)
> - Run the tests ` external-media-tests --binary <FF Binary>

Similar to all the other dependencies of external-media-tests you also have to install firefox-puppeteer from testing/marionette/puppeteer/firefox. If you don't do so it will fail indeed.

> I appreciate this isn't the normal route by which the tests are run (and
> that some are coming from in tree and others from PyPi). If all of these
> packages are released at the same time to PyPi, or if the new
> firefox-puppeteer is released before these then we should be fine. Is there
> a particular rlease order you anticipate? However, if these versions are
> bumped and packages released before firefox-puppeteer then local runs will
> be busted, which I'd prefer to avoid.

Both marionette packages will be released first. Reason is that puppeteer still needs some more updates before I can release it. So there will be a couple of days until its release. Keep in mind that other apps should always use hard-coded dependencies, so none of those should be affected.
I did another try build yesterday after a rebase and noticed some failures which most likely occurred due to a bad merge for a conflict. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90b4d6dd43d6

I will get this fixed, and once all is green we can get this patch landed.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d457264614b
Release marionette-driver 2.2.0 and marionette-client 3.3.0. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/476845a2cafb
Rename marionette-client to marionette-harness and release version 4.0.0. r=gps,maja_zf,mtseng,SingingTree
Keywords: leave-open
Whiteboard: [needs pypi releases]
Released the following packages:

Uploading distributions to https://upload.pypi.org/legacy/
Uploading marionette_driver-2.2.0-py2-none-any.whl
[================================] 50458/50458 - 00:00:02
Uploading marionette_driver-2.2.0.tar.gz
[================================] 42788/42788 - 00:00:01

Uploading distributions to https://upload.pypi.org/legacy/
Uploading marionette_client-3.3.0-py2-none-any.whl
[================================] 113576/113576 - 00:00:03
Uploading marionette_client-3.3.0.tar.gz
[================================] 86669/86669 - 00:00:01

For marionette_client we actually had a broken setting in setup.py which referred to the `marionette_client` sub directory instead of `marionette`. Given that this is a non-issue in tree with the  direct follow-up commit, I fixed it temporarily for the PyPI release only.

Going to release the new marionette_harness package in a moment.
Also registered marionette-harness as new package on pypi and submitted the first version:

Uploading distributions to https://pypi.python.org/pypi
Uploading marionette_harness-4.0.0-py2-none-any.whl
[================================] 140271/140271 - 00:00:03
Uploading marionette-harness-4.0.0.tar.gz
[================================] 103386/103386 - 00:00:02

I have also updated the roles for the new packages.

An email with the news will be sent out soon.

If all works fine we will get this backported to aurora soon.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → affected
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1323217
Changelog for marionette_driver 2.2.0 release:

hg log -M -r 86bcafe66592:: --template "{desc|firstline}\n" .

Bug 1141483 - After a restart Marionette doesn't restore the previous context.
Bug 1299216 - Don't care about socket not connected for sock.shutdown() call.
Bug 1299216 - Make use of MOZ_CRASHREPORTER_SHUTDOWN environment variable in Marionette.
Bug 1299216 - Enhance handling of crashes in Marionette.
Bug 1299216 - Remove always parameter from do_process_check decorator.
Bug 1299216 - Wait for process exit first before checking for crashes.
Bug 1316552 - Marionette has to force close the application in case of socket failures.
Bug 1308902 - Add l10n module and commands to Marionette.
Bug 1298025 - Move addon installation internally to Marionette.
Bug 1316707 - Remove B2G related code from Marionette client.
Bug 1316987 - Improve handling of preferences for default branch and complex values.
Bug 1150527 - Remove error numbers from Marionette Python client.
Bug 1150527 - Remove unusued InstallGeckoError type.
Bug 1316622 - Rename Marionette command timeouts to setTimeouts.
Bug 1316622 - New timeouts inteface in Marionette Python client.
Bug 1316622 - Deprecate old Marionette timeouts API.
Bug 1316622 - Remove wait utility dependency on Marionette default timeout.
Bug 1316622 - Add timeout reset API to Marionette Python client.
Bug 1318644 - Auto-detect application type if binary has been specified.
Bug 1320099 - Move marionette_driver docs to testing/marionette/client.
Bug 1319024 - Sync default testing preferences for Marionette.
Bug 1103196 - Add insecure certificate error.
Bug 1299414 - Always reset timeout parameters for a new session.
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.
Bug 1321278 - Rename processId capability to moz:processID.
Bug 1321278 - Return profile directory in capabilities.
Bug 1321775 - Better handling for client property in Marionette class.
Bug 1320643 - Use device manager directly when forwarding Marionette port.
Bug 1322721 - Fix test_geckoinstance.py to pass for Fennec.

Changelog for marionette_client 3.3.0 release:

hg log -M -r 86bcafe66592:: --template "{desc|firstline}\n" .

Bug 1311676 - Allow MRO for Marionette testcase classes.
Bug 1141483 - After a restart Marionette doesn't restore the previous context.
Bug 1309318 - Make the httpd server available to TestCase class to allow custom handlers.
Bug 1259055 - Update Marionette unit tests to use correct handles for chrome windows.
Bug 1259055 - Use window management class for handling of new tabs.
Bug 1259055 - Use window management class for handling of new windows.
Bug 1315522 - Marionette command-line option --addon does not work.
Bug 1299216 - Enhance handling of crashes in Marionette.
Bug 1315506 - Remove app.update.url.override preference.
Bug 1316800 - Remove session copy of Marionette harness.
Bug 1308902 - Add l10n module and commands to Marionette.
Bug 1316707 - Remove Marionette unit tests for B2G.
Bug 1316707 - Remove Marionette B2G update tests.
Bug 1316707 - Remove B2G related code from Marionette harness.
Bug 1316707 - Remove B2G related code from Marionette client.
Bug 1316987 - Improve handling of preferences for default branch and complex values.
Bug 1316707 - Remove Marionette unit tests for B2G.
Bug 1317462 - Remove version capability from Marionette.
Bug 1317462 - Remove XULappId capability from Marionette.
Bug 1317462 - Remove appBuildId capability from Marionette.
Bug 1317462 - Remove platform capability from Marionette.
Bug 1317462 - Remove screenshot capabilities from Marionette.
Bug 1317462 - Add test for processId capability.
Bug 1150527 - Remove error numbers from Marionette Python client.
Bug 1317386 - Add test for overlay element after scroll.
Bug 1317386 - Test pointer interactability of first element in paint order.
Bug 1317386 - Swap expectation of which button causes scroll.
Bug 1316622 - Rename Marionette command timeouts to setTimeouts.
Bug 1316622 - New timeouts inteface in Marionette Python client.
Bug 1316622 - Move Marionette unit tests to new timeouts API.
Bug 1316622 - Move Marionette harness tests to new timeouts API.
Bug 1316622 - Correct Marionette timeouts tests.
Bug 1316622 - Remove wait utility dependency on Marionette default timeout.
Bug 1316622 - Add timeout reset API to Marionette Python client.
Bug 1318644 - Auto-detect application type if binary has been specified.
Bug 1320099 - Move marionette_driver docs to testing/marionette/client.
Bug 1297551 - Avoid cancelling content timeout callback.
Bug 1103196 - Add HTTPS fixture server for Marionette.
Bug 1103196 - Add ability to ignore invalid TLS certificates.
Bug 1312674 - Navigating to about:blank for a new docshell should not timeout.
Bug 1299414 - Always reset timeout parameters for a new session.
Bug 1279203 - Make Get Page Source command spec conformant.
Bug 1321278 - Rename processId capability to moz:processID.
Bug 1321278 - Return profile directory in capabilities.
Bug 1320643 - Let HTTPDs bind to custom interface.
Bug 1320643 - Bind fixture servers to public interface when testing Fennec.
Bug 1320629 - Increase timeout of test_window_set_timeout_is_not_cancelled.
Bug 1322721 - Fix test_geckoinstance.py to pass for Fennec.

Changelog for marionette_harness 4.0.0 release:

hg log -M -r 8d457264614b:: --template "{desc|firstline}\n" .

Bug 1320073 - Rename marionette-client to marionette-harness.

I will send an email to the Marionette list tomorrow.
Keywords: leave-open
Whiteboard: [needs pypi releases]
No longer blocks: 1323048
All tests on mozilla-central are succeeding. So please uplift the test-only patch to aurora. Please do also land the patch on bug 1323217 with the same merge. Thanks.
Whiteboard: [checkin-needed-aurora]
Some deps haven't been landed on aurora yet and cause merge bustage. I will request those uplifts first.
Whiteboard: [checkin-needed-aurora]
(In reply to Henrik Skupin (:whimboo) from comment #49)
> All tests on mozilla-central are succeeding. So please uplift the test-only
> patch to aurora. Please do also land the patch on bug 1323217 with the same
> merge. Thanks.

We can now successfully apply this patches to aurora. Please uplift together with the patch on bug 1323217. Thanks.
Whiteboard: [checkin-needed-aurora]
Whiteboard: [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: