Skip to content

Do not wait to store state when signing out #4490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Do not wait to store state when signing out #4490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9afb4ba
Do not wait to store state when signing out
lfkellogg Dec 22, 2022
ef1897a
Remove unnecessary assertions during test setup
lfkellogg Dec 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions ...n/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,8 @@ public Task<Void> signInTester() {

@Override
public void signOutTester() {
cachedNewRelease
.set(null)
.addOnSuccessListener(lightweightExecutor, unused -> signInStorage.setSignInStatus(false));
cachedNewRelease.set(null);
signInStorage.setSignInStatus(false);
}

@Override
Expand Down
41 changes: 33 additions & 8 deletions ...java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.HOST_ACTIVITY_INTERRUPTED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.INSTALLATION_CANCELED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.NETWORK_FAILURE;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UNKNOWN;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UPDATE_NOT_AVAILABLE;
import static com.google.firebase.appdistribution.impl.ErrorMessages.AUTHENTICATION_ERROR;
import static com.google.firebase.appdistribution.impl.ErrorMessages.JSON_PARSING_ERROR;
Expand All @@ -42,6 +43,7 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand All @@ -68,6 +70,8 @@
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApp;
import com.google.firebase.FirebaseOptions;
import com.google.firebase.annotations.concurrent.Blocking;
import com.google.firebase.annotations.concurrent.Lightweight;
import com.google.firebase.appdistribution.AppDistributionRelease;
import com.google.firebase.appdistribution.BinaryType;
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
Expand Down Expand Up @@ -137,8 +141,8 @@ public class FirebaseAppDistributionServiceImplTest {
.setDownloadUrl(TEST_URL)
.build();

private final ExecutorService lightweightExecutor = TestOnlyExecutors.lite();
private final ExecutorService blockingExecutor = TestOnlyExecutors.blocking();
@Lightweight private final ExecutorService lightweightExecutor = TestOnlyExecutors.lite();
@Blocking private final ExecutorService blockingExecutor = TestOnlyExecutors.blocking();

private FirebaseAppDistributionImpl firebaseAppDistribution;
private ActivityController<TestActivity> activityController;
Expand Down Expand Up @@ -190,6 +194,7 @@ public void setup() throws FirebaseAppDistributionException {

when(mockTesterSignInManager.signInTester()).thenReturn(Tasks.forResult(null));
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));
when(mockSignInStorage.setSignInStatus(anyBoolean())).thenReturn(Tasks.forResult(null));

when(mockInstallationTokenResult.getToken()).thenReturn(TEST_AUTH_TOKEN);

Expand Down Expand Up @@ -506,12 +511,33 @@ private AlertDialog assertAlertDialogShown() {
}

@Test
public void signOutTester_setsSignInStatusFalse() throws InterruptedException {
public void signOutTester_setsSignInStatusFalse() {
firebaseAppDistribution.signOutTester();
awaitTermination(lightweightExecutor);

verify(mockSignInStorage).setSignInStatus(false);
}

@Test
public void signOutTester_unsetsCachedNewRelease()
throws InterruptedException, FirebaseAppDistributionException, ExecutionException {
Task<AppDistributionReleaseInternal> setCachedNewReleaseTask =
firebaseAppDistribution.getCachedNewRelease().set(TEST_RELEASE_NEWER_AAB_INTERNAL);

// Confirm that the cached new release is initially set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this pattern - I think, in general, we should trust (but not verify) test setup.
In this case I think you should be able to drop this comment, the blank line above it, and the assert on line 528

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Removed.

awaitTask(setCachedNewReleaseTask);
assertThat(setCachedNewReleaseTask.getResult()).isEqualTo(TEST_RELEASE_NEWER_AAB_INTERNAL);

// Sign out the tester
firebaseAppDistribution.signOutTester();
awaitAsyncOperations(lightweightExecutor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this change correctly, this should no be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right, great catch!


// Confirm that the cached new release is now null
Task<AppDistributionReleaseInternal> cachedNewReleaseTask =
firebaseAppDistribution.getCachedNewRelease().get();
awaitTask(cachedNewReleaseTask);
assertThat(cachedNewReleaseTask.getResult()).isNull();
}

@Test
public void updateIfNewReleaseAvailable_receiveProgressUpdateFromUpdateApp()
throws InterruptedException {
Expand Down Expand Up @@ -759,8 +785,7 @@ public boolean isFinishing() {
public void startFeedback_screenshotFails_startActivityWithNoScreenshot()
throws InterruptedException {
when(mockScreenshotTaker.takeScreenshot())
.thenReturn(
Tasks.forException(new FirebaseAppDistributionException("Error", Status.UNKNOWN)));
.thenReturn(Tasks.forException(new FirebaseAppDistributionException("Error", UNKNOWN)));
when(mockReleaseIdentifier.identifyRelease()).thenReturn(Tasks.forResult("release-name"));

firebaseAppDistribution.startFeedback("Some terms and conditions");
Expand All @@ -781,7 +806,7 @@ public void startFeedback_signInTesterFails_logsAndSetsInProgressToFalse()
throws InterruptedException {
when(mockReleaseIdentifier.identifyRelease()).thenReturn(Tasks.forResult("release-name"));
FirebaseAppDistributionException exception =
new FirebaseAppDistributionException("Error", Status.UNKNOWN);
new FirebaseAppDistributionException("Error", UNKNOWN);
when(mockTesterSignInManager.signInTester()).thenReturn(Tasks.forException(exception));

firebaseAppDistribution.startFeedback("Some terms and conditions");
Expand All @@ -795,7 +820,7 @@ public void startFeedback_signInTesterFails_logsAndSetsInProgressToFalse()
public void startFeedback_cantIdentifyRelease_logsAndSetsInProgressToFalse()
throws InterruptedException {
FirebaseAppDistributionException exception =
new FirebaseAppDistributionException("Error", Status.UNKNOWN);
new FirebaseAppDistributionException("Error", UNKNOWN);
when(mockReleaseIdentifier.identifyRelease()).thenReturn(Tasks.forException(exception));

firebaseAppDistribution.startFeedback("Some terms and conditions");
Expand Down