[B2G] Need screen orientation command in b2g marionette

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

Testing
Marionette
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: zac, Assigned: ato)

Tracking

unspecified
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
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.
OS: Windows 7 → All
Hardware: x86_64 → All
(Reporter)

Comment 2

6 years ago
Regardless of the emulator we should put this on so we can switch orientation on real devices.
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
(Reporter)

Comment 4

6 years ago
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.
Blocks: 801898
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

Updated

5 years ago
Blocks: 938021

Updated

5 years ago
No longer blocks: 938021
Jonathan- Can we prioritize the real fix here soon?
Flags: needinfo?(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.
Flags: needinfo?(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.
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
Assignee: nobody → ato
(Assignee)

Comment 10

4 years ago
Created attachment 8344191 [details] [diff] [review]
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
Attachment #8344191 - Flags: review?(mdas)
Attachment #8344191 - Flags: review?(jgriffin)
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.
Attachment #8344191 - Flags: review?(mdas) → review-
(Assignee)

Comment 12

4 years ago
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.
(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.
(Assignee)

Comment 14

4 years ago
Created attachment 8347231 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

Submitted a revised patch that addresses the concerns raised above.
Attachment #8344191 - Attachment is obsolete: true
Attachment #8344191 - Flags: review?(jgriffin)
Attachment #8347231 - Flags: review?(mdas)
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
Attachment #8347231 - Flags: review?(mdas) → review-
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8348772 [details] [diff] [review]
0001-Bug-804515-Add-screen-orientation-commands-to-Marion.patch

Revised patch uses assertEqual for checking state.
Attachment #8347231 - Attachment is obsolete: true
Attachment #8348772 - Flags: review?(mdas)
(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 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.
Attachment #8348772 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/bee6174bea6b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
status-b2g-v1.3: --- → fixed
status-b2g-v1.4: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed
You need to log in before you can comment on or make changes to this bug.