Modify ProximitySensor tests #19214
Modify ProximitySensor tests #19214
Conversation
xiuqijix
commented
Sep 23, 2019
xiuqijix
commented
- Support mock sensor
- Add verifyProSensorReading function
| @@ -92,6 +93,11 @@ function verifyGeoSensorReading(pattern, {latitude, longitude, altitude, | |||
| accuracy, altitudeAccuracy, heading, speed], timestamp, isNull); | |||
| } | |||
|
|
|||
| function verifyProSensorReading(pattern, {distance, max, near, timestamp}, | |||
There was a problem hiding this comment.
Abbreviating "proximity" to "pro" is confusing. I'd rather call this verifyProximitySensorReading().
| @@ -92,6 +93,11 @@ function verifyGeoSensorReading(pattern, {latitude, longitude, altitude, | |||
| accuracy, altitudeAccuracy, heading, speed], timestamp, isNull); | |||
| } | |||
|
|
|||
| function verifyProSensorReading(pattern, {distance, max, near, timestamp}, | |||
| isNull) { | |||
There was a problem hiding this comment.
There's no need for this line break here.
proximity/ProximitySensor.https.html
Outdated
|
|
||
| const kReadings = { | ||
| readings: [ | ||
| [1.12345, 2.12345, true] |
There was a problem hiding this comment.
This is supposed to be a raw reading value, so a boolean does not make much sense here. For proper testing, you should provide a number that the sensor code will convert into a boolean.
There was a problem hiding this comment.
ProximitySensor is not supported in mojo yet, so I think we could optimize this in future. If you agree on this, I will add a TODO note. :)
There was a problem hiding this comment.
Do you mean it's not handled in resources/chromium/generic_sensor_mocks.js? If that's the case, it should be pretty easy to add the required bits there in this PR to make it complete. If not, can you clarify what you're referring to, and how this has been tested so far?
| @@ -62,6 +62,7 @@ function sensor_test(func, name, properties) { | |||
|
|
|||
| function verifySensorReading(pattern, values, timestamp, isNull) { | |||
| function round(val) { | |||
| if (typeof(val) === "boolean") return val; | |||
There was a problem hiding this comment.
I don't like this special handling here, but I understand that the proximity sensor is the first case where we are testing a non-numeric property as well. I'd rather do something that just returns val if the float conversion below fails.
xiuqijix
force-pushed
the
sensor_proximity
branch
from
56d8099 to
b0433f0
Compare
- Support mock sensor - Add verifyProSensorReading function
| @@ -150,7 +150,8 @@ var GenericSensorTest = (() => { | |||
| ['RelativeOrientationSensor', | |||
| device.mojom.SensorType.RELATIVE_ORIENTATION_QUATERNION], | |||
| ['RelativeOrientationEulerAngles', | |||
| device.mojom.SensorType.RELATIVE_ORIENTATION_EULER_ANGLES] | |||
| device.mojom.SensorType.RELATIVE_ORIENTATION_EULER_ANGLES], | |||
| ['proximity', device.mojom.SensorType.PROXIMITY] | |||
There was a problem hiding this comment.
This name does not match the one you are using in ProximitySensor.https.html ("ProximitySensor").
| @@ -62,7 +62,8 @@ function sensor_test(func, name, properties) { | |||
|
|
|||
| function verifySensorReading(pattern, values, timestamp, isNull) { | |||
| function round(val) { | |||
| return Number.parseFloat(val).toPrecision(6); | |||
| const res = Number.parseFloat(val).toPrecision(6); | |||
There was a problem hiding this comment.
Can you add a comment explaining why we do this (something like "If |val| cannot be converted to a float, we return the original value. This can happen when a value in |pattern| is not a number.")?
proximity/ProximitySensor.https.html
Outdated
| [1.12345, 2.12345, 1] | ||
| ], | ||
| expectedReadings: [ | ||
| [1.12345, 2.12345, 1] |
There was a problem hiding this comment.
This one should be true rather than 1. readings are raw readings passed by the mocked platform, expectedReadings are the sanitized values obtained from a sensor object.
|
Taskcluster failing the infrastructure/ tests is likely because of #19362. There's a workaround in place so I'll close and reopen to retrigger all CI. |
