Andrew Halberstadt [:ahal] Assignee
	Description • 10 years ago

When doing some test conversion from mozmill to marionette, I found myself needing to switch context almost every second line. For example, one might need to click the back button which requires 'chrome' and then switch to 'content' to assert that the proper page was loaded. And then switch back to 'chrome' to click the forward button, etc..

I think a with syntax could make things a lot simpler:

# content space
with self.marionette.set_context('chrome'):
    # chrome space
    ... do stuff ...
# content space

Or vice versa. Just an idea, feel free to reject it.

	James Lal [:lightsofapollo]
	Comment 1 • 10 years ago

++ fwiw the JS client does something like:

client.scope({ context: 'chrome' }).<chrome stuff>
client.scope({ context: 'content' }).<content stuff>

(Any methods on the object scope returns gets called in that context and the api calls are handled internally by the client code)

	Andrew Halberstadt [:ahal] Assignee
	Comment 2 • 10 years ago

One thing that might be tricky is what to do when we use the with syntax to set the state that we are already in (e.g a function that takes a marionette object as a parameter and doesn't know what the context is). This illustrates the possibilities:

1)
    # chrome space
    with marionette.set_context('chrome'):
        # chrome space
        ... do stuff ...
    # chrome space
     
2)
    # chrome space
    with marionette.set_context('chrome'):
        # chrome space
        ... do stuff ...
    # content space

Imo 2) seems like asking for trouble, whereas I don't think 1) is currently possible. We'd need a way to query the server for current context.

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 3 • 10 years ago

I'm sure I've raised this before but I can't find it. +1 for this suggestion.

	Zac C (:zac)
	Comment 4 • 10 years ago

One thing to consider is how it behaves when you switch back to the content frame.

Probably not so much an issue for desktop testing but for Gaia you are often in a frame that's a child of the top content frame, so Marionette would have to remember the hierarchy of frames and switch back into it when the `with` returns to the content and nowhere else right now does Marionette attempt to do that.

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 5 • 10 years ago

(In reply to Zac C (:zac) from comment #4)
> One thing to consider is how it behaves when you switch back to the content
> frame.
> 
> Probably not so much an issue for desktop testing but for Gaia you are often
> in a frame that's a child of the top content frame, so Marionette would have
> to remember the hierarchy of frames and switch back into it when the `with`
> returns to the content and nowhere else right now does Marionette attempt to
> do that.

This isn't currently an issue, so I don't believe it would be an issue here. For example I can launch and switch to an application, then set the context to be chrome, switch back to content, and be in the same frame as I was before switching to chrome.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 6 • 10 years ago

Something else I have noticed today while implementing a test we currently have in Mozmill. In this case I have to call execute_script to retrieve data as generated by Firefox itself. Therefore I have to call "Components.classes". As I have found it, you can only do that when you are in chrome context. Given that this execute_script will be part of a library it doesn't know the current context. Also there is no get_context. That means I have to switch to chrome if necessary, and run this script. But then I'm not sure what to restore. Was it content or chrome before. With the 'with' statement the tests wouldn't have to care about that.

	Andrew Halberstadt [:ahal] Assignee
	Comment 7 • 10 years ago

We can solve this a few ways:

1) Add an API to get the context from the server. AutomatedTester was opposed to this since the user should "know" what context the marionette instance is in. Though that argument doesn't take into account cases like whimboo mentioned in comment 6.
2) Keep track of the context on the client side. This would be easiest but could run into problems if there are multiple clients connected.
3) Do something hacky like:
def get_context():
    return marionette.execute_script("""
             try {
               Cc;
               return "chrome";
             } catch(e) {
               return "content";
             }
           """)

This is probably not a good idea... though I guess libraries could do this if they really wanted to.

	David Burns :automatedtester
	Comment 8 • 10 years ago

My reason for being opposed to this is that I am of the opinion that a test should know what context it is in, if you are switching it then you go to X and then you switch back to Y.

doing a number of steps and then switching context and not know where you are seems like poor practise since the test is not deterministic in my opinion. The test should start from a known state and then finish in a known state so while this is a "new" test suite we should keep that practise going. 

If we are going from context to context continually perhaps we need to see how we can rewrite the test in a format that would minimise it. This could be in the form of multiple tests.

My argument is that I want to keep things deterministic so that if people who dont work on the code(new contributors) can look at it and figure out what the state should be in when the tests fail and fix them easily.

	Andrew Halberstadt [:ahal] Assignee
	Comment 9 • 10 years ago

I see where you're coming from, but I think if anything, this proposal makes things *more* deterministic, not less.

Currently, this happens:

lib.py

    def do_something(marionette):
        marionette.set_context("chrome")


test.py

    from lib import do_something

    def my_test():
        # running test in content scope
        do_something(marionette)
        # all of a sudden running test in chrome scope... wtf?


This situation isn't uncommon. Whimboo ran into it in comment 6, here's an example of it happening in the wild:
http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/decorators.py#60

If we had a way of knowing what the state of the context in marionette server was, the "do_something" method would be able to clean up after itself and keep things deterministic.

What if there was a "getContext" method on the server, but the python client only used it internally so it was never exposed to test authors? Would that be an acceptable compromise?

	Andrew Halberstadt [:ahal] Assignee
	Comment 10 • 10 years ago

Something like this is what I meant. There's no public facing "get_context" API and the context manager can be used to implicitly revert the marionette object to the proper state, so it's the best of both worlds (I think?).

Usage:
with marionette.using_context("chrome"):
   ... do stuff ...

Note: I wanted to re-use marionette.set_context, but I couldn't figure out a way to do that so invented using_context instead.

	David Burns :automatedtester
	Comment 11 • 10 years ago

Comment on attachment 8512244 [details] [diff] [review]
Add a marionette.using_context() context manager

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

I think that this is a good compromise.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 12 • 10 years ago

Comment on attachment 8512244 [details] [diff] [review]
Add a marionette.using_context() context manager

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

::: testing/marionette/client/marionette/tests/unit/test_with_using_context.py
@@ +13,5 @@
> +        self.marionette.set_context("content")
> +
> +    def _get_context(self):
> +        return self.marionette.execute_script("""
> +                 if (Components.classes === undefined) {

Shouldnt this be simply 'Components === undefined'? I think that this should raise because you are trying to access a property of a non-existent variable.

	Andrew Halberstadt [:ahal] Assignee
	Comment 13 • 10 years ago

Actually Components does exist in content scope (test it out in the console).. though there's a deprecation warning so you're right that I probably shouldn't use it as a test.

	Andrew Halberstadt [:ahal] Assignee
	Comment 14 • 10 years ago

This fixes up the tests and adds a few more.

Try runs:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=688d3ab7d4a0
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=87b53451ba29

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 10 years ago

Comment on attachment 8512921 [details] [diff] [review]
Add a marionette.using_context() context manager

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

::: testing/marionette/client/marionette/marionette.py
@@ +967,5 @@
>          assert(context == self.CONTEXT_CHROME or context == self.CONTEXT_CONTENT)
>          return self._send_message('setContext', 'ok', value=context)
>  
> +    @contextmanager
> +    def using_context(self, context):

Wouldn't with marionette.set_context("chrome") be better?  This would replicate the behaviour of the built-in open:

    fh = open(…)

    with open(…) as fh:
        …

@@ +978,5 @@
> +         `CONTEXT_CHROME` or `CONTEXT_CONTENT`.
> +
> +        Usage example:
> +
> +        ::

Usage example:: will save us two lines.

::: testing/marionette/client/marionette/tests/unit/test_with_using_context.py
@@ +10,5 @@
> +    def setUp(self):
> +        MarionetteTestCase.setUp(self)
> +        test_url = self.marionette.absolute_url('empty.html')
> +        self.marionette.navigate(test_url)
> +        self.marionette.set_context("content")

Assert that the switch was successful and that we're in the correct context before running the test.

@@ +13,5 @@
> +        self.marionette.navigate(test_url)
> +        self.marionette.set_context("content")
> +
> +    def is_chrome_context(self):
> +        return self.marionette._send_message('getContext', 'value') == 'chrome'

self.marionette.CONTEXT_CHROME

@@ +16,5 @@
> +    def is_chrome_context(self):
> +        return self.marionette._send_message('getContext', 'value') == 'chrome'
> +
> +    def test_set_different_context_using_with_block(self):
> +        self.assertFalse(self.is_chrome_context())

We don't need this if we assert that we're in the right context in setUp.

@@ +18,5 @@
> +
> +    def test_set_different_context_using_with_block(self):
> +        self.assertFalse(self.is_chrome_context())
> +        with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
> +            self.assertTrue(self.is_chrome_context())

I think an assert_is_chrome function that uses assertEquals would be better because this will give a “True is not False” type of error message if it fails.

@@ +47,5 @@
> +
> +    def test_exception_raised_while_in_with_block_is_propagated(self):
> +        with self.assertRaises(MarionetteException):
> +            with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
> +                raise MarionetteException

Missing assertion on which context is active after exception abruptly exits the using_context function.

::: testing/marionette/marionette-server.js
@@ +679,5 @@
>  
>    /**
> +   * Gets the context of the server, either 'chrome' or 'content'.
> +   */
> +  getContext: function MDA_getContext() {

MDA_getContext isn't necessary anymore, but won't hurt if you want it to bear stylistic resemblance with the rest of the code in this file.

	Andrew Halberstadt [:ahal] Assignee
	Comment 16 • 10 years ago

Thanks for the review! I completely agree re: "set_context()" vs "using_context()".. I spent a fair amount of time trying to support both the new and old syntax just using "set_context()", but failed.

We basically need a way to tell whether a call to "set_context()" is being used as part of a with statement or not, and I don't think that's possible. In the stdlib they use the number of parameters to differentiate (e.g "with assertRaises(Exception)" vs "assertRaises(Exception, func)")[1], but in our case that's not an option. I'll stare at it a little longer, but I'm not optimistic I'll be able to get it working.

I'll upload a new patch to address the other comments.

[1] https://github.com/python/cpython/blob/master/Lib/unittest/case.py#L154

	Andrew Halberstadt [:ahal] Assignee
	Comment 17 • 10 years ago

Also the reason open() differs from our case is that both open() and with open() return an instance of an object. In our case we need with set_context() to return an instance and set_context() to execute some code.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 10 years ago

Thanks for the careful investigation, I see what you mean now.  It appears that marking set_context as a property and returning an uninitialised object also doesn't work because the with statement checks if the object implements the __exit__ interface.

I think your current solution is Good Enough.

	Andrew Halberstadt [:ahal] Assignee
	Comment 20 • 10 years ago

/r/153 - Bug 1088905 - Support with syntax when setting marionette's context, r=ato
/r/155 - Fix review comments

Pull down these commits:

hg pull review -r 14059074d85bb20890208e6d02570b08b029c70d

	Andrew Halberstadt [:ahal] Assignee
	Comment 21 • 10 years ago

/r/153 - Bug 1088905 - Support with syntax when setting marionette's context, r=ato
/r/155 - Fix review comments

Pull down these commits:

hg pull review -r 14059074d85bb20890208e6d02570b08b029c70d

	Andrew Halberstadt [:ahal] Assignee
	Comment 22 • 10 years ago

/r/153 - Bug 1088905 - Support with syntax when setting marionette's context, r=ato
/r/155 - Fix review comments

Pull down these commits:

hg pull review -r 14059074d85bb20890208e6d02570b08b029c70d

	Andrew Halberstadt [:ahal] Assignee
	Comment 23 • 10 years ago

Comment on attachment 8516300 [details]
MozReview Request: bz://1088905/ahal

I'm not 100% sure what just happened.. I think mozreview was supposed to set the flag automatically, but I think I published first by accident.. just setting it manually for now.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 10 years ago

https://reviewboard.mozilla.org/r/151/#review101

::: testing/marionette/client/marionette/marionette.py
(Diff revision 1)
> +from contextlib import contextmanager

Create a separate group below for standard library `from x import y` style imports.

::: testing/marionette/client/marionette/marionette.py
(Diff revision 1)
> +        Sets the context that marionette commands are running in using

s/marionette/Marionette/

::: testing/marionette/client/marionette/marionette.py
(Diff revision 1)
> +        a 'with' statement. The state of the context on the server is

Use `with` instead

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 10 years ago

https://reviewboard.mozilla.org/r/151/#review103

	Andrew Halberstadt [:ahal] Assignee
	Comment 26 • 10 years ago

/r/153 - Bug 1088905 - Support with syntax when setting marionette's context, r=ato
/r/155 - Fix review comments
/r/209 - Fix review comments round 2

Pull down these commits:

hg pull review -r d6391c3c8e50afc9529ab1cb134ad2041855c331

	Andreas Tolfsen ❲:ato❳
	Comment 27 • 10 years ago

https://reviewboard.mozilla.org/r/151/#review105

	Andreas Tolfsen ❲:ato❳
	Comment 28 • 10 years ago

https://reviewboard.mozilla.org/r/151/#review107

Ship It!

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fca687b28092