Fix TelemetryService and applicationContext issue #265
Conversation
- add AndroidX for service testing - checksLocationPermission test - make locationPermissionCheck package-private - remove null check from method - move to getApplicationContext()
| @@ -181,6 +179,16 @@ boolean isTelemetryReceiverRegistered() { | |||
| return isTelemetryReceiverRegistered; | |||
| } | |||
|
|
|||
| boolean locationPermissionCheck() { | |||
There was a problem hiding this comment.
since we've dependency on supportlib-v4 is there a reason why we're not simply doing:
| boolean locationPermissionCheck() { | |
| private boolean locationPermissionCheck() { | |
| return ContextCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.ACCESS_FINE_LOCATION) | |
| == PackageManager.PERMISSION_GRANTED; | |
| } |
wouldn't ContextCompat handle if (Build.VERSION.SDK_INT >= API_LEVEL_23) logic on your behalf? Also if it does how do we test for that?
| @@ -181,6 +179,16 @@ boolean isTelemetryReceiverRegistered() { | |||
| return isTelemetryReceiverRegistered; | |||
| } | |||
|
|
|||
| boolean locationPermissionCheck() { | |||
There was a problem hiding this comment.
if we're changing method's visibility modifier in order to get it under test harness, we should always annotate it with @VisibleForTesting otherwise someone may think that we're not properly encapsulating our code. Now, another question is it really necessary to change its visibility in order to test it?
| @@ -135,6 +139,17 @@ public void checksOnTaskRemovedCallbackWhenOnTaskRemovedCalled() throws Exceptio | |||
| verify(mockedCallback, times(1)).onTaskRemoved(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void checksLocationPermission() throws Exception { | |||
| Intent serviceIntent = new Intent(InstrumentationRegistry.getTargetContext(), TelemetryService.class); | |||
There was a problem hiding this comment.
what are we trying to test here for? since permission is already granted with GrantPermissionRule how useful is it? Should we consider implementing a test when permissions are not present and try to verify the outcome/result with public APIs exposed by TelemetryService?
gradle/dependencies.gradle
Outdated
| assertJ : "org.assertj:assertj-core:${version.assertJ}", | ||
|
|
||
| //AndroidX | ||
| androidXEspresso : "androidx.test.espresso:espresso-core:${version.espressoVersion}", |
There was a problem hiding this comment.
are these dependencies necessary to compile test code?
- remove unused rule - remove unused dependencies - simplify locationPermissionCheck
- unused imports
- remove unused import in test (not caught by local checkstyle)
| @@ -181,6 +178,12 @@ boolean isTelemetryReceiverRegistered() { | |||
| return isTelemetryReceiverRegistered; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| boolean locationPermissionCheck() { | |||
| return ContextCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.ACCESS_FINE_LOCATION) | |||
There was a problem hiding this comment.
would you be able to test this fix on at API 19, 23, 26, 28?
* Refactor TelemetryService out of static application context reference * Add integration test with activity and service * Fix potential issue with intent being null onStartCommand
Addresses: #263
Address issue where static applicationContext set by the activity is null, but service is still operating. Lifecycles are detached and can cause potential issues in lower API levels. Goal is to test and prevent crash from occurring. If possible, refactor all tangental code ran by the service to prevent this as well.