Skip to content

Make isTesterSignedIn more reliable #4493

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 1 commit into from
Jan 3, 2023
Merged

Make isTesterSignedIn more reliable #4493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion ...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,8 +244,9 @@ public Task<Void> signInTester() {

@Override
public void signOutTester() {
cachedNewRelease.set(null);
// Set sign in status first so isTesterSigned returns the correct state as soon as possible
signInStorage.setSignInStatus(false);
cachedNewRelease.set(null);
}

@Override
Expand Down
20 changes: 11 additions & 9 deletions ...appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import android.content.Context;
import android.content.SharedPreferences;
import androidx.annotation.VisibleForTesting;
import com.google.android.gms.tasks.Task;
import com.google.android.gms.tasks.TaskCompletionSource;
import com.google.android.gms.tasks.Tasks;
Expand All @@ -24,8 +25,10 @@

/** Class that handles storage for App Distribution SignIn persistence. */
class SignInStorage {
private static final String SIGNIN_PREFERENCES_NAME = "FirebaseAppDistributionSignInStorage";
private static final String SIGNIN_TAG = "firebase_app_distribution_signin";
@VisibleForTesting
static final String SIGNIN_PREFERENCES_NAME = "FirebaseAppDistributionSignInStorage";

@VisibleForTesting static final String SIGNIN_TAG = "firebase_app_distribution_signin";

private final Context applicationContext;
@Background private final Executor backgroundExecutor;
Expand Down Expand Up @@ -54,12 +57,14 @@ Task<Boolean> getSignInStatus() {
}

boolean getSignInStatusBlocking() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this method be renamed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think so. It still returns the sign in status, and it is still potentially blocking.

I changed getSharedPreferencesBlocking() to getAndCacheSharedPreferences() because if I included Blocking it would be super long and I didn't think it was necessary for the private method name to communicate that it blocks.

return getSharedPreferencesBlocking().getBoolean(SIGNIN_TAG, false);
return getAndCacheSharedPreferences().getBoolean(SIGNIN_TAG, false);
}

private SharedPreferences getSharedPreferencesBlocking() {
private SharedPreferences getAndCacheSharedPreferences() {
// This may construct a new SharedPreferences object, which requires storage I/O
return applicationContext.getSharedPreferences(SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE);
sharedPreferences =
applicationContext.getSharedPreferences(SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE);
return sharedPreferences;
}

private <T> Task<T> applyToSharedPreferences(SharedPreferencesFunction<T> func) {
Expand All @@ -72,10 +77,7 @@ private <T> Task<T> applyToSharedPreferences(SharedPreferencesFunction<T> func)
}
TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
backgroundExecutor.execute(
() -> {
sharedPreferences = getSharedPreferencesBlocking();
taskCompletionSource.setResult(func.apply(sharedPreferences));
});
() -> taskCompletionSource.setResult(func.apply(getAndCacheSharedPreferences())));
return taskCompletionSource.getTask();
}
}
91 changes: 61 additions & 30 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 @@ -14,6 +14,7 @@

package com.google.firebase.appdistribution.impl;

import static android.content.Context.MODE_PRIVATE;
import static android.os.Looper.getMainLooper;
import static com.google.common.truth.Truth.assertThat;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_CANCELED;
Expand Down Expand Up @@ -43,7 +44,6 @@
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 @@ -58,6 +58,7 @@
import android.app.Dialog;
import android.content.DialogInterface;
import android.content.Intent;
import android.content.SharedPreferences;
import android.content.pm.ApplicationInfo;
import android.content.pm.PackageInfo;
import android.net.Uri;
Expand All @@ -70,6 +71,7 @@
import com.google.android.gms.tasks.Tasks;
import com.google.firebase.FirebaseApp;
import com.google.firebase.FirebaseOptions;
import com.google.firebase.annotations.concurrent.Background;
import com.google.firebase.annotations.concurrent.Blocking;
import com.google.firebase.annotations.concurrent.Lightweight;
import com.google.firebase.appdistribution.AppDistributionRelease;
Expand Down Expand Up @@ -143,18 +145,18 @@ public class FirebaseAppDistributionServiceImplTest {

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

private FirebaseAppDistributionImpl firebaseAppDistribution;
private ActivityController<TestActivity> activityController;
private TestActivity activity;
private FirebaseApp firebaseApp;

@Mock private InstallationTokenResult mockInstallationTokenResult;
@Mock private TesterSignInManager mockTesterSignInManager;
@Mock private NewReleaseFetcher mockNewReleaseFetcher;
@Mock private ApkUpdater mockApkUpdater;
@Mock private AabUpdater mockAabUpdater;
@Mock private SignInStorage mockSignInStorage;
@Mock private SignInStorage signInStorage;
@Mock private FirebaseAppDistributionLifecycleNotifier mockLifecycleNotifier;
@Mock private ReleaseIdentifier mockReleaseIdentifier;
@Mock private ScreenshotTaker mockScreenshotTaker;
Expand All @@ -168,7 +170,10 @@ public void setup() throws FirebaseAppDistributionException {

FirebaseApp.clearInstancesForTest();

firebaseApp =
signInStorage =
spy(new SignInStorage(ApplicationProvider.getApplicationContext(), backgroundExecutor));

FirebaseApp firebaseApp =
FirebaseApp.initializeApp(
ApplicationProvider.getApplicationContext(),
new FirebaseOptions.Builder()
Expand All @@ -185,16 +190,15 @@ public void setup() throws FirebaseAppDistributionException {
mockNewReleaseFetcher,
mockApkUpdater,
mockAabUpdater,
mockSignInStorage,
signInStorage,
mockLifecycleNotifier,
mockReleaseIdentifier,
mockScreenshotTaker,
lightweightExecutor,
blockingExecutor));

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

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

Expand Down Expand Up @@ -244,7 +248,7 @@ public void checkForNewRelease_whenCheckForNewReleaseFails_throwsError() {

@Test
public void checkForNewRelease_testerIsNotSignedIn_taskFails() {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);

Task<AppDistributionRelease> task = firebaseAppDistribution.checkForNewRelease();

Expand Down Expand Up @@ -277,12 +281,12 @@ public void checkForNewRelease_authenticationFailure_signOutTester() throws Inte

awaitTaskFailure(task, AUTHENTICATION_FAILURE, "Test");
awaitTermination(lightweightExecutor);
verify(mockSignInStorage, times(1)).setSignInStatus(false);
verify(signInStorage, times(1)).setSignInStatus(false);
}

@Test
public void updateApp_whenNotSignedIn_throwsError() {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);

UpdateTask updateTask = firebaseAppDistribution.updateApp();

Expand Down Expand Up @@ -372,7 +376,7 @@ public void updateIfNewReleaseAvailable_whenActivityBackgrounded_updateDialogNot
@Test
public void updateIfNewReleaseAvailable_whenSignInCancelled_checkForUpdateNotCalled()
throws InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);
when(mockTesterSignInManager.signInTester())
.thenReturn(
Tasks.forException(
Expand All @@ -393,7 +397,7 @@ public void updateIfNewReleaseAvailable_whenSignInCancelled_checkForUpdateNotCal
@Test
public void updateIfNewReleaseAvailable_whenSignInFailed_checkForUpdateNotCalled()
throws InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);
when(mockTesterSignInManager.signInTester())
.thenReturn(
Tasks.forException(
Expand Down Expand Up @@ -469,7 +473,6 @@ public void updateIfNewReleaseAvailable_whenCheckForUpdateFails_updateAppNotCall
public void updateIfNewReleaseAvailable_whenTesterIsSignedIn_doesNotOpenDialog()
throws InterruptedException, FirebaseAppDistributionException, ExecutionException {
when(mockNewReleaseFetcher.checkForNewRelease()).thenReturn(Tasks.forResult(null));
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));

UpdateTask task = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitTask(task);
Expand All @@ -479,8 +482,8 @@ public void updateIfNewReleaseAvailable_whenTesterIsSignedIn_doesNotOpenDialog()

@Test
public void signInTester_whenDialogDismissed_taskFails() throws InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
Task updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
setSignInStatusSharedPreference(false);
UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(lightweightExecutor);

AlertDialog dialog = assertAlertDialogShown();
Expand All @@ -492,8 +495,8 @@ public void signInTester_whenDialogDismissed_taskFails() throws InterruptedExcep
@Test
public void updateIfNewReleaseAvailable_whenSignInDialogCanceled_taskFails()
throws InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
Task signInTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
setSignInStatusSharedPreference(false);
UpdateTask signInTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(lightweightExecutor);

AlertDialog dialog = assertAlertDialogShown();
Expand All @@ -514,7 +517,7 @@ private AlertDialog assertAlertDialogShown() {
public void signOutTester_setsSignInStatusFalse() {
firebaseAppDistribution.signOutTester();

verify(mockSignInStorage).setSignInStatus(false);
assertThat(signInStorage.getSignInStatusBlocking()).isFalse();
}

@Test
Expand All @@ -532,6 +535,30 @@ public void signOutTester_unsetsCachedNewRelease()
assertThat(cachedNewReleaseTask.getResult()).isNull();
}

@Test
public void isTesterSignedIn_returnsTrueWhenSharePreferenceIsTrue() {
setSignInStatusSharedPreference(true);
assertThat(firebaseAppDistribution.isTesterSignedIn()).isTrue();
}

@Test
public void isTesterSignedIn_returnsFalseWhenSharePreferenceIsFalse() {
setSignInStatusSharedPreference(false);
assertThat(firebaseAppDistribution.isTesterSignedIn()).isFalse();
}

@Test
public void isTesterSignedIn_whenStorageWarmedUp_returnsCorrectStatusImmediately() {
// First check the sign in status, which has the side effect of warming up the SharedPreferences
// instance. This simulates a typical flow, where a developer would check the status before
// trying to sign out the tester.
firebaseAppDistribution.isTesterSignedIn();

firebaseAppDistribution.signOutTester();

assertThat(firebaseAppDistribution.isTesterSignedIn()).isFalse();
}

@Test
public void updateIfNewReleaseAvailable_receiveProgressUpdateFromUpdateApp()
throws InterruptedException {
Expand Down Expand Up @@ -565,21 +592,21 @@ public void updateIfNewReleaseAvailable_receiveProgressUpdateFromUpdateApp()
@Test
public void updateIfNewReleaseAvailable_fromABackgroundThread_showsSignInDialog()
throws InterruptedException, ExecutionException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);

ExecutorService executorService = Executors.newSingleThreadExecutor();
Future<UpdateTask> future =
executorService.submit(() -> firebaseAppDistribution.updateIfNewReleaseAvailable());
awaitAsyncOperations(executorService);

assertAlertDialogShown();
assertFalse(((UpdateTask) future.get()).isComplete());
assertFalse((future.get()).isComplete());
}

@Test
public void updateIfNewReleaseAvailable_whenScreenRotates_signInConfirmationDialogReappears()
throws InterruptedException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);
when(activity.isChangingConfigurations()).thenReturn(true);

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
Expand All @@ -597,8 +624,8 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_signInConfirmationDial
@Test
public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears()
throws InterruptedException {
AppDistributionReleaseInternal newRelease = TEST_RELEASE_NEWER_AAB_INTERNAL;
when(mockNewReleaseFetcher.checkForNewRelease()).thenReturn(Tasks.forResult(newRelease));
when(mockNewReleaseFetcher.checkForNewRelease())
.thenReturn(Tasks.forResult(TEST_RELEASE_NEWER_AAB_INTERNAL));
when(activity.isChangingConfigurations()).thenReturn(true);

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
Expand All @@ -617,7 +644,7 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears(
updateIfNewReleaseAvailable_whenSignInDialogShowingAndNewActivityStarts_signInTaskCancelled()
throws InterruptedException {
TestActivity testActivity2 = new TestActivity();
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(false));
setSignInStatusSharedPreference(false);

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(lightweightExecutor);
Expand All @@ -635,8 +662,8 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears(
updateIfNewReleaseAvailable_whenUpdateDialogShowingAndNewActivityStarts_updateTaskCancelled()
throws InterruptedException {
TestActivity testActivity2 = new TestActivity();
AppDistributionReleaseInternal newRelease = TEST_RELEASE_NEWER_AAB_INTERNAL;
when(mockNewReleaseFetcher.checkForNewRelease()).thenReturn(Tasks.forResult(newRelease));
when(mockNewReleaseFetcher.checkForNewRelease())
.thenReturn(Tasks.forResult(TEST_RELEASE_NEWER_AAB_INTERNAL));

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(lightweightExecutor);
Expand All @@ -652,7 +679,6 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears(
@Test
public void updateAppTask_whenNoReleaseAvailable_throwsError() {
firebaseAppDistribution.getCachedNewRelease().set(null);
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));

UpdateTask updateTask = firebaseAppDistribution.updateApp();

Expand All @@ -666,7 +692,6 @@ public void updateApp_withAabReleaseAvailable_returnsSameAabTask()
firebaseAppDistribution.getCachedNewRelease().set(release);
UpdateTaskImpl updateTaskToReturn = new UpdateTaskImpl();
doReturn(updateTaskToReturn).when(mockAabUpdater).updateAab(release);
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));

UpdateTask updateTask = firebaseAppDistribution.updateApp();
assertFalse(updateTask.isComplete());
Expand All @@ -682,7 +707,6 @@ public void updateApp_withAabReleaseAvailable_returnsSameAabTask()
@Test
public void updateApp_withApkReleaseAvailable_returnsSameApkTask()
throws InterruptedException, FirebaseAppDistributionException, ExecutionException {
when(mockSignInStorage.getSignInStatus()).thenReturn(Tasks.forResult(true));
AppDistributionReleaseInternal release = TEST_RELEASE_NEWER_APK_INTERNAL;
firebaseAppDistribution.getCachedNewRelease().set(release);
UpdateTaskImpl updateTaskToReturn = new UpdateTaskImpl();
Expand Down Expand Up @@ -829,9 +853,16 @@ private static void assertLoggedError(String partialMessage, Throwable e) {
log ->
log.type == Log.ERROR
&& log.msg.contains(partialMessage)
&& (e != null ? log.throwable == e : true);
&& (e == null || log.throwable == e);
List<ShadowLog.LogItem> matchingLogs =
ShadowLog.getLogs().stream().filter(predicate).collect(toList());
assertThat(matchingLogs).hasSize(1);
}

private void setSignInStatusSharedPreference(boolean testerSignedIn) {
SharedPreferences sharedPreferences =
ApplicationProvider.getApplicationContext()
.getSharedPreferences(SignInStorage.SIGNIN_PREFERENCES_NAME, MODE_PRIVATE);
sharedPreferences.edit().putBoolean(SignInStorage.SIGNIN_TAG, testerSignedIn).commit();
}
}