804515 - [B2G] Need screen orientation command in b2g marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Zac C (:zac)]

Some smoke tests require the screen orientation to be changed.

Thus we need this added to B2G's marionette.

http://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/orientation

Comment 1

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

This will not work at the moment because the emulator seems to be broken when it comes to changing the orientation.

This is something I will work on in the next couple of days when trying to get a couple of Marionette tests written for screen orientation.

Comment 2

[Zac C (:zac)]

Regardless of the emulator we should put this on so we can switch orientation on real devices.

Comment 3

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

Zac, so what do you need here? We have the following:

Properties:
window.screen.mozOrientation (current orientation)

Methods:
window.screen.mozLockOrientation(orientation)
window.screen.mozUnlockOrientation(orientation)

Events:
window.screen.onmozorientationchange

Comment 4

[Zac C (:zac)]

Judging by the API docs, mozLockOrientation is the one we are after as we'd want to lock it to a particular state for the duration of the test.

Comment 5

[Stephen Donner [:stephend] Not actively reading bugmail]

This might not help us implement it, but I was pointed to this by "alive1" in #gaia: http://dxr.mozilla.org/mozilla-central/dom/base/test/test_screen_orientation.html.html

Comment 6

[James Lal [:lightsofapollo]]

Jonathan- Can we prioritize the real fix here soon?

Comment 7

[Jonathan Griffin (:jgriffin)]

James and I discussed this; this is definitely non-trivial since there's no API exposed to JS right now that we can use to accomplish this.

Comment 8

[Jonathan Griffin (:jgriffin)]

It turns out that mozLockOrientation does work on the emulator, and likely on device as well (mdas confirmed it worked earlier on the panda).  So, we can implement an API for it.

The issue that Gaia developers will likely care about, though, is that it doesn't work on b2g desktop builds...but that's a separate issue.

Comment 9

[James Lal [:lightsofapollo]]

Also here is some prior art which more or less shims the whole interface out: https://github.com/mozilla/r2d2b2g/blob/master/prosthesis/modules/GlobalSimulatorScreen.jsm

Comment 10

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

Attaching a patch that implements getScreenOrientation and
setScreenOrientation commands.  Try run:

    https://tbpl.mozilla.org/?tree=Try&rev=2dcb403447f7

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8344191 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

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

minor issues, but it's looking good!

::: testing/marionette/client/marionette/marionette.py
@@ +1299,5 @@
> +        """
> +        return self._send_message("getScreenOrientation", "value")
> +
> +    @orientation.setter
> +    def orientation(self, orientation):

I strongly prefer if this was set_orientation, since it maintains the style of the rest of the setters in the API

::: testing/marionette/client/marionette/tests/unit/test_screen_orientation.py
@@ +13,5 @@
> +
> +class TestScreenOrientation(MarionetteTestCase):
> +    def tearDown(self):
> +        self.marionette.orientation = default_orientation
> +        assert self.marionette.orientation == default_orientation

Why are we checking if we can set the orientation on each teardown?

@@ +18,5 @@
> +        super(MarionetteTestCase, self).tearDown()
> +
> +    def test_default_orientation(self):
> +        orientation = self.marionette.orientation
> +        assert orientation == default_orientation

unittest's asserts (in this case, self.assertEquals) should be used in this file, as you get better reporting.

If an 'assert' line fails, then the output error line looks something like "TEST-UNEXPECTED-FAIL | test_log.py test_log.TestLog.test_log_basic | AssertionError" instead of containing specifics like: "TEST-UNEXPECTED-FAIL | test_log.py test_log.TestLog.test_log_basic | AssertionError: 'landscape-primary' != 'asdf'"

@@ +22,5 @@
> +        assert orientation == default_orientation
> +
> +    def test_set_orientation_to_portrait_primary(self):
> +        self.marionette.orientation = "portrait-primary"
> +

the additional newline here is kind of unexpected. Can it be removed, or am I forgetting something in PEP8?

@@ +63,5 @@
> +
> +    def test_set_orientation_to_shorthand_portrait(self):
> +        # Set orientation to something other than portrait-primary first, since the default is
> +        # portrait-primary.
> +        self.marionette.orientation = "landscape-primary"

I would add a check here to make sure that self.marionette.orientation returns 'landscape-primary' before checking the rest, just to make sure we do unset the default.

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8344191 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

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

::: testing/marionette/client/marionette/tests/unit/test_screen_orientation.py
@@ +13,5 @@
> +
> +class TestScreenOrientation(MarionetteTestCase):
> +    def tearDown(self):
> +        self.marionette.orientation = default_orientation
> +        assert self.marionette.orientation == default_orientation

It's meant as a sanity check before the next test runs that we were able to reset the state back to the original.  If we have the wrong preconditions before running the next test, it will be fairly hard to determine if the problem is in the test itself, or if the problem was in the previous test.

But since this will fail the currently running test instead of the new test, it might make more sense to do s/tearDown/setUp/ here.

What do you think?

@@ +18,5 @@
> +        super(MarionetteTestCase, self).tearDown()
> +
> +    def test_default_orientation(self):
> +        orientation = self.marionette.orientation
> +        assert orientation == default_orientation

I sometimes find they add more verbosity than is strictly needed hence use them only when needed, but I don't have any particular feelings on this so I'll go ahead and update all the assertions if that makes you happy.

@@ +22,5 @@
> +        assert orientation == default_orientation
> +
> +    def test_set_orientation_to_portrait_primary(self):
> +        self.marionette.orientation = "portrait-primary"
> +

It was added purely to increase readability.  I can remove it.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Andreas Tolfsen (:ato) from comment #12)
> Comment on attachment 8344191 [details] [diff] [review]
> 0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch
> 
> Review of attachment 8344191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/marionette/client/marionette/tests/unit/test_screen_orientation.py
> @@ +13,5 @@
> > +
> > +class TestScreenOrientation(MarionetteTestCase):
> > +    def tearDown(self):
> > +        self.marionette.orientation = default_orientation
> > +        assert self.marionette.orientation == default_orientation
> 
> It's meant as a sanity check before the next test runs that we were able to
> reset the state back to the original.  If we have the wrong preconditions
> before running the next test, it will be fairly hard to determine if the
> problem is in the test itself, or if the problem was in the previous test.
> 
> But since this will fail the currently running test instead of the new test,
> it might make more sense to do s/tearDown/setUp/ here.
> 
> What do you think?
> 
Ah, that makes sense. I like the idea of just having it in tearDown, so that the test case that causes the failure will be the one to raise the problem. I don't think it's needed in setUp.

> > +        super(MarionetteTestCase, self).tearDown()
> > +
> > +    def test_default_orientation(self):
> > +        orientation = self.marionette.orientation
> > +        assert orientation == default_orientation
> 
> I sometimes find they add more verbosity than is strictly needed hence use
> them only when needed, but I don't have any particular feelings on this so
> I'll go ahead and update all the assertions if that makes you happy.

Yes, not only is it useful locally, but it's most important for TBPL. It gathers the TEST-UNEXPECTED-* failure lines when your test run has failures, so having context is really helpful, and helps with starring intermittents. 
> 
> @@ +22,5 @@
> > +        assert orientation == default_orientation
> > +
> > +    def test_set_orientation_to_portrait_primary(self):
> > +        self.marionette.orientation = "portrait-primary"
> > +
> 
> It was added purely to increase readability.  I can remove it.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

Submitted a revised patch that addresses the concerns raised above.

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8347231 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

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

Thanks, just address the test_screen_orientation.py in the next patch and then I can r+!

::: testing/marionette/client/marionette/tests/unit/test_screen_orientation.py
@@ +13,5 @@
> +
> +class TestScreenOrientation(MarionetteTestCase):
> +    def tearDown(self):
> +        self.marionette.set_orientation(default_orientation)
> +        assert self.marionette.orientation == default_orientation

there are still a non-unittest asserts here.

@@ +56,5 @@
> +    def test_set_orientation_to_shorthand_portrait(self):
> +        # Set orientation to something other than portrait-primary first, since the default is
> +        # portrait-primary.
> +        self.marionette.set_orientation("landscape-primary")
> +        assert self.marionette.orientation == "landscape-primary"

and here

Comment 16

[Andreas Tolfsen ❲:ato❳]

Those were left there intentionally to make the distinction between
failing tests and incorrect state preventing the program to continue
on.

But we can of course treat them as test failures, if you prefer?
Possibly with a msg argument passed to it to explain the state is
wrong.

Comment 17

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

Revised patch uses assertEqual for checking state.

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Andreas Tolfsen (:ato) from comment #16)
> Those were left there intentionally to make the distinction between
> failing tests and incorrect state preventing the program to continue
> on.
> 
> But we can of course treat them as test failures, if you prefer?
> Possibly with a msg argument passed to it to explain the state is
> wrong.

Yes, if the state is incorrect, it should fail just like any other failure since it's unexpected, and again, this way it will be picked up by TBPL's log parsers.

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8348772 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

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

Thanks! I'll land it soon.

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

land on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bee6174bea6b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Malini Das [:mdas] - Away, not checking bugmail]

uplifted to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/1eaed355e1b4