Skip to content

Use sequential executor in place of synchronized blocks for cached new release #4430

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 12 commits into from
Dec 9, 2022
Merged

Use sequential executor in place of synchronized blocks for cached new release #4430

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8d14efe
Remove some synchronized blocks from FirebaseAppDistributionImpl
lfkellogg Nov 22, 2022
218af55
Rename UpdateTaskCache.java to TaskCache.java
lfkellogg Nov 22, 2022
2f398e6
Remove more sync blocks around new release cache
lfkellogg Nov 22, 2022
4c9243e
Some refactoring
lfkellogg Dec 7, 2022
8c8ac7b
Java formatting
lfkellogg Dec 7, 2022
1355cb1
Add tests
lfkellogg Dec 7, 2022
928ff1a
Add javadocs
lfkellogg Dec 8, 2022
f618340
Simplify SequentialReference and update tests
lfkellogg Dec 8, 2022
1472f3e
Update registrar
lfkellogg Dec 9, 2022
0b6a977
Fix test and add static imports from TestUtils
lfkellogg Dec 9, 2022
e1bd329
Incorporate some feedback
lfkellogg Dec 9, 2022
9ef0e50
Use actual lightweight executor in test
lfkellogg Dec 9, 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
1 change: 1 addition & 0 deletions firebase-appdistribution/firebase-appdistribution.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ dependencies {
implementation project(':firebase-installations-interop')
implementation project(':firebase-common')
testImplementation project(path: ':firebase-appdistribution')
testImplementation project(':integ-testing')
runtimeOnly project(':firebase-installations')

testImplementation 'junit:junit:4.13.2'
Expand Down
1 change: 1 addition & 0 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 @@ -320,6 +320,7 @@ void onActivityResumed(Activity activity) {
cachedNewRelease
.get()
.addOnSuccessListener(
lightweightExecutor,
release ->
showUpdateConfirmationDialog(
activity, ReleaseUtils.convertToAppDistributionRelease(release)));
Expand Down
29 changes: 15 additions & 14 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 @@ -65,6 +65,7 @@
import com.google.firebase.appdistribution.UpdateStatus;
import com.google.firebase.appdistribution.UpdateTask;
import com.google.firebase.concurrent.FirebaseExecutors;
import com.google.firebase.concurrent.TestOnlyExecutors;
import com.google.firebase.installations.InstallationTokenResult;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -119,7 +120,7 @@ public class FirebaseAppDistributionServiceImplTest {
.setDownloadUrl(TEST_URL)
.build();

private final ExecutorService testExecutor = Executors.newSingleThreadExecutor();
private final ExecutorService lightweightExecutor = TestOnlyExecutors.lite();

private FirebaseAppDistributionImpl firebaseAppDistribution;
private TestActivity activity;
Expand Down Expand Up @@ -161,7 +162,7 @@ public void setup() throws FirebaseAppDistributionException {
mockAabUpdater,
mockSignInStorage,
mockLifecycleNotifier,
testExecutor));
lightweightExecutor));

when(mockTesterSignInManager.signInTester()).thenReturn(Tasks.forResult(null));
when(mockSignInStorage.getSignInStatus()).thenReturn(true);
Expand Down Expand Up @@ -236,7 +237,7 @@ public void checkForNewRelease_authenticationFailure_signOutTester() throws Inte
Task<AppDistributionRelease> task = firebaseAppDistribution.checkForNewRelease();

awaitTaskFailure(task, AUTHENTICATION_FAILURE, "Test");
awaitTermination(testExecutor);
awaitTermination(lightweightExecutor);
Copy link
Member

Choose a reason for hiding this comment

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

hm, these executors cannot be shutdown so I am not sure awaitTermination would work

Copy link
Member

Choose a reason for hiding this comment

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

but looks like tests have passed, so use your judgement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm realizing that all of our awaitTermination() calls are really just sleeping for 100 ms because we're never shutting them down first. We need a better way to wait for executor-run tasks throughout our tests. Since these are passing right now though I'm inclined to tackle that in a followup.

verify(mockSignInStorage, times(1)).setSignInStatus(false);
}

Expand All @@ -256,7 +257,7 @@ public void updateIfNewReleaseAvailable_whenNewAabReleaseAvailable_showsUpdateDi
.thenReturn(Tasks.forResult((TEST_RELEASE_NEWER_AAB_INTERNAL)));

firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

AlertDialog dialog = assertAlertDialogShown();
assertEquals(
Expand Down Expand Up @@ -290,7 +291,7 @@ public void updateIfNewReleaseAvailable_whenReleaseNotesEmpty_doesNotShowRelease
(TEST_RELEASE_NEWER_AAB_INTERNAL.toBuilder().setReleaseNotes("").build())));

firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

AlertDialog dialog = assertAlertDialogShown();
assertEquals(
Expand All @@ -309,7 +310,7 @@ public void updateIfNewReleaseAvailable_whenNoReleaseAvailable_updateDialogNotSh

List<UpdateProgress> progressEvents = new ArrayList<>();
task.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents::add);
awaitTermination(testExecutor);
awaitTermination(lightweightExecutor);

assertEquals(1, progressEvents.size());
assertEquals(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE, progressEvents.get(0).getUpdateStatus());
Expand All @@ -325,7 +326,7 @@ public void updateIfNewReleaseAvailable_whenActivityBackgrounded_updateDialogNot

List<UpdateProgress> progressEvents = new ArrayList<>();
task.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents::add);
awaitTermination(testExecutor);
awaitTermination(lightweightExecutor);

assertEquals(1, progressEvents.size());
assertEquals(UpdateStatus.NEW_RELEASE_NOT_AVAILABLE, progressEvents.get(0).getUpdateStatus());
Expand Down Expand Up @@ -375,7 +376,7 @@ public void updateIfNewReleaseAvailable_whenDialogDismissed_taskFails()
.thenReturn(Tasks.forResult(TEST_RELEASE_NEWER_AAB_INTERNAL));

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

AlertDialog updateDialog = assertAlertDialogShown();
updateDialog.getButton(AlertDialog.BUTTON_NEGATIVE).performClick(); // dismiss dialog
Expand All @@ -393,7 +394,7 @@ public void updateIfNewReleaseAvailable_whenDialogCanceled_taskFails()
UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();

// Task callbacks happen on the executor
awaitTermination(testExecutor);
awaitTermination(lightweightExecutor);

// Show update confirmation dialog happens on the UI thread
shadowOf(getMainLooper()).idle();
Expand Down Expand Up @@ -469,7 +470,7 @@ private AlertDialog assertAlertDialogShown() {
@Test
public void signOutTester_setsSignInStatusFalse() throws InterruptedException {
firebaseAppDistribution.signOutTester();
awaitTermination(testExecutor);
awaitTermination(lightweightExecutor);
verify(mockSignInStorage).setSignInStatus(false);
}

Expand All @@ -491,12 +492,12 @@ public void updateIfNewReleaseAvailable_receiveProgressUpdateFromUpdateApp()

List<UpdateProgress> progressEvents = new ArrayList<>();
updateTask.addOnProgressListener(FirebaseExecutors.directExecutor(), progressEvents::add);
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

// Clicking the update button.
AlertDialog updateDialog = (AlertDialog) ShadowAlertDialog.getLatestDialog();
updateDialog.getButton(Dialog.BUTTON_POSITIVE).performClick();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

// Update flow
assertEquals(1, progressEvents.size());
Expand Down Expand Up @@ -541,7 +542,7 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears(
when(activity.isChangingConfigurations()).thenReturn(true);

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

// Mimic activity recreation due to a configuration change
firebaseAppDistribution.onActivityDestroyed(activity);
Expand Down Expand Up @@ -576,7 +577,7 @@ public void updateIfNewReleaseAvailable_whenScreenRotates_updateDialogReappears(
when(mockNewReleaseFetcher.checkForNewRelease()).thenReturn(Tasks.forResult(newRelease));

UpdateTask updateTask = firebaseAppDistribution.updateIfNewReleaseAvailable();
awaitAsyncOperations(testExecutor);
awaitAsyncOperations(lightweightExecutor);

// Mimic different activity getting resumed
firebaseAppDistribution.onActivityPaused(activity);
Expand Down