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

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 1

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 3

[Aditya Motwani (:AdityaM)]

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?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 5

[Aditya Motwani (:AdityaM)]

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?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 7

[Aditya Motwani (:AdityaM)]

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.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: 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.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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!

Comment 10

[Aditya Motwani (:AdityaM)]

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?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 12

[Aditya Motwani (:AdityaM)]

Attachment: pull_request.txt

Fixed the unrelated change.

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

Comment 13

[Aditya Motwani (:AdityaM)]

Comment on attachment 8669106 [details]
pull_request.txt

>https://github.com/mozilla/firefox-ui-tests/pull/255

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8669106 [details]
pull_request.txt

Just a duplicate of the attachment I have added earlier. So removing it from the list.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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)

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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