Closed
Bug 760735
Opened 12 years ago
Closed 12 years ago
Write tests for Screen Orientation API
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: gmealer, Assigned: gmealer)
References
()
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
philikon
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
application/javascript
|
Details | |
(deleted),
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
Write tests for Screen Orientation API, as planned here:
https://wiki.mozilla.org/B2G/QA/WebAPI_Test_Plan/Screen_Orientation
Assignee | ||
Comment 1•12 years ago
|
||
This is a work-in-progress. Posting to get some feedback re: functional programming style, since I'm relatively new to it.
Some notes:
The original code is in dom/base. I'm not at all sure where tests for that would properly go (the dom/base/test directory is pretty spare). I punted to dom/tests/marionette, but welcome correction.
Right now, nothing in the test would fire a mozorientationchange event, so I'm keying order off either the emulator callback, or with an explicit setTimeout to pass control between functions.
I'm unsure in cases where I've used setTimeout to chain test functions whether just doing a synchronous call would be legit too. I assume setTimeout keeps the stack from going too deep, but this is one of the areas where I'm a noob.
I don't have tests yet for cycling the X and Y axes. My initial exploration showed some unexpected results doing that, and I want to loop in Mounir to find out if the current behavior is proper before writing tests to it.
I have tests written for cycling through orientation locks, but they don't currently work, as you can only lock orientation as an app or a full-screen page. I need to find out how (if) I can get this test to run in a way that it can lock; then I can uncomment the rest of that function.
I'll follow up presently with more about the unexpected results I got trying the other axis values, and post the testbed I used to get them.
Attachment #629441 -
Flags: feedback?(philipp)
Assignee | ||
Comment 2•12 years ago
|
||
This is the testbed I used to see how mozOrientation behaved for different orientations of the emulator. You can see logs of the results at:
http://people.mozilla.org/~gmealer/orientation/orientation.txt (full 360 degree sweep of all axes)
http://people.mozilla.org/~gmealer/orientation/orientation-x.txt (360 degree sweep of just the x axis)
http://people.mozilla.org/~gmealer/orientation/orientation-y.txt (360 degree sweep of just the y axis)
http://people.mozilla.org/~gmealer/orientation/orientation-z.txt (360 degree sweep of just the z axis)
I got some unusual behavior:
Z axis didn't change orientation, which is expected (Z is towards center of the earth, i.e. up and down, so spinning around it doesn't change anything).
X and Y axes only changed mozOrientation at the ~45 degree mark, which was unexpected.
I would have expected one (whichever axis is flipping the device top-to-bottom on the axis parallel to the horizon) to change near the 90 degree mark to portrait-secondary, and again near 270 degree to portrait primary again. In other words, it'd change to upside down as it passed horizontal, and back as it passed again.
I would have expected the other (whichever axis is rotating the device in the usual portrait->landscape way) to change to landscape-primary near 90, portrait-secondary near 180, landscape-secondary near 270, and back to portrait-primary near 360.
Also, the values seem to be "sticky," as you can see by comparing the separate axis runs to the combined run. The portrait-secondary from X "stuck" at the beginning of the Y run, such that 0:0:0 was primary sometimes and secondary other times. I would have expected fairly deterministic values such that any given Z:X:Y is always a specific orientation.
One possibility is that the emulator and the API don't play nicely somehow.
One other possibility is I'm incorrect about the emulator values being degrees. I tried radians, though, and there was no change from 0-2pi in .5pi increments on any axis. Plus, the change at 45 suggests degrees, switching from mostly-vertical to mostly-horizontal.
I'm not sure yet this is behavior is improper, so want some feedback from Mounir prior to moving forward. How should I be seeing this behave?
Attachment #629443 -
Flags: feedback?(mounir)
Comment 3•12 years ago
|
||
Comment on attachment 629443 [details]
Testbed to display mozOrientation at different emulator orientations
I think there are two things to test here:
1. the DOM API simply by using .mozLockOrientation/.mozOrientation/etc. This can be done with a mochitest (for Android and B2G only). Basically, you would check that calling lock sent the event and change the orientation value and in some situations you can't call lock, etc.
2. the backends (B2G only, using marionette): checking that for some sensor orientation values, the screen orientation is Foo or Bar.
I can help you, give feedback and even revew (1) but you will have to find someone else for (2).
Attachment #629443 -
Flags: feedback?(mounir)
Assignee | ||
Comment 4•12 years ago
|
||
Mounir,
Yeah, I agree re: the breakdown. I'll fill in the rest of the lock tests (1) after I figure out how to make the test run as full screen or an app, so lock works.
The problem is that the sensor orientation tests (2) are giving me unexpected results.
I'm virtually spinning the phone in a complete circle on X, Y, and Z, and it's not traversing all four cardinal orientations on any axis. At least one axis should.
I understand you can't give feedback on how the emulator is being used, but assuming I'm using it correctly can you clarify what the behavior of the Foo and Bar in question should be?
Failing that, who can? I traced your code up to the point where you're getting the true orientation from the aConfiguration structure, but I'm unclear how/where that's getting filled in.
Comment 5•12 years ago
|
||
(In reply to Geo Mealer [:geo] from comment #4)
> Mounir,
>
> Yeah, I agree re: the breakdown. I'll fill in the rest of the lock tests (1)
> after I figure out how to make the test run as full screen or an app, so
> lock works.
You should use requestFullScreen() (see develeoper.mozilla.org).
> The problem is that the sensor orientation tests (2) are giving me
> unexpected results.
>
> I'm virtually spinning the phone in a complete circle on X, Y, and Z, and
> it's not traversing all four cardinal orientations on any axis. At least one
> axis should.
>
> I understand you can't give feedback on how the emulator is being used, but
> assuming I'm using it correctly can you clarify what the behavior of the Foo
> and Bar in question should be?
>
> Failing that, who can? I traced your code up to the point where you're
> getting the true orientation from the aConfiguration structure, but I'm
> unclear how/where that's getting filled in.
As I said, I really have no idea how X,Y,Z should be related to orientation. This is fully up to the backend. On Android, we should heavily test that. Maybe one very simple position for each orientation. On B2G, you should ask the assignee of bug 743638.
I think you should open another bug to test the B2G backend.
Assignee | ||
Comment 6•12 years ago
|
||
OK, so it sounds like I've conflated the back end and your API.
If I understand the responsibilities correctly, then:
1) The back end is responsible for translating an X, Y, Z into a simplified orientation that you consume.
2) Your API is responsible for sending out the mozorientationchange event when the simplified orientation changes.
3) Your API is also responsible for locking/unlocking.
Is that correct?
Comment 7•12 years ago
|
||
The DOM level is responsible of:
- the rules regarding when the lock method can be called;
- the rules regarding which orientation names can be used;
- asking the backend to lock/unlock and current state of the screen orientation;
- dispatching the orientation change event to the content (from the backend's input).
Assignee | ||
Comment 8•12 years ago
|
||
So the good news is that testing this on-device shows that orientation translation and events work correctly in the final stack.
Testbed for that is here: http://people.mozilla.org/~gmealer/exercises/orientation .
So, there's something wonky in how I'm attempting to use the emulator, or in how it behaves. I'm not sure yet if this means we can't test orientation this way or not.
I'll do some tests that try a very full range of values in the emulator to see if I can gain understanding of how it's using the {*:*:*} values you use to set the orientation sensors.
In the meantime, I'll separate out the tests that don't need emulator (sanity check and negatives) into mochitests and get a patch up to get into CI. I expect that to be done on Monday.
I have not yet been able to test locking; in emulator, it's blocked by Bug 761856, and on-device the browser doesn't seem to allow full-screen content at all. I may have to put my code into a standalone app to test it.
Comment 9•12 years ago
|
||
(In reply to Geo Mealer [:geo] from comment #8)
> I have not yet been able to test locking; in emulator, it's blocked by Bug
> 761856, and on-device the browser doesn't seem to allow full-screen content
> at all. I may have to put my code into a standalone app to test it.
The Firefox on Android should allow locking and fullscreen.
Comment 10•12 years ago
|
||
Comment on attachment 629441 [details] [diff] [review]
(WIP) First pass at Marionette tests for Screen Orientation API
Review of attachment 629441 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good superficially. I don't know this API well enough to go beyond that.
Attachment #629441 -
Flags: feedback?(philipp) → feedback+
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 629441 [details] [diff] [review]
> (WIP) First pass at Marionette tests for Screen Orientation API
>
> Review of attachment 629441 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks good superficially. I don't know this API well enough to go beyond
> that.
No worries, I didn't expect domain expertise. Was more concerned about my use of asynchronous functions, since my background is mostly structured/OOP, not functional.
Thanks!
Assignee | ||
Comment 12•12 years ago
|
||
As mentioned before, I've moved basic negative and sanity testing into mochitest so it can run widely.
I haven't put positive tests in here. I have no idea if it's legit for me to attempt to fullscreen during a mochitest run, as locking requires. I'd also need to put in platform selectors as desktop in particular doesn't support locking. If it looks feasible, I'll add them later.
Attachment #633754 -
Flags: review?(mounir)
Comment 13•12 years ago
|
||
Comment on attachment 633754 [details] [diff] [review]
Mochitest for negative/sanity tests of Screen Orientation API
Review of attachment 633754 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the change I suggested below and also, move the test here:
dom/base/test/test_screen_orientation.html
::: dom/tests/mochitest/orientation/test_bug760735.html
@@ +32,5 @@
> +try {
> + var initialOrientation = window.screen.mozOrientation;
> +
> + // Sanity tests
> + ok(initialOrientation !== undefined, "mozOrientation is defined (value=" + initialOrientation + ")");
You could check that the orientation is one of those:
["portrait-primary", "portrait-secondary", "landscape-primary", "landscape-secondary"]
You can do that with:
var allowedOrientations = ["portrait-primary", "portrait-secondary", "landscape-primary", "landscape-secondary"];
isnot(allowedOrientations.indexOf(initialOrientation), -1, "...");
Attachment #633754 -
Flags: review?(mounir) → review+
Comment 14•12 years ago
|
||
And thank you for writing this test :)
Assignee | ||
Comment 15•12 years ago
|
||
Has the r=me changes requested. Resubmitting for review because I don't have level 1 commit, so can't land the patch myself per r=me.
Also, no problem. It's great to be helping out directly!
Attachment #633754 -
Attachment is obsolete: true
Attachment #635493 -
Flags: review?(mounir)
Comment 16•12 years ago
|
||
Comment on attachment 635493 [details] [diff] [review]
Revised mochitests with mounir's requested changes
Review of attachment 635493 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
You said you do not have level 1 commit access so you can't push but you actually need level 3 commit access to push to m-i or m-c. Do you really don't have level 1 commit access or level 3 commit access ?
I can vouch you for the level 1 commit access. Just open a bug to request it and CC me.
Attachment #635493 -
Flags: review?(mounir) → review+
Comment 17•12 years ago
|
||
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d02a0dc39aae
Assignee | ||
Comment 18•12 years ago
|
||
Mounir: sorry, I misspoke. I have level 2 access from my other automation efforts. I did mean mozilla-central/level 3.
For some reason when I wrote that I was thinking 1=higher, not 3=higher.
Thanks for the voucher offer!
Comment 19•12 years ago
|
||
Pushed to mozilla-inbound.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Comment 20•12 years ago
|
||
Please post a changeset link when pushing to inbound...
https://hg.mozilla.org/integration/mozilla-inbound/rev/d592966ede4f
Comment 21•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•