Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1804093003:
Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test (Closed)
DescriptionAdd BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test
This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test.
It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback.
BUG=579202
Committed: https://crrev.com/92409da1108f220fae4a4b4a3f71493da12098b1
Cr-Commit-Position: refs/heads/master@{#391299}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 12
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 52 (26 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== add reentrant unit tests BUG= ========== to ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error callback. BUG=579202 ==========
Description was changed from ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error callback. BUG=579202 ========== to ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error callback. BUG=579202 ==========
gogerald@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
PTAL
https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:901: TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Reentrant) { Please test all 4 combinations of initial callback, reentrant callback success, success success, error error, success error, error I suggest using a helper method on the fixture for this and calling with 4 very short tests with a single call and different arguments. https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:907: "00002902-0000-1000-8000-00805F9B34FB"); BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid()
ping?
Working on Clank sign in for Narnia 2.0 which has higher priority with tight schedule (target M51), will come back to this CL after I've done that. On Wed, Apr 13, 2016 at 5:30 PM, <ortuno@chromium.org> wrote: > ping? > > https://codereview.chromium.org/1804093003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, thanks for the update! On Wed, Apr 13, 2016 at 2:43 PM Ganggui Tang <gogerald@google.com> wrote: > Working on Clank sign in for Narnia 2.0 which has higher priority with > tight schedule (target M51), will come back to this CL after I've done that. > > On Wed, Apr 13, 2016 at 5:30 PM, <ortuno@chromium.org> wrote: > >> ping? >> >> https://codereview.chromium.org/1804093003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
BTW, it would be better that somebody from your team could design these tests for one platform and let me apply them on Windows, since your team will be the owner to maintain this component. On Wed, Apr 13, 2016 at 5:43 PM, Ganggui Tang <gogerald@google.com> wrote: > Working on Clank sign in for Narnia 2.0 which has higher priority with > tight schedule (target M51), will come back to this CL after I've done that. > > On Wed, Apr 13, 2016 at 5:30 PM, <ortuno@chromium.org> wrote: > >> ping? >> >> https://codereview.chromium.org/1804093003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/120001
https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:901: TEST_F(BluetoothGattCharacteristicTest, StartNotifySession_Reentrant) { On 2016/03/19 04:58:07, scheib wrote: > Please test all 4 combinations of > initial callback, reentrant callback > success, success > success, error > error, success > error, error > I suggest using a helper method on the fixture for this and calling with 4 very > short tests with a single call and different arguments. Done. "success, error" looks impossible in current code. https://codereview.chromium.org/1804093003/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:907: "00002902-0000-1000-8000-00805F9B34FB"); On 2016/03/19 04:58:07, scheib wrote: > BluetoothGattDescriptor::ClientCharacteristicConfigurationUuid() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
The CQ bit was unchecked by gogerald@chromium.org
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error callback. BUG=579202 ========== to ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback. BUG=579202 ==========
FYI: You are still missing two tests: RegisterGattEvents fails and SetDescriptor fails. No need to do them on this CL though. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1010: characteristic1_, false)); false /* error_in_reentrant */ https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1050: characteristic1_, false)); false /* error_in_reentrant */ https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1053: EXPECT_EQ(0, gatt_notify_characteristic_attempts_); Add: EXPECT_EQ(1, error_callback_count_); https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1088: characteristic1_, true)); true /* error_in_reentrant */ https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.cc (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.cc:169: notify_sessions_.push_back(notify_session.release()); I think std::move(notify_session) would be better here. Specially since we will soon get rid of ScopedVector. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.h (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.h:175: virtual void SimulateGattCharacteristicSetNotifyWillFailAsynchronouslyOnce( I think you can just reuse: SimulateGattNotifySessionStartError
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/180001
https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1010: characteristic1_, false)); On 2016/04/22 16:23:39, ortuno wrote: > false /* error_in_reentrant */ Done. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1050: characteristic1_, false)); On 2016/04/22 16:23:39, ortuno wrote: > false /* error_in_reentrant */ Done. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1053: EXPECT_EQ(0, gatt_notify_characteristic_attempts_); On 2016/04/22 16:23:39, ortuno wrote: > Add: EXPECT_EQ(1, error_callback_count_); Done. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:1088: characteristic1_, true)); On 2016/04/22 16:23:39, ortuno wrote: > true /* error_in_reentrant */ Done. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.cc (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.cc:169: notify_sessions_.push_back(notify_session.release()); On 2016/04/22 16:23:39, ortuno wrote: > I think std::move(notify_session) would be better here. Specially since we will > soon get rid of ScopedVector. Done. https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test.h (right): https://codereview.chromium.org/1804093003/diff/160001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test.h:175: virtual void SimulateGattCharacteristicSetNotifyWillFailAsynchronouslyOnce( On 2016/04/22 16:23:39, ortuno wrote: > I think you can just reuse: SimulateGattNotifySessionStartError Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1804093003/diff/180001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1804093003/diff/180001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:389: CHECK(simulated_characteristic); DCHECK is enough. CHECK is normally used if failing the condition would result in a security vulnerability: https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...
FYI: You are still missing two tests: RegisterGattEvents fails and SetDescriptor fails. Are you planning on writing those as well? If not, could you open an issue so we don't forget?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/200001
Sure, will create a bug to track it, but I am not planning to work on it, so will assign it to you. I will stop working on this module after this CL and probably one additional simple CL to enable existing tests for Descriptor and one for stop notify session after the test fixture is ready when I have time. https://codereview.chromium.org/1804093003/diff/180001/device/bluetooth/test/... File device/bluetooth/test/bluetooth_test_win.cc (right): https://codereview.chromium.org/1804093003/diff/180001/device/bluetooth/test/... device/bluetooth/test/bluetooth_test_win.cc:389: CHECK(simulated_characteristic); On 2016/04/28 16:21:25, ortuno wrote: > DCHECK is enough. CHECK is normally used if failing the condition would result > in a security vulnerability: > > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/1804093003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804093003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804093003/200001
Message was sent while issue was closed.
Description was changed from ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback. BUG=579202 ========== to ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback. BUG=579202 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback. BUG=579202 ========== to ========== Add BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test This CL implements BluetoothGattCharacteristicTest::StartNotifySession_Reentrant unit test. It tests BluetoothGattCharacteristic::StartNotifySession in StartNotifySession error and success callback. BUG=579202 Committed: https://crrev.com/92409da1108f220fae4a4b4a3f71493da12098b1 Cr-Commit-Position: refs/heads/master@{#391299} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/92409da1108f220fae4a4b4a3f71493da12098b1 Cr-Commit-Position: refs/heads/master@{#391299} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
