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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

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.

Comment 1

[Robert Wood [:rwood]]

Attachment: Add tests for proximity sensor WebAPI

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

Comment 2

[Robert Wood [:rwood]]

Comment on attachment 684151 [details] [diff] [review]
Add tests for proximity sensor WebAPI

Adding review flag so can get these tests on TBPL.

Comment 3

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

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.

Comment 4

[Robert Wood [:rwood]]

Attachment: Updated Proximity Sensor API tests

Thanks for the comments Dave! New patch attached.

Comment 5

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

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afe8da61183f

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a30678eab6f3
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0322b9e13e97