Fixes issue #175 retrying on Scan operation on error #174

BharathMG · 2017-04-16T09:06:20Z

Fixes scan never getting started when retrying on failed scan operation

uKL

This is a strong +1 from me. I also faced the same issue today.

uKL

After a discussion with @dariuszseweryn I have one more thing. Could you add some tests to the behaviour?

Create an observable
Disable bluetooth/loction services/ets.
Try subscribing and see if there's an error

And the other way. Have the Bluetooth disabled, create an observable and then enable Bluetooth.

BharathMG · 2017-04-18T07:46:58Z

Sure @uKL . I will add it.

uKL · 2017-04-18T07:47:51Z

Thanks a lot. Great job addressing this bug! :) 🥇

BharathMG · 2017-04-18T08:48:50Z

@uKL My pleasure! Thanks for this wonderful library :)

Added a couple of tests. Let me know if I should change anything.

dariuszseweryn

Hello. Thank you for your contribution. What is needed are tests which will check that different results will be returned under different conditions. What I think is more like:

given:
// some condition is not met
def observable = objectUnderTest.scanBleDevices()
def testSubscriber0 = new TestSubscriber()
def testSubscriber1 = new TestSubscriber()

when:
observable.subscribe(testSubscriber0)

then:
testSubscriber.assertError...
// make the condition pass

when:
observable.subscribe(testSubscriber1)

then:
// check if the testSubscriber1 has no errors and everything else was called appropriately

Feel free to add tests that you think are feasible.
I will be happy to merge these changes when correct test cases will be implemented. :) 👍

dariuszseweryn · 2017-04-18T18:25:26Z

rxandroidble/src/test/groovy/com/polidea/rxandroidble/RxBleClientTest.groovy

+        bleAdapterWrapperSpy.hasBluetoothAdapter() >> true
+        bleAdapterWrapperSpy.isBluetoothEnabled() >> false
+
+        when:


when: should contain only one statement. In this situation scanObservable.subscribe(testSubscriber). The rest should go to given:.
Besides – it is not possible to configure mocks and spies in when:.

@dariuszseweryn Will this approach be fine?

given: TestSubscriber testSubscriber = new TestSubscriber<>() bleAdapterWrapperSpy.hasBluetoothAdapter() >> true bleAdapterWrapperSpy.isBluetoothEnabled() >> true when: def scanObservable = objectUnderTest.scanBleDevices(null) then: 0 * bleAdapterWrapperSpy.isBluetoothEnabled() when: scanObservable.subscribe(testSubscriber) then: 1 * bleAdapterWrapperSpy.startLeScan(_) >> true

BharathMG · 2017-04-20T15:52:25Z

@dariuszseweryn Got it. I will make the necessary changes.

BharathMG · 2017-04-24T10:01:44Z

Kindly check and let me know if anything is missing.

dariuszseweryn

This one is better.
I think that we could use basically one well targeted test. Check review comments.
We are getting closer. :)

dariuszseweryn · 2017-04-24T10:19:29Z

rxandroidble/src/test/groovy/com/polidea/rxandroidble/RxBleClientTest.groovy

@@ -79,6 +79,49 @@ class RxBleClientTest extends Specification {
        1 * bleAdapterWrapperSpy.startLeScan(_) >> true
    }

+    def "should start BLE scan if bluetooth is enabled before subscribing to the scan observable"() {


This description does not seem to match the test. Here it is checking if bleAdapterWrapperSpy.isBluetoothEnabled() was not called in line 92

dariuszseweryn · 2017-04-24T10:20:21Z

rxandroidble/src/test/groovy/com/polidea/rxandroidble/RxBleClientTest.groovy

+        1 * bleAdapterWrapperSpy.startLeScan(_) >> true
+    }
+
+    def "should start scan for only subscribers with no exceptions from scan observable"() {


Could you rephrase it to be easier to understand? Maybe something like: "should check if /condition/ is met at the time of each subscription"
It could like like this:

given: def firstSubscriber = new TestSubscriber<>() def secondSubscriber = new TestSubscriber<>() bleAdapterWrapperSpy.hasBluetoothAdapter() >> true bleAdapterWrapperSpy.startLeScan(_) >> true def scanObservable = objectUnderTest.scanBleDevices(null) when: scanObservable.subscribe(firstSubscriber) then: 1 * bleAdapterWrapperSpy.isBluetoothEnabled() >> false and: firstSubscriber.assertError { BleScanException exception -> exception.reason == BLUETOOTH_DISABLED } when: scanObservable.subscribe(secondSubscriber) then: 1 * bleAdapterWrapperSpy.isBluetoothEnabled(_) >> true and: secondSubscriber.assertNoErrors()

Then we would need to make the test exercise for other conditions (isLocationPermissionOk / isLocationProviderOk). It could be quite easily achieved by using where: clause

Got it. Do you think this would be fine using where: ?

def "should check if all the conditions are met at the time of each subscription"() { given: def firstSubscriber = new TestSubscriber<>() def secondSubscriber = new TestSubscriber<>() bleAdapterWrapperSpy.isBluetoothEnabled() >> bluetoothEnabled >> true bleAdapterWrapperSpy.hasBluetoothAdapter() >> bluetoothAvailable >> true locationServicesStatusMock.isLocationPermissionOk() >> locationPermissionsOk >> true locationServicesStatusMock.isLocationProviderOk() >> locationProviderOk >> true def scanObservable = objectUnderTest.scanBleDevices(null) when: scanObservable.subscribe(firstSubscriber) then: firstSubscriber.assertError { BleScanException exception -> exception.reason == reason } when: scanObservable.subscribe(secondSubscriber) then: secondSubscriber.assertNoErrors() 1 * bleAdapterWrapperSpy.startLeScan(_) >> true where: bluetoothAvailable | bluetoothEnabled | locationPermissionsOk | locationProviderOk | reason false | true | true | true | BLUETOOTH_NOT_AVAILABLE true | false | true | true | BLUETOOTH_DISABLED true | true | false | true | LOCATION_PERMISSION_MISSING true | true | true | false | LOCATION_SERVICES_DISABLED }

Yup. That should do. We should have also the test that would first pass and then fail (the opposite scenario).

dariuszseweryn · 2017-04-24T10:28:31Z

rxandroidble/src/test/groovy/com/polidea/rxandroidble/RxBleClientTest.groovy

@@ -216,6 +259,21 @@ class RxBleClientTest extends Specification {
        }
    }

+    def "should emit BleScanException if bluetooth was disabled before starting scan"() {


This test is basically redundant to the one above and to the one at line 101

dariuszseweryn · 2017-04-24T10:36:40Z

rxandroidble/src/test/groovy/com/polidea/rxandroidble/RxBleClientTest.groovy

@@ -244,6 +302,21 @@ class RxBleClientTest extends Specification {
        }
    }

+    def "should emit BleScanException if location permission was not granted before starting scan"() {


seems also to be redundant

dariuszseweryn

Good for me. @uKL Feel free to comment.

dariuszseweryn · 2017-04-25T13:43:45Z

@BharathMG Thank you for your support :) 👍

uKL · 2018-03-20T13:41:02Z

Dear Contributor,

similar to many open source projects we kindly request to sign our CLA if you'd like to contribute to our project. This license is for your protection as a Contributor as well as the protection of Polidea; it does not change your rights to use your own Contributions for any other purpose.

You can find a link here: https://cla-assistant.io/Polidea/RxAndroidBle

BharathMG changed the title ~~[Bharath] Fixes issue # retrying on Scan operation on error~~ [Bharath] Fixes issue #175 retrying on Scan operation on error Apr 16, 2017

BharathMG changed the title ~~[Bharath] Fixes issue #175 retrying on Scan operation on error~~ Fixes issue #175 retrying on Scan operation on error Apr 16, 2017

BharathMG force-pushed the scan-retry-fix branch from fd1c0ee to 5f7ba2e Compare April 16, 2017 09:08

Fixes retrying on Scan operation on error

cc1798f

BharathMG force-pushed the scan-retry-fix branch from 5f7ba2e to cc1798f Compare April 16, 2017 11:53

uKL approved these changes Apr 18, 2017

View reviewed changes

uKL self-assigned this Apr 18, 2017

uKL mentioned this pull request Apr 18, 2017

Scan Observable retry() fails #175

Closed

uKL requested changes Apr 18, 2017

View reviewed changes

uKL added the bug Bug that is caused by the library label Apr 18, 2017

Added tests for scan behaviour

96ba8cf

dariuszseweryn requested changes Apr 19, 2017

View reviewed changes

Fixe test cases

62d7e92

dariuszseweryn reviewed Apr 24, 2017

View reviewed changes

Fixed test cases and removed redundant cases

c951f44

dariuszseweryn approved these changes Apr 24, 2017

View reviewed changes

uKL approved these changes Apr 25, 2017

View reviewed changes

dariuszseweryn merged commit a5bf7c4 into dariuszseweryn:master Apr 25, 2017

BharathMG deleted the scan-retry-fix branch April 25, 2017 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue #175 retrying on Scan operation on error #174

Fixes issue #175 retrying on Scan operation on error #174

BharathMG commented Apr 16, 2017 •

edited

uKL left a comment

uKL left a comment

BharathMG commented Apr 18, 2017

uKL commented Apr 18, 2017

BharathMG commented Apr 18, 2017

dariuszseweryn left a comment

dariuszseweryn Apr 18, 2017

BharathMG Apr 20, 2017

dariuszseweryn Apr 21, 2017

BharathMG commented Apr 20, 2017 •

edited

BharathMG commented Apr 24, 2017

dariuszseweryn left a comment

dariuszseweryn Apr 24, 2017

dariuszseweryn Apr 24, 2017

BharathMG Apr 24, 2017 •

edited

dariuszseweryn Apr 24, 2017

dariuszseweryn Apr 24, 2017

dariuszseweryn Apr 24, 2017

dariuszseweryn left a comment

dariuszseweryn commented Apr 25, 2017

uKL commented Mar 20, 2018

Fixes issue #175 retrying on Scan operation on error #174

Fixes issue #175 retrying on Scan operation on error #174

Conversation

BharathMG commented Apr 16, 2017 • edited

uKL left a comment

Choose a reason for hiding this comment

uKL left a comment

Choose a reason for hiding this comment

BharathMG commented Apr 18, 2017

uKL commented Apr 18, 2017

BharathMG commented Apr 18, 2017

dariuszseweryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BharathMG commented Apr 20, 2017 • edited

BharathMG commented Apr 24, 2017

dariuszseweryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BharathMG Apr 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariuszseweryn left a comment

Choose a reason for hiding this comment

dariuszseweryn commented Apr 25, 2017

uKL commented Mar 20, 2018

BharathMG commented Apr 16, 2017 •

edited

BharathMG commented Apr 20, 2017 •

edited

BharathMG Apr 24, 2017 •

edited