Fad cancel task when dialog closed #3228
Conversation
rachaprince
requested review from
pranavrajgopal,
lfkellogg and
manny-jimenez
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (295c9a5d) is created by Prow via merging commits: 28e1a16 2bbf265. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (295c9a5d) is created by Prow via merging commits: 28e1a16 2bbf265. |
...distribution/src/main/java/com/google/firebase/app/distribution/FirebaseAppDistribution.java
Show resolved
Hide resolved
| } | ||
| }); | ||
| } | ||
| (dialogInterface, i) -> { |
| when(mockCheckForNewReleaseClient.checkForNewRelease()).thenReturn(Tasks.forResult(newRelease)); | ||
| firebaseAppDistribution.setCachedNewRelease(newRelease); |
There was a problem hiding this comment.
Why do we have to both mock checkForNewRelease() AND set the cached new release?
There was a problem hiding this comment.
We don't. Good catch. (I think this is dead code from a previous implementation)
| @@ -475,4 +475,20 @@ public void updateApp_withApkReleaseAvailable_returnsSameApkTask() { | |||
|
|
|||
| assertEquals(updateTask, updateTaskToReturn); | |||
| } | |||
|
|
|||
| private UpdateTask updateToNewRelease(AppDistributionReleaseInternal newRelease) { | |||
There was a problem hiding this comment.
I like the consolidation of setup, but this is combining some of the setup with the actual actions being tested. Could we split out the mocking/caching from the updateIfNewReleaseAvailable() call?
There was a problem hiding this comment.
Good call. Done. After removing the setCachedNewRelease call below, I felt like this wasn't doing enough to justify a separate method.
I moved the signin mocking to the setup method, since most tests aren't testing the signIn logic
| @@ -374,14 +393,6 @@ public void updateToNewRelease_whenCheckForUpdateFails_updateAppNotCalled() { | |||
| assertEquals(FirebaseAppDistributionException.Status.NETWORK_FAILURE, e.getErrorCode()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void updateToNewRelease_callsSignInTester() { | |||
There was a problem hiding this comment.
Just checking: did you remove this test because it's only verifying a method call?
There was a problem hiding this comment.
Ah yes, I meant to leave a note here. I felt this test was testing implementation, not behavior, so I removed it
Cancel the ongoing task when the dialog is dismissed or cancelled. Prevents hanging tasks