Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2046283003:
Add unit test for ArcBluetoothBridge and TypeConverter (Closed)
|
Created:
4 years, 6 months ago by puthik_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bt Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit test for ArcBluetoothBridge and TypeConverter
- Refactor ArcBluetoothBridge to make it easier for testing.
- Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC
- Add tests for BluetoothTypeConverter
- Add tests for ArcBluetoothBridge
BUG=b:28250518
TEST=Build / Run tests
Committed: https://crrev.com/7851146bed75ddf054e1b755c337018753951a3a
Cr-Commit-Position: refs/heads/master@{#409945}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : WIP: Add fake BT instance #Patch Set 4 : Add DeviceFound test #Patch Set 5 : Add AdvertiseData test #Patch Set 6 : rebase to latest bt #Patch Set 7 : rebase #
Total comments: 7
Patch Set 8 : Use fake dbus client instead of mock Bluetooth type #
Total comments: 3
Patch Set 9 : Refactor to use instance holder #Patch Set 10 : Use std::unique_ptr for fake arcbridge #
Total comments: 8
Patch Set 11 : Fix unique pointer #
Total comments: 2
Patch Set 12 : Fix #11 nit #
Total comments: 2
Patch Set 13 : Fix rkc@ nit / buildbot error #
Total comments: 4
Patch Set 14 : Address palmer's nit #Patch Set 15 : Range check in type_converters #
Messages
Total messages: 62 (27 generated)
puthik@chromium.org changed reviewers: + rkc@chromium.org
Any update on this?
Description was changed from ========== WIP: Add unit test for ArcBluetoothBridge BUG=b:28250518 ========== to ========== Add unit test for ArcBluetoothBridge and TypeConverter - Refactor ArcBluetoothBridge to make it easier for testing. - Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC - Add tests for BluetoothTypeConverter - Add tests for ArcBluetoothBridge BUG=b:28250518 TEST=Build / Run tests ==========
Will add lhchavez@ for owner approval once got l-g-t-m from you
I think these tests are probably enough for the easy to break thing.
puthik@chromium.org changed reviewers: + lhchavez@chromium.org
Opal, I apologize for taking so long for this review, but I wanted to create a functional CL that shows the changes I would like made. So we really want to have these tests go all the way to the Bluetooth -> BlueZ -> DBus -> DBusFakes layer. That will test all the code in between and expose any bugs in our usage of BlueZ or below. Using just mocks doesn't test very much since we are tailoring our tests to react a certain way. For example, if we use a MockDevice, we really aren't testing the actual discovery of devices and all the events that get called that actually add a device to the adapter. Additionally, we really don't want to make any changes to the actual production code for tests. Making changes to fake implementations, or making changes to 'allow' fake implementations is fine. Usually we try to stay away from SetXXXForTest methods. Please see: https://codereview.chromium.org/2078263002/ Currently two tests fail in the CL but I believe the first failure is actually a bug. You might need to change more of FakeBluetoothDeviceClient to get the third test to work. I would suggest working off the CL I uploaded and getting the first couple of tests to work, then adding any changes needed for FakeBluetoothDeviceClient + the tests depending on it on another CL. The tests are not blocking, so taking some time on these to get all the infrastructure in place is fine. It will help everyone writing ARC++ Bluetooth tests after you! :) https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.cc (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:75: ArcBluetoothBridge::ArcBluetoothBridge(ArcBridgeService* bridge_service, We don't need this constructor. We never use the test_flag variable anywhere anyway. https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.cc:106: bluetooth_instance_ = base::WrapUnique(bluetooth_instance); None of these changes are needed. This file, in fact, should really not be touched at all for the test. https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge.h (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge.h:256: void SetAdapterForTest(device::BluetoothAdapter* adapter) { None of these are required. Adding "SetXXXForTesting" is not a preferable way to test classes if we can avoid it. It is much cleaner to have fake classes create their own fakes as needed. See the example CL in the review message. https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:27: const char kAddressStr[] = "1A:2B:3C:4D:5E:6F"; constexpr everywhere. https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:18: const std::string kAddressStr = "1A:2B:3C:4D:5E:6F"; constexpr everywhere https://codereview.chromium.org/2046283003/diff/120001/components/arc/test/fa... File components/arc/test/fake_bluetooth_instance.cc (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/test/fa... components/arc/test/fake_bluetooth_instance.cc:38: void FakeBluetoothInstance::OnDeviceFound( What does the actual OnDeviceFound method do? This might be exposing a bug. This method apparently ends up getting called twice; once when the DeviceAdded observer method is called, and then again when we do SendCachedDevices.
I rewrite the test to use fake dbus layer instead. The bluez::FakeBluetoothDeviceClient::CreateDevice(kLowEnergyPath) will conveniently create a fake heart rate monitor. I make the test to check the ArcBridge method against that device. https://codereview.chromium.org/2046283003/diff/120001/components/arc/test/fa... File components/arc/test/fake_bluetooth_instance.cc (right): https://codereview.chromium.org/2046283003/diff/120001/components/arc/test/fa... components/arc/test/fake_bluetooth_instance.cc:38: void FakeBluetoothInstance::OnDeviceFound( On 2016/06/19 18:45:43, rkc wrote: > What does the actual OnDeviceFound method do? This might be exposing a bug. This > method apparently ends up getting called twice; once when the DeviceAdded > observer method is called, and then again when we do SendCachedDevices. It's will send the list of known device to Android. Use case 1: User does not run Bluetooth scan before log in - Android asked Chrome to scan - Chrome found new device. - DeviceAdded() get called - Chrome send that device data to Android via OndeviceFound Use case 2: User already scans device before log in - Android asked Chrome to scan - SendCachedDevices() get called - Chrome send cached devices data without need to scanning again. - Additional device will send via DeviceAdded()
Excellent! Thanks for doing this! lgtm
lhchavez@ PTAL
https://codereview.chromium.org/2046283003/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2046283003/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_service.h:195: virtual mojom::BluetoothInstance* bluetooth_instance(); ouch. you can't have name this "bluetooth_instance" if you don't just inline the method, which means that you'll need to change it to GetBluetoothInstance(). But that will make it inconsistent with all the other methods. Let me see if there's a way around it.
Had an offline discussion. I'm going to migrate _all_ methods of ArcBridgeService to be virtual and use the GetXXXInstance() naming convention before this can land.
Luis: Ping :) Now that branch is done, we should look at landing this so we can continue to write more tests based off the changes brought in with this CL.
On 2016/07/01 22:37:10, rkc wrote: > Luis: Ping :) > Now that branch is done, we should look at landing this so we can continue to > write more tests based off the changes brought in with this CL. err, it's going to be way more work than I expected originally. Can this be modelled like the ArcAppTest in the meantime? https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_t...
https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:87: delete fake_arc_bridge_service_; Try to avoid this raw pointers as much as possible. Use std::unique_ptr like the other tests.
On 2016/07/06 22:34:27, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): > > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:87: delete > fake_arc_bridge_service_; > Try to avoid this raw pointers as much as possible. Use std::unique_ptr like the > other tests. Update on the refactoring: https://codereview.chromium.org/2133503002/ I expect this to land sometime next week
On 2016/07/08 18:05:03, Luis Héctor Chávez wrote: > On 2016/07/06 22:34:27, Luis Héctor Chávez wrote: > > > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > > File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): > > > > > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > > components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:87: delete > > fake_arc_bridge_service_; > > Try to avoid this raw pointers as much as possible. Use std::unique_ptr like > the > > other tests. > > Update on the refactoring: https://codereview.chromium.org/2133503002/ > > I expect this to land sometime next week it has landed. the changes to the bridge are now unneeded since you can set both the raw mojom::BluetoothInstance* and its version manually.
On 2016/07/13 15:20:32, Luis Héctor Chávez wrote: > On 2016/07/08 18:05:03, Luis Héctor Chávez wrote: > > On 2016/07/06 22:34:27, Luis Héctor Chávez wrote: > > > > > > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > > > File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... > > > components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:87: delete > > > fake_arc_bridge_service_; > > > Try to avoid this raw pointers as much as possible. Use std::unique_ptr like > > the > > > other tests. > > > > Update on the refactoring: https://codereview.chromium.org/2133503002/ > > > > I expect this to land sometime next week > > it has landed. the changes to the bridge are now unneeded since you can set both > the raw mojom::BluetoothInstance* and its version manually. welp, it got rolled back. you should still be able to model the test using the same technique since the interface did land and it stuck.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Will add more test in another CLs. I don't want it to be too big. https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/140001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:87: delete fake_arc_bridge_service_; On 2016/07/06 22:34:26, Luis Héctor Chávez wrote: > Try to avoid this raw pointers as much as possible. Use std::unique_ptr like the > other tests. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:61: base::WrapUnique(fake_bluetooth_device_client_)); This pattern looks very bad (you are keeping a reference to a unique_ptr that you already transferred ownership of). Is it possible to write it like: dbus_setter->SetXxx(base::MakeUnique<FakeXxx>(...)); fake_xxx_ = static_cast<FakeXxx>(xxx->GetXxx()); ? Otherwise, please add a comment about this pattern. It's ugly, but it's limited to tests :( https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:96: EXPECT_EQ((size_t)0, fake_bluetooth_instance_->device_found_data().size()); Try to avoid C-style casts (since they are forbidden by the style guide). Does 0u help avoid the compilation error? Another question: does `git cl lint` complain about this?
https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:61: base::WrapUnique(fake_bluetooth_device_client_)); On 2016/07/29 23:09:09, Luis Héctor Chávez wrote: > This pattern looks very bad (you are keeping a reference to a unique_ptr that > you already transferred ownership of). > > Is it possible to write it like: > > dbus_setter->SetXxx(base::MakeUnique<FakeXxx>(...)); > fake_xxx_ = static_cast<FakeXxx>(xxx->GetXxx()); > > ? > > Otherwise, please add a comment about this pattern. It's ugly, but it's limited > to tests :( The problem with this pattern is holding on to the raw pointer of a unique_ptr owned by another class. Whether we hold on to the raw pointer from before we passed ownership or hold on to it after getting it back from a getter doesn't ameliorate that issue at all. The correct solution would be to not hold on to the raw pointer and always getting it from the getter. Though doing so in a limited controlled environment like a unittest has generally been acceptable practice. https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:98: EXPECT_EQ((size_t)1, fake_bluetooth_instance_->device_found_data().size()); As Luis said, all these casts need to go. When comparing variables to literal, please use literal suffixes (like u, or l, etc) to match the types, not a cast.
https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:61: base::WrapUnique(fake_bluetooth_device_client_)); On 2016/07/30 22:40:55, rkc wrote: > On 2016/07/29 23:09:09, Luis Héctor Chávez wrote: > > This pattern looks very bad (you are keeping a reference to a unique_ptr that > > you already transferred ownership of). > > > > Is it possible to write it like: > > > > dbus_setter->SetXxx(base::MakeUnique<FakeXxx>(...)); > > fake_xxx_ = static_cast<FakeXxx>(xxx->GetXxx()); > > > > ? > > > > Otherwise, please add a comment about this pattern. It's ugly, but it's > limited > > to tests :( > > The problem with this pattern is holding on to the raw pointer of a unique_ptr > owned by another class. Whether we hold on to the raw pointer from before we > passed ownership or hold on to it after getting it back from a getter doesn't > ameliorate that issue at all. > > The correct solution would be to not hold on to the raw pointer and always > getting it from the getter. Though doing so in a limited controlled environment > like a unittest has generally been acceptable practice. I changed it to use the get the FakeBTClient from DBus when we want to use it. https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:96: EXPECT_EQ((size_t)0, fake_bluetooth_instance_->device_found_data().size()); On 2016/07/29 23:09:09, Luis Héctor Chávez wrote: > Try to avoid C-style casts (since they are forbidden by the style guide). Does > 0u help avoid the compilation error? > > Another question: does `git cl lint` complain about this? Done. Also, git cl lint does not complain about (size_t) cast. https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:98: EXPECT_EQ((size_t)1, fake_bluetooth_instance_->device_found_data().size()); On 2016/07/30 22:40:55, rkc wrote: > As Luis said, all these casts need to go. When comparing variables to literal, > please use literal suffixes (like u, or l, etc) to match the types, not a cast. Done.
lgtm https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/180001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:96: EXPECT_EQ((size_t)0, fake_bluetooth_instance_->device_found_data().size()); On 2016/08/01 18:02:48, puthik_chromium wrote: > On 2016/07/29 23:09:09, Luis Héctor Chávez wrote: > > Try to avoid C-style casts (since they are forbidden by the style guide). Does > > 0u help avoid the compilation error? > > > > Another question: does `git cl lint` complain about this? > > Done. > Also, git cl lint does not complain about (size_t) cast. Maybe it should :/ I'll ask around. https://codereview.chromium.org/2046283003/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/200001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:64: auto fake_bluetooth_device_client_ = s/fake_bluetooth_device_client_/fake_bluetooth_device_client/
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
re-lgtm % comment https://codereview.chromium.org/2046283003/diff/220001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:51: std::unique_ptr<FakeArcBridgeService> fake_arc_bridge_service_; Move to the bottom of the section, along with the other variable definitions.
https://codereview.chromium.org/2046283003/diff/200001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/200001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:64: auto fake_bluetooth_device_client_ = On 2016/08/01 18:10:58, Luis Héctor Chávez wrote: > s/fake_bluetooth_device_client_/fake_bluetooth_device_client/ Done. https://codereview.chromium.org/2046283003/diff/220001/components/arc/bluetoo... File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/220001/components/arc/bluetoo... components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:51: std::unique_ptr<FakeArcBridgeService> fake_arc_bridge_service_; On 2016/08/01 19:24:39, rkc wrote: > Move to the bottom of the section, along with the other variable definitions. Done.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, rkc@chromium.org Link to the patchset: https://codereview.chromium.org/2046283003/#ps240001 (title: "Fix rkc@ nit / buildbot error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
puthik@chromium.org changed reviewers: + palmer@chromium.org
+palmer Apparently, unit test for bluetooth type converters need security review.
Yes, type_converter files count as security-sensitive IPC files. Sorry. :) https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:18: const std::string kAddressStr = "1A:2B:3C:4D:5E:6F"; Minor nit: You could save some lines by moving these repeated declarations into the anonymous namespace. (Same with the repeated declarations in the 3rd and 4th tests.) https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:72: It might be good to have some negative tests — tests you expect to fail, because the incoming data is bogus.
https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... File components/arc/bluetooth/bluetooth_type_converters_unittest.cc (right): https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:18: const std::string kAddressStr = "1A:2B:3C:4D:5E:6F"; On 2016/08/03 21:16:44, palmer (OOO until 3 Aug) wrote: > Minor nit: You could save some lines by moving these repeated declarations into > the anonymous namespace. (Same with the repeated declarations in the 3rd and 4th > tests.) Done. https://codereview.chromium.org/2046283003/diff/240001/components/arc/bluetoo... components/arc/bluetooth/bluetooth_type_converters_unittest.cc:72: On 2016/08/03 21:16:44, palmer (OOO until 3 Aug) wrote: > It might be good to have some negative tests — tests you expect to fail, because > the incoming data is bogus. There is a high chance that it will crash if the input is bogus. So I don't think we need to have the test here.
> There is a high chance that it will crash if the input is bogus. So I don't
> think we need to have the test here.
I don't believe that there is any magic safety inside Mojo that would handle
this for you. It is not a guarantee of Mojo that it will gracefully die on a
TypeConverter error. The crash would be most likely due to out-of-bounds memory
access — the thing we strive to avoid. This TypeConverter:
87 // static
88 device::BluetoothUUID
89 TypeConverter<device::BluetoothUUID, arc::mojom::BluetoothUUIDPtr>::Convert(
90 const arc::mojom::BluetoothUUIDPtr& uuid) {
91 std::vector<uint8_t> address_bytes =
uuid->uuid.To<std::vector<uint8_t>>();
92
93 // BluetoothUUID expects the format below with the dashes inserted.
94 // xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
95 std::string uuid_str =
96 base::HexEncode(address_bytes.data(), address_bytes.size());
97 const size_t uuid_dash_pos[] = {8, 13, 18, 23};
98 for (auto pos : uuid_dash_pos)
99 uuid_str = uuid_str.insert(pos, "-");
100
101 device::BluetoothUUID result(uuid_str);
102
103 DCHECK(result.IsValid());
104 return result;
105 }
would probably crash given a short string — something a test can check for.
Also, a colleague of mine suggests that you might be better off with type
mapping than a TypeConverter.
The CQ bit was checked by puthik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/04 20:52:56, palmer (OOO thru August)) wrote:
> > There is a high chance that it will crash if the input is bogus. So I don't
> > think we need to have the test here.
>
> I don't believe that there is any magic safety inside Mojo that would handle
> this for you. It is not a guarantee of Mojo that it will gracefully die on a
> TypeConverter error. The crash would be most likely due to out-of-bounds
memory
> access — the thing we strive to avoid. This TypeConverter:
>
> 87 // static
> 88 device::BluetoothUUID
> 89 TypeConverter<device::BluetoothUUID,
arc::mojom::BluetoothUUIDPtr>::Convert(
> 90 const arc::mojom::BluetoothUUIDPtr& uuid) {
> 91 std::vector<uint8_t> address_bytes =
> uuid->uuid.To<std::vector<uint8_t>>();
> 92
> 93 // BluetoothUUID expects the format below with the dashes inserted.
> 94 // xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> 95 std::string uuid_str =
> 96 base::HexEncode(address_bytes.data(), address_bytes.size());
> 97 const size_t uuid_dash_pos[] = {8, 13, 18, 23};
> 98 for (auto pos : uuid_dash_pos)
> 99 uuid_str = uuid_str.insert(pos, "-");
> 100
> 101 device::BluetoothUUID result(uuid_str);
> 102
> 103 DCHECK(result.IsValid());
> 104 return result;
> 105 }
>
> would probably crash given a short string — something a test can check for.
>
> Also, a colleague of mine suggests that you might be better off with type
> mapping than a TypeConverter.
Done. I added a check for that. And tests for both shorter and longer length.
LGTM. Thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by puthik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2046283003/#ps280001 (title: "Range check in type_converters")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add unit test for ArcBluetoothBridge and TypeConverter - Refactor ArcBluetoothBridge to make it easier for testing. - Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC - Add tests for BluetoothTypeConverter - Add tests for ArcBluetoothBridge BUG=b:28250518 TEST=Build / Run tests ========== to ========== Add unit test for ArcBluetoothBridge and TypeConverter - Refactor ArcBluetoothBridge to make it easier for testing. - Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC - Add tests for BluetoothTypeConverter - Add tests for ArcBluetoothBridge BUG=b:28250518 TEST=Build / Run tests ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add unit test for ArcBluetoothBridge and TypeConverter - Refactor ArcBluetoothBridge to make it easier for testing. - Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC - Add tests for BluetoothTypeConverter - Add tests for ArcBluetoothBridge BUG=b:28250518 TEST=Build / Run tests ========== to ========== Add unit test for ArcBluetoothBridge and TypeConverter - Refactor ArcBluetoothBridge to make it easier for testing. - Add new FakeBluetoothInstance class that simulate Android side Bluetooth ARC - Add tests for BluetoothTypeConverter - Add tests for ArcBluetoothBridge BUG=b:28250518 TEST=Build / Run tests Committed: https://crrev.com/7851146bed75ddf054e1b755c337018753951a3a Cr-Commit-Position: refs/heads/master@{#409945} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/7851146bed75ddf054e1b755c337018753951a3a Cr-Commit-Position: refs/heads/master@{#409945} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
