AmbientLightSensor: Update tests to be compliance with latest ED #4203

Honry · 2016-11-14T09:30:15Z

Add https postfix as all interfaces must only be available within a secure context
Add new tests for onerror event, errored state, sensor object contruction, sensor readings
Update browsing context test
Reorg some tests

wpt-pr-bot · 2016-11-14T09:30:22Z

Reviewers for this pull request are: @Volker-E, @dontcallmedom, @riju, @tobie, and @zqzhang.

tobie · 2016-11-14T09:53:25Z

ambient-light/AmbientLightSensor_browsing_context.https.html

+  sensor.onchange = t.step_func_done(function() {
+    assert_not_equals(sensor.reading, null);
+    let win = window.open('', '_blank');
+    assert_equals(sensor.reading, null);


This might probably change to hold on to the latest reading.

Thanks, fixed in new commit.

tobie · 2016-11-14T09:56:45Z

ambient-light/AmbientLightSensor_onchange.https.html

-  sensor.onchange = t.step_func_done(function(event) {
+  sensor.onchange = t.step_func_done(function() {
+    assert_greater_than_equal(sensor.reading.illuminance, 0);
+    assert_equals(sensor.state, "active");


The state should now be called "activated".

Spec dependence.

https://w3c.github.io/sensors/#dom-sensorstate-activated

tobie · 2016-11-14T09:57:55Z

ambient-light/AmbientLightSensor_onerror-manual.https.html

+});
+
+async_test(function(t) {
+  sensor1.onstatechange = t.step_func_done(function(event) {


We're removing onstatechange this from the spec.

Spec dependence.

https://w3c.github.io/sensors/#dom-sensor-onactivate

tobie · 2016-11-14T10:00:26Z

ambient-light/AmbientLightSensor_reading.https.html

+  let cachedReading = sensor.reading;
+  let cachedIlluminance = cachedReading.illuminance;
+  sensor.stop();
+  assert_equals(cachedReading.illuminance, cachedIlluminance);


You probably want to test for this asynchronously.

Thanks, you are right, fixed in new commit.

tobie · 2016-11-14T10:03:19Z

ambient-light/AmbientLightSensor_reading.https.html

+}, "Test that sensor reading must be immutable.");
+
+test(function() {
+  assert_equals(cachedReading1, cachedReading2);


This should really be: assert(cachedReading1 === cachedReading2); or something similar, imho.

Actually assert_equals can be use for objects comparison as well. Following note is picked from assert_equals from testharness.js.

Test if two primitives are equal or two objects are the same object.

tobie · 2016-11-14T10:03:49Z

ambient-light/AmbientLightSensor_reading.https.html

+  sensor1.stop();
+  assert_equals(sensor1.reading, null);
+  assert_equals(String(sensor2.reading), "[object AmbientLightSensorReading]");
+}, "Test that sensor is independent.");


I don't understand this test.

The test purpose is checking sensors don't interfere each others. After first sensor stops its reading is null, second sensor remains.

tobie · 2016-11-14T10:04:39Z

ambient-light/AmbientLightSensor_reading.https.html

+    assert_not_equals(cachedReading2, cachedReading3);
+    sensor2.stop();
+  }, 2000);
+}, "Test that the sensor reading is updated when time passes.");


This is flaky. You should instead rely on a new onchange event.

Thanks, fixed in new commit.

tobie · 2016-11-14T10:05:13Z

ambient-light/AmbientLightSensor_start.https.html

+}, "The default sensor.state is 'idle'");
+
+async_test(function(t) {
+  sensor.onstatechange = t.step_func_done(function(event) {


onstatechange has been removed.

Spec dependence.

https://w3c.github.io/sensors/#dom-sensor-onactivate

tobie · 2016-11-14T10:06:13Z

ambient-light/AmbientLightSensor_stop.https.html

 });

+async_test(function(t) {
+  sensor.onstatechange = t.step_func_done(function(event) {


onstatechange has been removed.

Spec dependence.

https://w3c.github.io/sensors/#dom-sensor-onactivate

tobie · 2016-11-14T10:06:40Z

ambient-light/AmbientLightSensor_stop.https.html

 test(function() {
-  assert_equals(String(sensor.reading), "null");
+  assert_equals(sensor.reading, null);


This might change. We might want to keep the latest reading around.

Spec dependence.

@tobie, I would like to leave it as is until we make the final conclusion.

Honry · 2016-11-15T01:35:59Z

@tobie, many thanks for your comments, at present tests are compliance to latest WD in order to match what Chromium implementation follows. And I will update them once new WD published. WDYT?

tobie · 2016-11-15T08:25:35Z

@Honry please track the ED not the WD.

Honry · 2016-11-15T08:30:26Z

@tobie, OK, I will update the tests later on.

- Add https postfix as all interfaces must only be available within a secure context - Add new tests for onerror event, errored state, sensor object contruction, sensor readings - Update browsing context test - Reorg some tests

Honry · 2016-11-15T12:49:11Z

@tobie, I have one concern about the condition to call onchange event.
Spec says, onchange event is called whenever a new reading is available. Meanwhile, according to the code snippet in your comment in issues#15, as my understanding, the sensor reading will change while time passing. But when I testing on Chromium, the onchange only can be called when the ambient light illuminance changes. Who is correct?

tobie · 2016-11-15T12:50:52Z

This is still in flux.

Honry · 2016-11-15T13:03:44Z

I think I should prioritize following the spec. :)

Honry · 2016-11-15T13:12:37Z

@tobie, Just update tests to latest ED, pls. help review. Thanks!

tobie · 2016-11-15T17:07:11Z

ambient-light/AmbientLightSensor_onerror-manual.https.html

+});
+
+async_test(function(t) {
+  sensor.onactivate = t.step_func_done(function(event) {


Can't you just rewrite this as:

sensor.onactivate = t.step_func_done(assert_unreached);

Not necessary, but clearer, imho.

Of course, updated.

- Move reading stuff into onchange event - reading would hold on to the latest reading when the tab is inactive

- Remove onstatechange event tests - Update "active" state to "activated" - Add test for onactivate event

dontcallmedom · 2016-12-01T15:20:05Z

Merging as discussed on DAS call

wpt-pr-bot added ambient-light wg-dap labels Nov 14, 2016

tobie reviewed Nov 14, 2016

View reviewed changes

tobie reviewed Nov 15, 2016

View reviewed changes

Fix trailing whitespace

d69335d

Honry changed the title ~~AmbientLightSensor: Update tests to be compliance with latest WD~~ AmbientLightSensor: Update tests to be compliance with latest ED Nov 16, 2016

Honry added 4 commits November 16, 2016 15:04

Address @tobie's comments

867043a

- Move reading stuff into onchange event - reading would hold on to the latest reading when the tab is inactive

Update tests to latest ED

5245912

- Remove onstatechange event tests - Update "active" state to "activated" - Add test for onactivate event

Make tests clear

f96b506

Remove duplicated tests

84ad754

dontcallmedom merged commit 4c038b7 into web-platform-tests:master Dec 1, 2016

Fix compatibility issue between idlharness.js and chromium

8bba338

Honry deleted the ALS/refresh-tests branch December 19, 2016 08:01

AmbientLightSensor: Update tests to be compliance with latest ED #4203

AmbientLightSensor: Update tests to be compliance with latest ED #4203

Conversation

Honry commented Nov 14, 2016 • edited Loading

wpt-pr-bot commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobie Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Honry commented Nov 15, 2016 • edited Loading

tobie commented Nov 15, 2016

Honry commented Nov 15, 2016

Honry commented Nov 15, 2016

tobie commented Nov 15, 2016

Honry commented Nov 15, 2016

Honry commented Nov 15, 2016

tobie Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dontcallmedom commented Dec 1, 2016

Honry commented Nov 14, 2016 •

edited

Loading

tobie Nov 14, 2016 •

edited

Loading

Honry commented Nov 15, 2016 •

edited

Loading

tobie Nov 15, 2016 •

edited

Loading