Closed Bug 811583 Opened 7 years ago Closed 7 years ago

[WebAPI] WebProximitySensor: Develop test(s) to verify the Proximity Sensor WebAPI

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: rwood, Assigned: rwood)

Details

Attachments

(1 file, 1 obsolete file)

Develop Marionette test(s) (to run on the B2G device emulator) that will verify the Proximity Sensor WebAPI. Note, not too much is possible on the emulator other than setting/faking a proximity sensor reading, and verifying that can be retrieved as well as the corresponding events are fired as expected.
Hi Doug if you could please have a quick look when you have a chance that'd be great, just want to make sure I am using the callbacks correctly etc. Thanks
Attachment #684151 - Flags: feedback?(doug.turner)
Comment on attachment 684151 [details] [diff] [review]
Add tests for proximity sensor WebAPI

Adding review flag so can get these tests on TBPL.
Attachment #684151 - Flags: review?(dave.hunt)
Comment on attachment 684151 [details] [diff] [review]
Add tests for proximity sensor WebAPI

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

I have a few suggestions, and have not yet been able to run these tests. I'm currently syncing and will then rebuild the emulator so will hopefully be able to run them today.

::: dom/system/tests/marionette/test_proximity_change.js
@@ +14,5 @@
> +  // Until that is fixed, expect 1:0:1 instead of 1:0:0
> +  // expectedEvent = new DeviceProximityEvent("deviceproximity",
> +  //     {value: 1, min: 0, max: 0});
> +  expectedEvent = new DeviceProximityEvent("deviceproximity",
> +      {value: 1, min:0, max: 1});

nit: can we be consistent with whitespace here? I think I would prefer no spaces either side of the : but happy so long as we have consistancy :)

@@ +57,5 @@
> +  log("Getting proximity via emulator command.");
> +
> +  runEmulatorCmd("sensor get proximity", function(result) {
> +    let values = result[0].split(" ")[2];
> +    if (values == expected) {

Use === here

@@ +72,5 @@
> +function verifyEvent() {
> +  // Setup handler and verify 'ondeviceproximity' event
> +  var receivedProxEvent = false;
> +
> +  window.ondeviceproximity = function(proxEvent) {

Can we just call this 'event'?

We set the proximity values and then immediately check them in the emulator, but if this incurred some delay could we miss this event firing?

::: dom/system/tests/marionette/test_proximity_init.js
@@ +16,5 @@
> +  is(event.type, "deviceproximity", "event type");
> +  is(event.value, initValue, "value");
> +  is(event.min, initMin, "min");
> +  is(event.max, initMax, "max");
> +  if (event.value != initValue ||

Use !== here

@@ +27,5 @@
> +  log("Verifying UserProximityEvent constructor.");
> +
> +  let event = new UserProximityEvent("userproximity", {near: true});
> +  is(event.type, "userproximity", "event type");
> +  is(event.near, true, "near");

You can use ok here instead of is

@@ +29,5 @@
> +  let event = new UserProximityEvent("userproximity", {near: true});
> +  is(event.type, "userproximity", "event type");
> +  is(event.near, true, "near");
> +  if (!event.near) {
> +    log("UserProximityEvent not initialized correctly.");

You could just put this as the message of the previous assertion

@@ +43,5 @@
> +  log("Getting sensor status from emulator.");
> +
> +  runEmulatorCmd("sensor status", function(result) {
> +    log("Received sensor status (" + result[4] + ").");
> +    if (result[4] == "proximity: enabled.") {

This block could be simplified to:

    is(result[4], "proximity: enabled.", "Proximity sensor not enabled by default, but should be.");

@@ +67,5 @@
> +
> +  runEmulatorCmd("sensor get proximity", function(result) {
> +    let values = result[0].split(" ")[2];
> +    log("Received emulator proximity (" + values + ").");
> +    if (values == expected) {

As above, you could simplify this to:

    is(value, expected, "Default proximity values");

The expected/actual should then by in any failure messages. With this, you also wouldn't need the expected variable as you'd only be using it once.
Attachment #684151 - Flags: review?(dave.hunt) → review-
Thanks for the comments Dave! New patch attached.
Attachment #684151 - Attachment is obsolete: true
Attachment #684151 - Flags: feedback?(doug.turner)
Attachment #689823 - Flags: review?(dave.hunt)
Comment on attachment 689823 [details] [diff] [review]
Updated Proximity Sensor API tests

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

Looks good, and the tests pass for me. One minor conflict that will need resolving. Also, I noticed that verifyEmulator has been removed from test_proximity_change.js, which I think is fine considering we want to test the API here and not the emulator.

::: testing/marionette/client/marionette/tests/unit-tests.ini
@@ +16,5 @@
>  [include:../../../../../dom/battery/test/marionette/manifest.ini]
>  [include:../../../../../dom/sms/tests/marionette/manifest.ini]
>  [include:../../../../../dom/network/tests/marionette/manifest.ini]
>  [include:../../../../../dom/system/gonk/tests/marionette/manifest.ini]
> +[include:../../../../../dom/system/tests/marionette/manifest.ini]

This is causing a conflict that will need to be resolved before pushing.
Attachment #689823 - Flags: review?(dave.hunt) → review+
Keywords: checkin-needed
Whiteboard: [automation-needed-in-aurora], [automation-needed-in-b2g18]
https://hg.mozilla.org/mozilla-central/rev/afe8da61183f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.