Closed Bug 1097676 Opened 10 years ago Closed 8 years ago

Create a convenience decorator for "using_context"

Categories

(Testing :: Marionette Client and Harness, defect)

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ahal, Assigned: ShrutiJ, Mentored)

Details

(Keywords: pi-marionette-client, Whiteboard: [good first bug][lang=py])

Attachments

(1 file, 8 obsolete files)

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.
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.
Mentor: jgriffin, ahalberstadt
Whiteboard: [good first bug][lang=py]
Hey can i work on this bug? I have the build, where can i find the code and what is the issue?
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!
Hello. I'm familiar with decorators. do you mean just the using_context(self, context) function?
Flags: needinfo?(ahalberstadt)
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)
Great! I will be uploading a patch shortly.
Attached patch bug1097676_decorators.diff (obsolete) — Splinter Review
Hi, please review. Do you want the commentary removed as well?
Attachment #8696111 - Flags: review?(ahalberstadt)
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)
Hello, I have read about decorators. Can I work on this bug?
Hey Shruti, yes, please feel free to work on it! Let me know if you have any questions.
Attachment #8696111 - Attachment is obsolete: true
Attached file Proposed changes for bug 1097676 (obsolete) —
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)
Attachment #8702220 - Attachment mime type: text/x-python → text/plain
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+
Attached patch bug1097676.diff (obsolete) — Splinter Review
Hello, please review the diff file.
Attachment #8702220 - Attachment is obsolete: true
Attachment #8705629 - Flags: review?(ahalberstadt)
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)
Attached patch bug1097676.diff (obsolete) — Splinter Review
Attachment #8706281 - Flags: feedback?(ahalberstadt)
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+
Flags: needinfo?(shrutijasoria1996)
Flags: needinfo?(shrutijasoria1996)
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
Thanks! I've started working on it. I'll let you know if I have any questions.
Attached patch bug1097676_test.diff (obsolete) — Splinter Review
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)
Attached patch bug1097676.diff (obsolete) — Splinter Review
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)
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)
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)
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.
Attached patch bug1097676.diff (obsolete) — Splinter Review
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)
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)
Oh. I'll make the changes. The patch which I'll submit with these corrections should have both the decorator and the test, right?
Yep, exactly!
Attached patch bug1097676.diff (obsolete) — Splinter Review
Here's the corrected patch.
Attachment #8709419 - Attachment is obsolete: true
Attachment #8709855 - Flags: feedback?(ahalberstadt)
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+
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+
Thanks for your contribution Shruti, and congrats.
https://hg.mozilla.org/mozilla-central/rev/8689d4697905
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Product: Testing → Remote Protocol

Moving bugs for Marionette client due to component changes.

Component: Marionette → Marionette Client and Harness
Product: Remote Protocol → Testing
You need to log in before you can comment on or make changes to this bug.