813774 - [WebAPI] ScreenOrientation: Develop tests to verify locking screen orientation

Status: RESOLVED WONTFIX

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Develop marionette tests (to run on the device emulator) that will verify locking the orientation via the Screen Orientation WebAPI.

Include the following test cases from QA's test plan (https://wiki.mozilla.org/B2G/QA/WebAPI_Test_Plan/Screen_Orientation):

- Lock orientation to each different value. (Rotate to different value first). Verify mozOrientation changes as expected and onmozorientationchange fires

- Lock orientation to the current value. Verify onmozorientationchange doesn't fire

Comment 1

[Robert Wood [:rwood]]

Note that Whimboo has created a marionette emulator screen orientation class, which is available for Python marionette tests currently:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/emulator_screen.py

More information about Python for Marionette tests:

https://developer.mozilla.org/en-US/docs/Marionette/Marionette_Python_Tests

Comment 2

[Robert Wood [:rwood]]

Attachment: WebAPI test for locking screen orientation

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 696394 [details] [diff] [review]
WebAPI test for locking screen orientation

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

Sorry that it has been taken a bit longer. It was falling through by all those many review requests I had in my queue. There are some things to clarify and clean-up but mostly looks fine.

::: dom/base/test/marionette/test_orientation_lock.py
@@ +5,5 @@
> +    def setUp(self):
> +        # code to execute before any tests are run
> +        MarionetteTestCase.setUp(self)
> +        self.marionette.log("Initializing.")
> +        screen = self.marionette.emulator.screen

I would make `screen` a class variable so you do not have to use the fullname in each test.

@@ +29,5 @@
> +        self.do_verify_event()
> +
> +    def test_portrait_primary(self):
> +        screen = self.marionette.emulator.screen
> +        # Already PORTRAIT_PRIMARY so change first, so will get event when lock

Is the setup step run for each individual test? If not why are we still in portrait primary mode? Aren't the test getting run in sequence, and always in the same order?

@@ +33,5 @@
> +        # Already PORTRAIT_PRIMARY so change first, so will get event when lock
> +        self.marionette.log("Changing orientation to LANDSCAPE_PRIMARY first.")
> +        screen.orientation = screen.SO_LANDSCAPE_PRIMARY
> +        # Sleep to let the orientation change actually happen
> +        time.sleep(1)

Isn't it a bad value for the sleep call? I think it will cause the test to fail if the machine is stressed. Do we have any better default timeout value?

@@ +69,5 @@
> +            lock_orientation + "'.")
> +        result = self.marionette.execute_script("""
> +        let result = window.screen.mozLockOrientation(arguments[0]);
> +        return result;
> +        """, script_args=[lock_orientation])

nit: broken indentation for the `execute_script()` block. Also why don't you directly return but create an extra `return` variable?

@@ +80,5 @@
> +        # Verify window.screen.orientation has expected value
> +        result = self.marionette.execute_script("""
> +        let myScreen = window.screen;
> +        return myScreen.mozOrientation;
> +        """)

Same as above.

Comment 4

[Robert Wood [:rwood]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Comment on attachment 696394 [details] [diff] [review]
> WebAPI test for locking screen orientation
> 
> Review of attachment 696394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry that it has been taken a bit longer. It was falling through by all
> those many review requests I had in my queue. There are some things to
> clarify and clean-up but mostly looks fine.
> 
> ::: dom/base/test/marionette/test_orientation_lock.py
> @@ +5,5 @@
> > +    def setUp(self):
> > +        # code to execute before any tests are run
> > +        MarionetteTestCase.setUp(self)
> > +        self.marionette.log("Initializing.")
> > +        screen = self.marionette.emulator.screen
> 
> I would make `screen` a class variable so you do not have to use the
> fullname in each test.

Done.

> 
> @@ +29,5 @@
> > +        self.do_verify_event()
> > +
> > +    def test_portrait_primary(self):
> > +        screen = self.marionette.emulator.screen
> > +        # Already PORTRAIT_PRIMARY so change first, so will get event when lock
> 
> Is the setup step run for each individual test? If not why are we still in
> portrait primary mode? Aren't the test getting run in sequence, and always
> in the same order?

Yes the setup is run for every test case.

> 
> @@ +33,5 @@
> > +        # Already PORTRAIT_PRIMARY so change first, so will get event when lock
> > +        self.marionette.log("Changing orientation to LANDSCAPE_PRIMARY first.")
> > +        screen.orientation = screen.SO_LANDSCAPE_PRIMARY
> > +        # Sleep to let the orientation change actually happen
> > +        time.sleep(1)
> 
> Isn't it a bad value for the sleep call? I think it will cause the test to
> fail if the machine is stressed. Do we have any better default timeout value?

I don't understand what you mean. Without a pause here of 1 second the test will fail, as it needs time to apply the new orientation.

> @@ +69,5 @@
> > +            lock_orientation + "'.")
> > +        result = self.marionette.execute_script("""
> > +        let result = window.screen.mozLockOrientation(arguments[0]);
> > +        return result;
> > +        """, script_args=[lock_orientation])
> 
> nit: broken indentation for the `execute_script()` block. Also why don't you
> directly return but create an extra `return` variable?

Done.

> 
> @@ +80,5 @@
> > +        # Verify window.screen.orientation has expected value
> > +        result = self.marionette.execute_script("""
> > +        let myScreen = window.screen;
> > +        return myScreen.mozOrientation;
> > +        """)
> 
> Same as above.

Done.

Thanks Henrik. Will attach new patch.

Comment 5

[Robert Wood [:rwood]]

Attachment: Updated screen orientation lock test

New patch updated as per review comments.

Comment 6

[Robert Wood [:rwood]]

Attachment: Newest test and manifest

New patch with test name change and some improvements.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 721879 [details] [diff] [review]
Newest test and manifest

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

Aside from the sleep, just minor nits.

::: dom/base/test/marionette/test_screen_orientation_lock.py
@@ +42,5 @@
> +    def test_portrait_primary_noevent(self):
> +        # Verify locking orientation to SO_PORTRAIT_PRIMARY
> +        self.do_test(self.screen.SO_PORTRAIT_PRIMARY)
> +        # Already was PORTRAIT_PRIMARY so don't expect event this time
> +        got_event = self.marionette.execute_script("return " +

Nit: I'd put this all on one line.

@@ +54,5 @@
> +        self.do_verify_event()
> +
> +    def do_test(self, lock_orientation):
> +        # Setup event handler
> +        self.marionette.execute_script("""

I'm never sure what the best indentation for multi-line strings is. It never looks quite right to me. The new lines and indentation is preserved, which would suggest that we should use JavaScript indentation of two spaces. I'm not really suggesting a change here, if this is working then it looks good to me. :P

@@ +61,5 @@
> +            window.wrappedJSObject.event = true;
> +            window.screen.onmozorientationchange = null;
> +        };
> +        """)
> +        self.marionette.log("Locking emulator orientation to '" +

Nit: Use string formatting and stick to one line.

@@ +68,5 @@
> +            return window.screen.mozLockOrientation(arguments[0]);
> +            """, script_args=[lock_orientation])
> +        self.assertTrue(result, "mozLockOrientation returns true")
> +        # Sleep to let the orientation change actually happen
> +        time.sleep(5)

As mentioned in the other bug, if we could make the assertion in the verify method wait, then we shouldn't need this sleep. If I get my emulator build running then I'll take a little look at this.

@@ +80,5 @@
> +        self.assertEqual(result, orientation_to_test, "correct mozOrientation")
> +
> +    def do_verify_event(self):
> +        # Verify recived 'mozorientationchange' event
> +        got_event = self.marionette.execute_script("return " +

Nit: I'd call this simply event and put it all on one line.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Okay, so I initially tried using execute_async_script to wait for the orientation to change, but the onmozorientationchange event handler interrupts this and causes Marionette to get out of sync.

I then tried putting the wait on the Python side, which was successful but not particularly clean (in my opinion) using:

def do_verify_orientation(self, orientation_to_test):
    # Verify window.screen.orientation has expected value
    timeout = 60 + time.time()
    while time.time() < timeout:
        time.sleep(0.5)
        if self.marionette.execute_script('return window.screen.mozOrientation') == orientation_to_test:
            break
        else:
            raise Exception()

Thinking more about this, I believe the cleanest solution would be to set a class variable for the initial orientation, and if this differs from the expected orientation wait for the event and the resulting orientation value. This is similar to the battery status tests.

Comment 9

[Robert Wood [:rwood]]

(In reply to Dave Hunt (:davehunt) from comment #7)
> Comment on attachment 721879 [details] [diff] [review]
> Newest test and manifest
> 
> Review of attachment 721879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Aside from the sleep, just minor nits.
> 
> ::: dom/base/test/marionette/test_screen_orientation_lock.py
> @@ +42,5 @@
> > +    def test_portrait_primary_noevent(self):
> > +        # Verify locking orientation to SO_PORTRAIT_PRIMARY
> > +        self.do_test(self.screen.SO_PORTRAIT_PRIMARY)
> > +        # Already was PORTRAIT_PRIMARY so don't expect event this time
> > +        got_event = self.marionette.execute_script("return " +
> 
> Nit: I'd put this all on one line.

Done.

> 
> @@ +54,5 @@
> > +        self.do_verify_event()
> > +
> > +    def do_test(self, lock_orientation):
> > +        # Setup event handler
> > +        self.marionette.execute_script("""
> 
> I'm never sure what the best indentation for multi-line strings is. It never
> looks quite right to me. The new lines and indentation is preserved, which
> would suggest that we should use JavaScript indentation of two spaces. I'm
> not really suggesting a change here, if this is working then it looks good
> to me. :P
> 

Yes it works so will leave it as is.

> @@ +61,5 @@
> > +            window.wrappedJSObject.event = true;
> > +            window.screen.onmozorientationchange = null;
> > +        };
> > +        """)
> > +        self.marionette.log("Locking emulator orientation to '" +
> 
> Nit: Use string formatting and stick to one line.
> 

Done.

> @@ +68,5 @@
> > +            return window.screen.mozLockOrientation(arguments[0]);
> > +            """, script_args=[lock_orientation])
> > +        self.assertTrue(result, "mozLockOrientation returns true")
> > +        # Sleep to let the orientation change actually happen
> > +        time.sleep(5)
> 
> As mentioned in the other bug, if we could make the assertion in the verify
> method wait, then we shouldn't need this sleep. If I get my emulator build
> running then I'll take a little look at this.
> 

See my reply to the next set of comments.

> @@ +80,5 @@
> > +        self.assertEqual(result, orientation_to_test, "correct mozOrientation")
> > +
> > +    def do_verify_event(self):
> > +        # Verify recived 'mozorientationchange' event
> > +        got_event = self.marionette.execute_script("return " +
> 
> Nit: I'd call this simply event and put it all on one line.

Done.

Comment 10

[Robert Wood [:rwood]]

(In reply to Dave Hunt (:davehunt) from comment #8)
> Okay, so I initially tried using execute_async_script to wait for the
> orientation to change, but the onmozorientationchange event handler
> interrupts this and causes Marionette to get out of sync.
> 
> I then tried putting the wait on the Python side, which was successful but
> not particularly clean (in my opinion) using:
> 
> def do_verify_orientation(self, orientation_to_test):
>     # Verify window.screen.orientation has expected value
>     timeout = 60 + time.time()
>     while time.time() < timeout:
>         time.sleep(0.5)
>         if self.marionette.execute_script('return
> window.screen.mozOrientation') == orientation_to_test:
>             break
>         else:
>             raise Exception()
> 
> Thinking more about this, I believe the cleanest solution would be to set a
> class variable for the initial orientation, and if this differs from the
> expected orientation wait for the event and the resulting orientation value.
> This is similar to the battery status tests.

Thanks Dave for checking into this also. The event fires right away but the orientation value is not changed until after the actual orientation change, which takes a second after the event. There is no way to do this without some kind of sleep. Therefore in order to get these test wrapped up, since they have been ongoing for ages, I am simplifying the test to only verify the orientation change events. If the expected events are received, then the test will pass.

Comment 11

[Robert Wood [:rwood]]

Attachment: WebAPI test for locking screen orientation

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Rob Wood [:rwood] from comment #10)
> (In reply to Dave Hunt (:davehunt) from comment #8)
> > Okay, so I initially tried using execute_async_script to wait for the
> > orientation to change, but the onmozorientationchange event handler
> > interrupts this and causes Marionette to get out of sync.
> > 
> > I then tried putting the wait on the Python side, which was successful but
> > not particularly clean (in my opinion) using:
> > 
> > def do_verify_orientation(self, orientation_to_test):
> >     # Verify window.screen.orientation has expected value
> >     timeout = 60 + time.time()
> >     while time.time() < timeout:
> >         time.sleep(0.5)
> >         if self.marionette.execute_script('return
> > window.screen.mozOrientation') == orientation_to_test:
> >             break
> >         else:
> >             raise Exception()
> > 
> > Thinking more about this, I believe the cleanest solution would be to set a
> > class variable for the initial orientation, and if this differs from the
> > expected orientation wait for the event and the resulting orientation value.
> > This is similar to the battery status tests.
> 
> Thanks Dave for checking into this also. The event fires right away but the
> orientation value is not changed until after the actual orientation change,
> which takes a second after the event. There is no way to do this without
> some kind of sleep. Therefore in order to get these test wrapped up, since
> they have been ongoing for ages, I am simplifying the test to only verify
> the orientation change events. If the expected events are received, then the
> test will pass.

I actually think this can be done. If we're expecting the orientation to change then we could use an execute_async_script that creates an event handler and in addition wait for the correct final orientation. Mind if I give it a go?

Comment 13

[Robert Wood [:rwood]]

I tried something like that, using execute_async_script and having a marionette waitFor there, waiting for the event and orientation value, however I was getting some kind of 'widget' errors and couldn't get it to work (I think something to do with the test being in python but the callback and async script being JS but I'm not sure). No I don't mind at all thanks Dave!

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I also got the widgetFrame is not defined error. :(

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

...and this morning it works! Patch coming up..!

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: WebAPI test for locking screen orientation (with wait)

I was able to resolve the widgetFrame issue by forcing use of a test page, much like the JavaScript tests do. I believe the issue was related to the FTE/Usage app, and we don't want such things to interfere with these tests.

The only thing remaining is to test for a non-event when switching to the same orientation, which might not be so easy. We could do create an unexpected event handler like we do in the battery tests.

Comment 17

[Robert Wood [:rwood]]

I believe screen orientation support is being added to Marionette (bug 804515).