WebBluetooth Test API and tests by zakorgy · Pull Request #13612 · servo/servo

zakorgy · 2016-10-06T08:03:19Z

This patch depends on the devices mock device support PR.
After it lands, the Cargo files can be updated.

Adjust to the changes in devices mock device support PR.
WebBluetooth Test API implementation. Based on : https://webbluetoothcg.github.io/web-bluetooth/tests.html
Wpt tests for the already landed WebBluetooth functions.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes

This change is

highfive · 2016-10-06T08:03:23Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/lib.rs, components/script/dom/mod.rs, components/script/dom/webidls/Window.webidl, components/net/bluetooth_thread.rs, components/script/dom/window.rs, components/net/bluetooth_test.rs, components/net/Cargo.toml, components/net_traits/bluetooth_thread.rs, components/net_traits/bluetooth_thread.rs, components/script/dom/webidls/TestRunner.webidl, components/script/dom/testrunner.rs

highfive · 2016-10-06T08:03:24Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

Manishearth · 2016-10-06T08:30:45Z

components/net/bluetooth_test.rs

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+


tests/unit/net instead?

Manishearth · 2016-10-06T08:31:57Z

Not sure if I'm the one to review this, I don't know anything about bluetooth

dati91 · 2016-10-06T08:32:43Z

r? @jdm

bors-servo · 2016-10-07T14:10:20Z

☔ The latest upstream changes (presumably #13596) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2016-10-14T14:55:00Z

I'm sorry that I haven't reviewed this yet. I've been swamped with reviews in multiple projects recently; this is in my queue.

jdm

So many tests for previous untestable code! This is really great! However, there is a lot of duplication in the test files, which makes them harder to read and maintain in the future. I propose we create helper functions for each class of test (out of range, disconnect after, disconnect during, etc.) that takes a callback, and move as much of the common code into the helpers as possible. I'm even tempted to suggest that the tests be organized in files that test those cases for all of the different methods, rather than having separate files that duplicate tests in each subdirectory. Does that make sense?

jdm · 2016-10-14T19:34:37Z

components/net/bluetooth_thread.rs

            Some(a) => a,
            None => return drop(sender.send(Err(BluetoothError::Type(ADAPTER_ERROR.to_string())))),
        };
+        let device = {


Why don't we make a device_from_service_id helper that returns Option<BluetoothDevice> and use it here and below?

jdm · 2016-10-14T19:40:52Z

components/script/dom/webidls/TestRunner.webidl

+
+// callback BluetoothManualChooserEventsCallback = void(sequence<DOMString> events);
+
+[Pref="dom.bluetooth.enabled", Exposed=(Window,Worker)]


Is WebBluetooth supposed to be exposed in workers? Let's use the dom.bluetooth.testing.enabled preference here instead, too.

jdm · 2016-10-14T19:42:36Z

components/script/dom/webidls/Window.webidl

+
+// https://webbluetoothcg.github.io/web-bluetooth/tests#test-interfaces
+partial interface Window {
+   readonly attribute TestRunner testRunner;


https://webbluetoothcg.github.io/web-bluetooth/tests#security-and-privacy suggests that this should have a Pref annotation for a separate preference like dom.bluetooth.testing.enabled.

jdm · 2016-10-14T19:44:25Z

components/script/dom/testrunner.rs

+        let (sender, receiver) = ipc::channel().unwrap();
+        self.get_bluetooth_thread().send(BluetoothMethodMsg::Test(String::from(dataSetName), sender)).unwrap();
+
+        let result = receiver.recv().unwrap();


We can replace this line and the match with receiver.recv().unwrap().into().

jdm · 2016-10-14T19:45:05Z

components/script/dom/testrunner.rs

+    }
+
+    fn get_bluetooth_thread(&self) -> IpcSender<BluetoothMethodMsg> {
+        self.global().as_window().bluetooth_thread()


This code looks like we don't expect bluetooth to work in Workers (as mentioned in a later comment).

jdm · 2016-10-14T19:59:49Z

components/net/bluetooth_test.rs

+}
+
+pub fn test(manager: &mut BluetoothManager, data_set_name: String, sender: &IpcSender<BluetoothResult<()>>) {
+    match manager.get_or_create_adapter().as_ref() {


let adapter = match manager.get_or_create_adapter().as_ref() { Some(adapter) => adapter, None => return drop(sender.send(...)), };

jdm · 2016-10-14T20:00:58Z

components/net/bluetooth_test.rs

+                GLUCOSE_HEART_RATE_ADAPTER => {
+                    set_adapter(adapter, GLUCOSE_HEART_RATE_ADAPTER.to_owned(), sender);
+
+                    // Glucose Device


I don't think the comments with the device names in this code add much - we already call the local variables names like glucose_device, and we're passing arguments like GLUCOSE_DEVICE_NAME :)

jdm · 2016-10-14T20:10:55Z

tests/wpt/mozilla/tests/mozilla/bluetooth/connect/connection-succeeds.html

+    })
+    .then(device => device.gatt.connect())
+    .then(gattServer => assert_true(gattServer.connected))
+    .catch(error => assert_unreached('Promise should have resolved. ' + error));


I believe promise_test automatically fails tests for unexpected rejections, so this can be removed from all the tests.

jdm · 2016-10-14T20:18:40Z

tests/wpt/mozilla/tests/mozilla/bluetooth/getCharacteristic/disconnect-called-during.html

+        });
+    })
+    .catch(error => assert_unreached('Promise should have resolved. ' + error));
+}, 'disconnect() called before getCharacteristic. Reject with NetworkError.');


s/before/during/

jdm · 2016-10-14T20:34:00Z

tests/wpt/mozilla/tests/mozilla/bluetooth/getCharacteristics/correct-characteristics.html

+    .then(service => service.getCharacteristics(body_sensor_location.name))
+    .then(characteristics => {
+        for (let i = 1; i < characteristics.length; i++) {
+            characteristics[i].readValue()


We probably need to store each promise in a vector, and return Promise.all(promises) from this function.

jdm · 2016-10-14T20:40:34Z

Note - I stopped reviewing the third commit about halfway through the tests once I realized how much duplication I was seeing. I believe it will be easier to review the tests after they are reorganized according to my suggestion.

zakorgy · 2016-10-18T12:12:11Z

@jdm we have fixed the first and second commit as you mentioned, this also indicated some changes in the third as well. We have not modified the tests yet, because the chrome folks have almost the same implementation with similar structure, and we wanted to stay consistent with their implementation.
On the other hand I agree, that organizing the tests as you requested, will make it more maintainable. If you really insist to the changes, we will do it in a next commit.

dati91 · 2016-10-20T11:35:47Z

So the main reason behind wanting to be consistent with chrome is that we think these tests should be platform/browser independent. I think in the near (or at least not "that" far) future these tests should be in a shared repository where all implementing browser can find and use them. That beeing said I could totally see (here or later there) your recommendation where we move out all the duplicated code for easier and cleaner use. But AFAIK there will be a new, more dynamic testing api which will replace the current one (and will modify these tests also). This is the main reason why the current Testing API document is (almost) deprecated.

jdm · 2016-10-20T12:39:01Z

Good to know.

bors-servo · 2016-11-04T10:05:20Z

⌛ Testing commit b468990 with merge a334747...

WebBluetooth Test API and tests  This patch depends on the [devices mock device support PR](servo/devices#17). After it lands, the Cargo files can be updated. 1. Adjust to the changes in [devices mock device support PR](servo/devices#17). 2. WebBluetooth Test API implementation. Based on : https://webbluetoothcg.github.io/web-bluetooth/tests.html 3. Wpt tests for the already landed WebBluetooth functions.  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13612)

bors-servo · 2016-11-04T10:26:26Z

💔 Test failed - linux-dev

larsbergstrom · 2016-11-04T10:31:42Z

diff --git a/ports/cef/Cargo.lock b/ports/cef/Cargo.lock
index c7679cc..a165ac6 100644
--- a/ports/cef/Cargo.lock
+++ b/ports/cef/Cargo.lock
@@ -170,6 +170,7 @@ dependencies = [
  "rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)",
  "tinyfiledialogs 0.1.0 (git+https://github.com/jdm/tinyfiledialogs)",
  "util 0.0.1",
+ "uuid 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)",
 ]

You may need to ./mach cargo-update -p uuid

jdm · 2016-11-04T12:10:09Z

@bors-servo: r+

bors-servo · 2016-11-04T12:10:10Z

📌 Commit ada0256 has been approved by jdm

bors-servo · 2016-11-04T12:44:43Z

⌛ Testing commit ada0256 with merge dae007f...

WebBluetooth Test API and tests  This patch depends on the [devices mock device support PR](servo/devices#17). After it lands, the Cargo files can be updated. 1. Adjust to the changes in [devices mock device support PR](servo/devices#17). 2. WebBluetooth Test API implementation. Based on : https://webbluetoothcg.github.io/web-bluetooth/tests.html 3. Wpt tests for the already landed WebBluetooth functions.  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13612)

bors-servo · 2016-11-04T13:44:46Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

Webbluetooth Async behaviour  Note: depends on #13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13909)

WebBluetooth fixes for the wpt tests  Note: depends on #13612 WebBluetooth fixes for the failing wpt tests, excluding the `disconnect-during` tests cases, due to the lack of the event handling in the current implementation. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13918)

Webbluetooth Async behaviour  Note: depends on #13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13909)

…se-queue); r=jdm  Note: depends on servo/servo#13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes  Source-Repo: https://github.com/servo/servo Source-Revision: 1153ca9841f458daf373471f3c65295abd872271

…se-queue); r=jdm  Note: depends on servo/servo#13612 The current WBT communication is synchronous. With this, it should work properly (except the disconnect function, which will need some more work, because the current implementation differ from the spec).  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  - [X] There are tests for these changes  Source-Repo: https://github.com/servo/servo Source-Revision: 1153ca9841f458daf373471f3c65295abd872271 UltraBlame original commit: 30cc9f4ec923a5bb7fc43857675aa2c6bf572e47

highfive assigned Manishearth Oct 6, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 6, 2016

zakorgy force-pushed the test-api-impl branch from f723101 to f362396 Compare October 6, 2016 08:07

Manishearth reviewed Oct 6, 2016

View reviewed changes

highfive assigned jdm and unassigned Manishearth Oct 6, 2016

zakorgy force-pushed the test-api-impl branch from f362396 to b32fd67 Compare October 6, 2016 10:56

highfive added the S-needs-rebase There are merge conflict errors. label Oct 7, 2016

zakorgy force-pushed the test-api-impl branch 2 times, most recently from d866053 to bbbe8a8 Compare October 14, 2016 14:22

jdm removed the S-needs-rebase There are merge conflict errors. label Oct 14, 2016

jdm requested changes Oct 14, 2016

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 14, 2016

zakorgy changed the title ~~[WIP] WebBluetooth Test API and tests~~ WebBluetooth Test API and tests Oct 18, 2016

zakorgy mentioned this pull request Oct 18, 2016

Test api fixes szeged/servo#72

Merged

2 tasks

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 18, 2016

zakorgy force-pushed the test-api-impl branch from 305b880 to f826423 Compare October 20, 2016 11:41

zakorgy force-pushed the test-api-impl branch from f826423 to 7ee04bc Compare October 24, 2016 11:21

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-rebase There are merge conflict errors. labels Nov 4, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 4, 2016

zakorgy added 2 commits November 4, 2016 12:36

WebBluetooth Test API

e15d54e

WebBluetooth WPT tests

ada0256

zakorgy force-pushed the test-api-impl branch from b468990 to ada0256 Compare November 4, 2016 11:37

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 4, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 4, 2016

bors-servo merged commit ada0256 into servo:master Nov 4, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 4, 2016

dati91 mentioned this pull request Jan 17, 2017

Add Bluetooth idlharness test #15080

Merged

5 tasks


		// callback BluetoothManualChooserEventsCallback = void(sequence<DOMString> events);

		[Pref="dom.bluetooth.enabled", Exposed=(Window,Worker)]

Uh oh!

Conversation

zakorgy commented Oct 6, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Oct 6, 2016

Uh oh!

highfive commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Oct 6, 2016

Uh oh!

dati91 commented Oct 6, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

jdm commented Oct 14, 2016

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdm commented Oct 14, 2016

Uh oh!

zakorgy commented Oct 18, 2016

Uh oh!

dati91 commented Oct 20, 2016

Uh oh!

jdm commented Oct 20, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

larsbergstrom commented Nov 4, 2016

Uh oh!

jdm commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zakorgy commented Oct 6, 2016 •

edited by larsbergstrom

Loading