Mock device support by dati91 · Pull Request #17 · servo/devices

dati91 · 2016-10-03T13:31:07Z

Adding mock device support for bluetooth.

Replaced the structs with enums, so we can implement the Bluetooth Test API with minimal code change.
For building the mock adapter, use --feature bluetooth-test.

This change is

dati91 · 2016-10-03T13:31:19Z

r? @jdm

larsbergstrom · 2016-10-09T11:12:05Z

Reviewed 3 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/bluetooth.rs, line 58 at r2 (raw file):

#[cfg(feature = "bluetooth-test")]
const NOT_SUPPORTED_ERROR: &'static str = "Error! Not supported function!";

How will this show up for developers? I see that the message is wrapped in an Err and returned in several places, but how will they know which function is not supported? If it won't be obvious from a call stack, it would be helpful to format the name of the function into the returned error message.

src/bluetooth.rs, line 70 at r2 (raw file):

    #[cfg(feature = "bluetooth-test")]
    Mock(Arc<FakeBluetoothAdapter>),
}

Given the complexity of the configuration space at this point, it might be worth a comment to describe what's going on here and what valid configuration sets are (e.g., which can co-exist like test and everything else). It's a nice set of macros, etc. to get the compiler to help you make sure you didn't miss one or have extra "can't happen" panic! paths in the code, but I do worry about the build-time experience a little.

Comments from Reviewable

larsbergstrom · 2016-10-09T11:12:26Z

Just a couple of small nits and otherwise good to go!

dati91 · 2016-10-09T13:59:51Z

src/bluetooth.rs, line 58 at r2 (raw file):

Previously, larsbergstrom (Lars Bergstrom) wrote…

How will this show up for developers? I see that the message is wrapped in an Err and returned in several places, but how will they know which function is not supported? If it won't be obvious from a call stack, it would be helpful to format the name of the function into the returned error message.

This error will be returned if you want to use a mock function (e.g. set name) and your adapter/device/etc is not mock.

Comments from Reviewable

zakorgy · 2016-10-11T12:44:50Z

I wrote some description about the crate dependencies, features and usage in the README.md, with some example code in the first commit.
In the meantime I found, that the create_device, create_service, etc. methods should be public only when creating dummy structures, or else we can create "real" structures calling these functions, which is not the proper way doing that, the second commit fixes this.
Also removed lines longer than 120 character.
Are these changes sufficient ?

larsbergstrom · 2016-10-12T21:17:59Z

@bors-servo r+

bors-servo · 2016-10-12T21:18:01Z

📌 Commit 434cb36 has been approved by larsbergstrom

bors-servo · 2016-10-12T21:18:04Z

⚡ Test exempted - status

Mock device support Adding mock device support for bluetooth. Replaced the structs with enums, so we can implement the Bluetooth Test API with minimal code change. For building the mock adapter, use `--feature bluetooth-test`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/devices/17)

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)

…st-api-impl); r=jdm  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  Source-Repo: https://github.com/servo/servo Source-Revision: dae007fd1634bcf1545e67abaa7746fa95f10e94

…st-api-impl); r=jdm  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  Source-Repo: https://github.com/servo/servo Source-Revision: dae007fd1634bcf1545e67abaa7746fa95f10e94 UltraBlame original commit: 44661663a5b516d0df0b5743091b1ed1ff72bb25

zakorgy and others added 3 commits October 3, 2016 19:22

Mock device support

f84973e

Update travis to use test feature

6cfdf95

Update appveyor to use test feature

a1a1265

dati91 force-pushed the test-api branch from 52ef00c to a1a1265 Compare October 3, 2016 17:22

zakorgy mentioned this pull request Oct 6, 2016

WebBluetooth Test API and tests servo/servo#13612

Merged

3 tasks

larsbergstrom self-assigned this Oct 8, 2016

zakorgy added 3 commits October 11, 2016 14:30

Remove lines longer than 120 character

90c5b46

Make the existing create methods private, add create_mock methods.

a94f214

Add Bluetooth description to README.md

434cb36

bors-servo merged commit 434cb36 into servo:master Oct 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mock device support#17

Mock device support#17
bors-servo merged 6 commits intoservo:masterfrom
szeged:test-api

dati91 commented Oct 3, 2016 •

edited by larsbergstrom

Loading

Uh oh!

dati91 commented Oct 3, 2016

Uh oh!

larsbergstrom commented Oct 9, 2016

Uh oh!

larsbergstrom commented Oct 9, 2016

Uh oh!

dati91 commented Oct 9, 2016

Uh oh!

zakorgy commented Oct 11, 2016 •

edited

Loading

Uh oh!

larsbergstrom commented Oct 12, 2016

Uh oh!

bors-servo commented Oct 12, 2016

Uh oh!

bors-servo commented Oct 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Uh oh!

dati91 commented Oct 3, 2016

Uh oh!

larsbergstrom commented Oct 9, 2016

Uh oh!

larsbergstrom commented Oct 9, 2016

Uh oh!

dati91 commented Oct 9, 2016

Uh oh!

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

Uh oh!

Uh oh!

larsbergstrom commented Oct 12, 2016

Uh oh!

bors-servo commented Oct 12, 2016

Uh oh!

bors-servo commented Oct 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dati91 commented Oct 3, 2016 •

edited by larsbergstrom

Loading

zakorgy commented Oct 11, 2016 •

edited

Loading