Skip to content

Commit

Permalink
sensors: Make frequency hint test wait for another slow sensor "tick".
Browse files Browse the repository at this point in the history
This test was flaky, especially on the Mac bots, where timers can often fire
with varying precision.

SensorProxy uses an ObserverSet to store Sensor instances that need to be
notified of a reading, so in some cases we could end up with the following
sequence of notifications in C++ and JavaScript:
1. OnSensorReadingChanged() is called for the fast sensor (running at either
   10Hz or 60Hz).
2. A "reading" event is delivered to the fast sensor's event handler in
   script.
3. OnSensorReadingChanged() is called for the slow sensor (running at 1/4th
   of the fast sensor's frequency).
4. A "reading" event is delivered to the slow sensor's event handler in
   script.
5. OnSensorReadingChanged() is called for the slow sensor again, but it is
   too early so it posts a delayed task to call NotifyReading().
6. OnSensorReadingChanged() is called for the fast sensor.
7. A "reading" event is delivered to the fast sensor's event handler in
   script.
8. The delayed task is executed, and a "reading" event is delivered to the
   slow sensor's event handler in script.

At this point, both |slowSensorNotifierCounter| and
|fastSensorNotifierCounter| are set to 2, and the assertion in the test
fails.

Fix it by waiting for another "reading" event to be delivered to the slow
sensor. Given the frequencies we choose, at this point a working
implementation's |fastSensorNotifierCounter| will definitely be higher than
2.

The downside is that we slow down the test by the value of another sensor
period -- for regular sensors, that is around 0.016s, while for slower
sensors such as AmbientLightSensor and Magnetometer (capped at 10Hz) this is
a 0.4s slowdown.

Bug: 731018
Change-Id: I249ab88eccf316888177a93614c2955b516bc835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2204023
Auto-Submit: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770328}
  • Loading branch information
rakuco authored and chromium-wpt-export-bot committed May 19, 2020
1 parent f1d5495 commit bede107
Showing 1 changed file with 10 additions and 4 deletions.
14 changes: 10 additions & 4 deletions generic-sensor/generic-sensor-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ function runGenericSensorTests(sensorName,
const mockSensor = await sensorProvider.getCreatedSensor(sensorName);
await mockSensor.setSensorReading(readings);

const fastCounter = await new Promise((resolve, reject) => {
return new Promise((resolve, reject) => {
let fastSensorNotifiedCounter = 0;
let slowSensorNotifiedCounter = 0;

Expand All @@ -413,10 +413,17 @@ function runGenericSensorTests(sensorName,
const slowSensor = new sensorType({frequency: slowFrequency});
slowSensor.onreading = () => {
// Skip the initial notification that always comes immediately.
if (slowSensorNotifiedCounter === 1) {
if (slowSensorNotifiedCounter === 2) {
fastSensor.stop();
slowSensor.stop();
resolve(fastSensorNotifiedCounter);

try {
assert_greater_than(fastSensorNotifiedCounter, 3,
"Fast sensor overtakes the slow one");
resolve();
} catch (e) {
reject(e);
}
}
slowSensorNotifiedCounter++;
}
Expand All @@ -427,7 +434,6 @@ function runGenericSensorTests(sensorName,
}
fastSensor.onerror = reject;
});
assert_greater_than(fastCounter, 2, "Fast sensor overtakes the slow one");
}, `${sensorName}: frequency hint works.`);

// Re-enable after https://github.com/w3c/sensors/issues/361 is fixed.
Expand Down

0 comments on commit bede107

Please sign in to comment.