Closed Bug 1209087 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: Firefox UI Tests, defect)

Version 3
defect
Not set

Tracking

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

RESOLVED FIXED
mozilla44
Tracking Status
e10s - ---
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed
firefox-esr38 --- wontfix

People

(Reporter: whimboo, Assigned: AdityaM, Mentored)

Details

(Whiteboard: [lang=py])

Attachments

(1 file, 1 obsolete file)

52 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review
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)
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]
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)
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)
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?
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.
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)
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)
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+
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)
(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)
Attached file pull_request.txt (obsolete) —
Fixed the unrelated change.

I have moderate to good experience in python. Thanks for your help Henrik.
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
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.