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)
Testing
Marionette Client and Harness
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)
3.14 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
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•10 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.
Updated•9 years ago
|
Keywords: ateam-marionette-client
Updated•9 years ago
|
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?
Reporter | ||
Comment 3•9 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!
Hello. I'm familiar with decorators. do you mean just the using_context(self, context) function?
Flags: needinfo?(ahalberstadt)
Reporter | ||
Comment 5•8 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)
Hi, please review. Do you want the commentary removed as well?
Attachment #8696111 -
Flags: review?(ahalberstadt)
Reporter | ||
Comment 8•8 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•8 years ago
|
||
Hello, I have read about decorators. Can I work on this bug?
Reporter | ||
Comment 10•8 years ago
|
||
Hey Shruti, yes, please feel free to work on it! Let me know if you have any questions.
Reporter | ||
Updated•8 years ago
|
Attachment #8696111 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8702220 -
Flags: feedback?(ahalberstadt)
Reporter | ||
Updated•8 years ago
|
Attachment #8702220 -
Attachment mime type: text/x-python → text/plain
Reporter | ||
Comment 12•8 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•8 years ago
|
||
Hello, please review the diff file.
Attachment #8702220 -
Attachment is obsolete: true
Attachment #8705629 -
Flags: review?(ahalberstadt)
Reporter | ||
Comment 14•8 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•8 years ago
|
||
Attachment #8706281 -
Flags: feedback?(ahalberstadt)
Reporter | ||
Comment 16•8 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•8 years ago
|
Flags: needinfo?(shrutijasoria1996)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(shrutijasoria1996)
Reporter | ||
Comment 17•8 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•8 years ago
|
||
Thanks! I've started working on it. I'll let you know if I have any questions.
Assignee | ||
Comment 19•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Yep, exactly!
Assignee | ||
Comment 28•8 years ago
|
||
Here's the corrected patch.
Attachment #8709419 -
Attachment is obsolete: true
Attachment #8709855 -
Flags: feedback?(ahalberstadt)
Reporter | ||
Comment 29•8 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•8 years ago
|
||
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 31•8 years ago
|
||
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+
Reporter | ||
Comment 33•8 years ago
|
||
Thanks for your contribution Shruti, and congrats.
Comment 34•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8689d4697905
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•1 year ago
|
Product: Testing → Remote Protocol
Comment 35•1 year ago
|
||
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.
Description
•