Create a convenience decorator for "using_context"

RESOLVED FIXED in Firefox 46

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ahal, Assigned: ShrutiJ, Mentored)

Tracking

({ateam-marionette-client})

unspecified
mozilla46
ateam-marionette-client
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

3 years ago
While converting some of the firefox-greenlight-tests and libraries to use the new marionette.using_context functionality, I realized that in most cases we want an entire function to execute with a certain scope and then restore the old scope once the function exits.

This is perfect for a decorator.
(Reporter)

Comment 1

3 years ago
I realized there's a case where using a decorator could cause lots of confusion. If the function that is decorated returns an HTMLElement object, then it shouldn't use 'using_context' at all (either in a decorator or not). For example the following code would raise because the returned element doesn't exist in the element map from content space:

def get_browser(marionette):
    with marionette.using_context('chrome'):
        return marionette.find_element('id', 'main-window')

marionette.set_context('content')
browser = get_browser(marionette)
browser.tag_name

I guess this is tangential to decorators, but still it should be noted that elements found in a certain scope can only be manipulated from that scope. Making a decorator may make this bad pattern easier to do. Then again, maybe it's up to library authors to be aware of this limitation.
Keywords: ateam-marionette-client
Mentor: jgriffin@mozilla.com, ahalberstadt@mozilla.com
Whiteboard: [good first bug][lang=py]

Comment 2

2 years ago
Hey can i work on this bug? I have the build, where can i find the code and what is the issue?
(Reporter)

Comment 3

2 years ago
Hi Shivam, you can find the code under "testing/marionette/driver/marionette_driver".

The problem is that we want to make a decorator version of this function:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#1357

It should live in the decorators.py file. In order to work on this, you should be familiar with python decorators. If you aren't, please do some reading on the topic. If that makes sense and you'd like to work on this, let me know and I can assign the bug to you.

Feel free to ask if you have any questions!

Comment 4

2 years ago
Hello. I'm familiar with decorators. do you mean just the using_context(self, context) function?
Flags: needinfo?(ahalberstadt)
(Reporter)

Comment 5

2 years ago
Yep, exactly. Basically a decorator that does:
with marionette.using_context(<scope>):
    return func()

You can copy some of the other decorators in decorators.py
Flags: needinfo?(ahalberstadt)

Comment 6

2 years ago
Great! I will be uploading a patch shortly.

Comment 7

2 years ago
Created attachment 8696111 [details] [diff] [review]
bug1097676_decorators.diff

Hi, please review. Do you want the commentary removed as well?
Attachment #8696111 - Flags: review?(ahalberstadt)
(Reporter)

Comment 8

2 years ago
Comment on attachment 8696111 [details] [diff] [review]
bug1097676_decorators.diff

Review of attachment 8696111 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, we appreciate it! But this doesn't do what you expect. First please brush up on decorators. Next, please make sure this passes all the Marionette unit tests. You can do this by building Firefox [1] and running |mach marionette-test|. For this bug, the only file that should change (apart from adding a test) is decorators.py. Thanks!

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Attachment #8696111 - Flags: review?(ahalberstadt)
(Assignee)

Comment 9

2 years ago
Hello, I have read about decorators. Can I work on this bug?
(Reporter)

Comment 10

2 years ago
Hey Shruti, yes, please feel free to work on it! Let me know if you have any questions.
(Reporter)

Updated

2 years ago
Attachment #8696111 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8702220 [details]
Proposed changes for bug 1097676

I have added a function named decorate_using_context in decorators.py. Please review it and let me know if any changes have to be made to it.
Attachment #8702220 - Flags: feedback?(ahalberstadt)
(Reporter)

Updated

2 years ago
Attachment #8702220 - Attachment mime type: text/x-python → text/plain
(Reporter)

Comment 12

2 years ago
Comment on attachment 8702220 [details]
Proposed changes for bug 1097676

Thanks, that's almost right! But instead of `return func` you need `return func(*args, **kwargs)`.

Please also rename the function to 'using_context' and add a docstring following the same format as 'do_crash_check' above. Bonus points if you can add a test for this as well.

Finally, you'll need to upload a diff of your changes as opposed to the entire file in order for us to get this landed. There are several ways to generate a diff file, but one is `hg export`. Please also see the commit message guidelines here:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

Thanks for your contribution, looking forward to the next patch!
Attachment #8702220 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 13

2 years ago
Created attachment 8705629 [details] [diff] [review]
bug1097676.diff

Hello, please review the diff file.
Attachment #8702220 - Attachment is obsolete: true
Attachment #8705629 - Flags: review?(ahalberstadt)
(Reporter)

Comment 14

2 years ago
Comment on attachment 8705629 [details] [diff] [review]
bug1097676.diff

Review of attachment 8705629 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is great! Just some very minor requests and then I think we'll be ready to land it. Also if you're interested in trying to write a test, you can do so in this file here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_with_using_context.py

One way to write a test would be to create an inner function that is wrapped by the decorator, and then asserting it's in the context that was passed in via the decorator. To run the test, you can run |mach marionette-test| from the root mozilla-central directory.

::: testing/marionette/driver/marionette_driver/decorators.py
@@ +68,5 @@
> +
> +def using_context(func, context):
> +    """ Decorator which allows a function to execute in certain scope
> +    using marionette.using_context functionality and returns to old
> +    scope once the function exits.

Add a :param context: notation with a brief description here, similar to the docstring in 'do_crash_check'. You should mention that valid values are either 'content' or 'chrome'.

@@ +70,5 @@
> +    """ Decorator which allows a function to execute in certain scope
> +    using marionette.using_context functionality and returns to old
> +    scope once the function exits.
> +    """
> +    @wraps(func)  

nit: please remove the whitespace at the end of this line
Attachment #8705629 - Flags: review?(ahalberstadt)
(Assignee)

Comment 15

2 years ago
Created attachment 8706281 [details] [diff] [review]
bug1097676.diff
Attachment #8706281 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 16

2 years ago
Comment on attachment 8706281 [details] [diff] [review]
bug1097676.diff

Review of attachment 8706281 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch! Are you still interested in working on a test? If not I'll amend one to your patch.
Attachment #8706281 - Flags: feedback?(ahalberstadt) → review+
(Reporter)

Updated

2 years ago
Flags: needinfo?(shrutijasoria1996)
(Reporter)

Updated

2 years ago
Flags: needinfo?(shrutijasoria1996)
(Reporter)

Comment 17

2 years ago
Hey Shruti, so for the test you can drop it into this file here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_with_using_context.py

Every function that starts with 'test_' is a new test case. So you'll need to create a new instance method with an appropriate name, and add your test assertions in there. You'll probably need to make an inner function that you can wrap with the decorator, and then put your test assertions inside the inner function.

Try to use the other test functions as a reference, and of course, read up on the python unittest module if you aren't familiar with it already:
https://docs.python.org/2/library/unittest.html

Hope that helps, and let me know if you have any questions.
Assignee: nobody → shrutijasoria1996
(Assignee)

Comment 18

2 years ago
Thanks! I've started working on it. I'll let you know if I have any questions.
(Assignee)

Comment 19

2 years ago
Created attachment 8708467 [details] [diff] [review]
bug1097676_test.diff

Here's the test for the the decorator using_context. Please review it and let me know if any changes have to be made.
Attachment #8708467 - Flags: feedback?(ahalberstadt)
(Assignee)

Comment 20

2 years ago
Created attachment 8708468 [details] [diff] [review]
bug1097676.diff

There was a little mistake in the decorator using_context which I had submitted before, there was an extra brace. I've corrected it in this submission.
Attachment #8708468 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 21

2 years ago
Comment on attachment 8708467 [details] [diff] [review]
bug1097676_test.diff

Review of attachment 8708467 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, though this isn't quite right. You need to pass the context in to the decorator. This also means you'll need two inner functions, one that tests using_context("chrome") and one that tests using_context("content").

Also please make sure you can run the test and that it passes by running:
./mach marionette-test testing/marionette/client/marionette/tests/unit/test_with_using_context.py
Attachment #8708467 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 22

2 years ago
Comment on attachment 8708468 [details] [diff] [review]
bug1097676.diff

Good catch. You don't need to submit a new patch for this though. Just amend it to your old patch and create a new one (you can use hg commit --amend for this). Also, it's good to mark your old patches obsolete if you are uploading a new one that is the same things except with bug fixes.

Thanks for sticking with this, getting close now!
Attachment #8708468 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 23

2 years ago
Also meant to add, please amend the test patch and the regular patch into one. We'll need all the changes in the same patch before it can be landed.
(Assignee)

Comment 24

2 years ago
Created attachment 8709419 [details] [diff] [review]
bug1097676.diff

Here's the patch for the decorator and its test. I've made some changes to the decorator so that it works properly. Also, I've run the test successfully.

Let me know if anymore changes have to be made.
Attachment #8705629 - Attachment is obsolete: true
Attachment #8706281 - Attachment is obsolete: true
Attachment #8708467 - Attachment is obsolete: true
Attachment #8708468 - Attachment is obsolete: true
Attachment #8709419 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 25

2 years ago
Comment on attachment 8709419 [details] [diff] [review]
bug1097676.diff

Review of attachment 8709419 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, though the reason this test is passing is that the inner functions are never getting called, and therefore the assertions aren't getting executed. You'll need to call each inner function from the test. When you do this, you'll get an error, because there is no marionette object in the arguments. So you should also make these inner functions take a marionette object as an argument.

The decorator is right though, just the test isn't doing quite what you expect :).
Attachment #8709419 - Flags: feedback?(ahalberstadt)
(Assignee)

Comment 26

2 years ago
Oh. I'll make the changes. The patch which I'll submit with these corrections should have both the decorator and the test, right?
(Reporter)

Comment 27

2 years ago
Yep, exactly!
(Assignee)

Comment 28

2 years ago
Created attachment 8709855 [details] [diff] [review]
bug1097676.diff

Here's the corrected patch.
Attachment #8709419 - Attachment is obsolete: true
Attachment #8709855 - Flags: feedback?(ahalberstadt)
(Reporter)

Comment 29

2 years ago
Comment on attachment 8709855 [details] [diff] [review]
bug1097676.diff

Perfect thanks!

I applied this locally and tested everything works. Though there was some bitrot which I had to fix (which just means the underlying code has changed since you last pulled your repository).

Since I already fixed it locally, I'll just upload the updated patch (don't worry, you'll still get credit). I'll get :ato to review it as I'm not a marionette peer.
Attachment #8709855 - Flags: feedback?(ahalberstadt) → feedback+
(Reporter)

Comment 30

2 years ago
Created attachment 8710000 [details] [diff] [review]
Create using_context decorator

Uploading an un-bitrotted patch on behalf of Shruti. I've verified this passes the unittest locally.
Attachment #8709855 - Attachment is obsolete: true
Attachment #8710000 - Flags: review?(ato)
Comment on attachment 8710000 [details] [diff] [review]
Create using_context decorator

Review of attachment 8710000 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.  Ship it if you’re confident with your testing.
Attachment #8710000 - Flags: review?(ato) → review+

Comment 32

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8689d4697905
(Reporter)

Comment 33

2 years ago
Thanks for your contribution Shruti, and congrats.

Comment 34

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8689d4697905
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.