Make use of Marionette's skip_if_e10s decorator instead of our own one

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: whimboo, Assigned: AdityaM, Mentored)

Tracking

Version 3
mozilla44
Points:
---

Firefox Tracking Flags

(e10s-, firefox42 fixed, firefox43 fixed, firefox44 fixed, firefox-esr38 wontfix)

Details

(Whiteboard: [lang=py])

Attachments

(1 attachment, 1 obsolete attachment)

52 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
I have seen the landing of the patch on bug 1184507 and wonder why our Firefox UI tests have their own version of the skip_if_e10s decorator: 

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#92

https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_ui_harness/decorators.py

Chris, would it make sense to just use the Marionette decorator here? Or is anything stopping us from doing that?
Flags: needinfo?(cmanchester)
The fx-ui version is slightly nicer because we have a convenience getter for browserTabsRemoteAutostart (implemented at your request).

I don't think there's any reason not to use the marionette version. They should never have a different result.
Flags: needinfo?(cmanchester)
(Reporter)

Comment 2

3 years ago
Alright. That sounds good. Reason for this change also is that with the move of our tests into m-c we also want to try to get most of our firefox-ui harness code back into Marionette. Maybe as mixins or separate classes. This is work which will be done next quarter. As result I would like to kill as many duplicated code as possible in our tests.

I think that this bug fits perfectly for a mentored bug.
Mentor: hskupin
Whiteboard: [lang=py]
(Assignee)

Comment 3

3 years ago
I would like to be assigned to this bug.

As I Understand, I have to use the Marionette Method 'skip_if_e10' in the decorators.py file.

Do I also need to copy the commented code to the file? If not what is to be written there?
Flags: needinfo?(hskupin)
(Reporter)

Comment 4

3 years ago
Aditya, thanks for your interest. I'm happy to assign this bug to you.

So what needs to be done here is to completely remove the firefox-ui-tests version of skip_if_e10s. Instead import the decorator from the marionette module. See the referenced bug in comment 0 in how to do so. This should be done for every file which uses the decorator. Once done please create a pull request on Github.
Assignee: nobody → adityamotwani
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
tracking-e10s: --- → -
(Assignee)

Comment 5

3 years ago
Running the command:
firefox-ui-tests --binary /usr/bin/firefox firefox_ui_tests/functional/private_browsing/test_about_private_browsing.py

Without any changes gives a Fail Result.

How can I verify the changes I have made to the file are correct if the Test Fails without an any changes?
(Reporter)

Comment 6

3 years ago
You cannot use the system version of Firefox but have to install one of our builds. Given that patches usually land on the mozilla-central branch first, you will need a Nightly build of Firefox. You can grab one from your platform here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

In general each of our branches map to a specific version of Firefox. So this might be the problem here given that privatebrowsing has been changed since the last release what you were testing.
(Assignee)

Comment 7

3 years ago
Thanks for your prompt reply. I finally got the code running and have made the changes in the files.
I have submitted a pull request on github and waiting for your review.
Flags: needinfo?(hskupin)
(Reporter)

Comment 8

3 years ago
Created attachment 8668323 [details] [review]
github_pull_request.txt

Ok, so what I missed to tell is that we usually add a text attachment to the bug which simply contains the URL to the Github PR. That way we can handle a review process here in Bugzilla.

I'm doing that now and will have a look at your patch soonish.
Flags: needinfo?(hskupin)
Attachment #8668323 - Flags: review?(hskupin)
(Reporter)

Comment 9

3 years ago
Comment on attachment 8668323 [details] [review]
github_pull_request.txt

All looks great here. Only a merge fix will have to be done. Please let me know when that happened and I will gladly merge your PR. Thanks!
Flags: needinfo?(adityamotwani)
Attachment #8668323 - Flags: review?(hskupin) → review+
(Assignee)

Comment 10

3 years ago
Fixed the merge conflict as requested.
I don't understand which development branch should I maintain for these changes?

Thanks for the adding me to the repository.What kind of what kind of access levels do I have? :)

Also would love to work on future bugs on QA. Can you point me to some?
Flags: needinfo?(adityamotwani) → needinfo?(hskupin)
(Reporter)

Comment 11

3 years ago
(In reply to Aditya Motwani from comment #10)
> Fixed the merge conflict as requested.

There is one unrelated change you did. Please fix it and I can get it landed afterward. Thanks!

> I don't understand which development branch should I maintain for these
> changes?

Or call it feature branch. I did a quick search and that should be a good read for you:
http://nvie.com/posts/a-successful-git-branching-model/#feature-branches

> Thanks for the adding me to the repository.What kind of what kind of access
> levels do I have? :)

It was necessary to be able to assign the issue/PR to you. And that way we can also see how is actively contributing. With more contributions and confidence you can certainly increase your level over time.

> Also would love to work on future bugs on QA. Can you point me to some?

We have lots of open bugs both for new features and also known issues. It would be good to know which level of knowledge you have in Python. That would make it easier for me to point out a specific bug. For now I would suggest you take a look at http://mzl.la/1M5LRX5, and maybe you already find something interesting for you. Even if a bug is marked as mentored or not, if you need assistance in something I will always be there for help.
Flags: needinfo?(hskupin)
(Assignee)

Comment 12

3 years ago
Created attachment 8669106 [details]
pull_request.txt

Fixed the unrelated change.

I have moderate to good experience in python. Thanks for your help Henrik.
(Reporter)

Comment 14

3 years ago
Comment on attachment 8669106 [details]
pull_request.txt

Just a duplicate of the attachment I have added earlier. So removing it from the list.
Attachment #8669106 - Attachment is obsolete: true
(Reporter)

Comment 15

3 years ago
PR got merged to mozilla-central as:
https://github.com/mozilla/firefox-ui-tests/commit/7c156f07841dc6c549bd1a230d31f5643823d9e4

Thanks for your work on this bug and to get it fixed! Please let me know if you want to work on something else, or maybe do a search for open bugs yourself. You can also reach me on IRC (irc.mozilla.org) in the #automation channel in case for a real-time conversation.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
(Reporter)

Comment 16

3 years ago
The landed code changes actually cause merge conflicts for a necessary backport of bug 1217046. That means I have to backport this patch to all the other remaining branches:

https://github.com/mozilla/firefox-ui-tests/commit/53e636edd273211b9a9e89583081ccdfc1d54a37 (beta)
https://github.com/mozilla/firefox-ui-tests/commit/ec5104636c2b406338af99a6f307cc04db48daaa (release)
https://github.com/mozilla/firefox-ui-tests/commit/69b8dce3471db19037e8abf9b42833b3b88c6961 (esr38)
status-firefox42: --- → fixed
status-firefox43: --- → fixed
status-firefox-esr38: --- → fixed
(Reporter)

Comment 17

3 years ago
I had to revert the landing for mozilla-esr38 because it caused bustage given that the skip decorator is not present in Marionette on that branch.

https://github.com/mozilla/firefox-ui-tests/commit/e5a4d324ac84757efba41f635b2f591a1a23d65e
status-firefox-esr38: fixed → wontfix
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.