Improve readability of a test in ApkUpdaterTest #4518
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
kaibolay
reviewed
Jan 4, 2023
| @@ -202,7 +202,7 @@ public void updateApk_whenSuccessfullyUpdated_notificationsSetCorrectly() | |||
|
|
|||
| UpdateTask updateTask = apkUpdater.updateApk(TEST_RELEASE, true); | |||
| List<UpdateProgress> events = | |||
| awaitProgressEvents(updateTask, 3, mockConnectionLatch::countDown); | |||
| awaitProgressEvents(updateTask, 3, countDownLatch::countDown); | |||
There was a problem hiding this comment.
I think this is difficult to understand. It might make sense to split up the awaitProgressEvents() method to make this more readable. E.g.:
- declare the
progressEventslists here (inline) - add the listener here (inline)
- count down the latch here (inline)
- call the truncated helper function:
awaitProgressEvents(progressEvents, 3)
There was a problem hiding this comment.
Yes I like that a lot better, thanks!
Size Report 1Affected Products
Test Logs |
lfkellogg
force-pushed
the
lk/rename-latch
branch
from
653c10a to
75108e0
Compare
lfkellogg
changed the title
kaibolay
reviewed
Jan 4, 2023
| @@ -104,24 +105,15 @@ static void awaitAsyncOperations(ExecutorService executorService) throws Interru | |||
| /** Await a specified number of progress events on an {@link UpdateTask}. */ | |||
| static List<UpdateProgress> awaitProgressEvents(UpdateTask updateTask, int count) | |||
There was a problem hiding this comment.
Aren't all callers of this prone to the problem that the task might have completed before the listener is added?
I think it might make sense to inline this everywhere (and employ a latch to prevent the race)
There was a problem hiding this comment.
Yeah I think you're right. There's only one other usage and maybe we're just getting lucky that it hasn't been failing. Updated.
lfkellogg
changed the title
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Startup time comparison between the CI merge commit (6a1fa5f) and the base commit (8c83cfc) are not available. No macrobenchmark data found for the base commit (8c83cfc). Analysis for the CI merge commit (6a1fa5f) can be found at: |
kaibolay
reviewed
Jan 4, 2023
| /** Await a specified number of progress events on an {@link UpdateTask}. */ | ||
| static List<UpdateProgress> awaitProgressEvents(UpdateTask updateTask, int count) | ||
| /** Await a specified number of progress events being added to the given collection. */ | ||
| static void awaitProgressEvents(Collection<UpdateProgress> progressEvents, int count) |
There was a problem hiding this comment.
Sorry, but I keep having ideas for this:
I think generalizing this would actually make it more readable. It doesn't need to be full-featured like http://google3/java/com/google/testing/util/WaitForCondition.java
How about:
static void waitForCondition(BooleanSupplier condition) {
ExecutorService executor = TestOnlyExecutors.blocking();
executor.execute(
() -> {
while (!condition.getAsBoolean()) {
try {
Thread.sleep(50);
} catch (InterruptedException e) {
throw new RuntimeException("Interrupted while waiting for progress events", e);
}
}
});
executor.awaitTermination(500, TimeUnit.MILLISECONDS);
assertThat(condition.getAsBoolean());
}
That way the caller becomes more expressive:
waitForCondition(() -> progressEvents.size() == 3);
There was a problem hiding this comment.
No I think this makes more sense now that most of the progress event specific logic is gone. I also removed the Executor since I realized that wasn't actually adding value. It was also inadvertently causing us to wait a minimum of 500ms each time.
kaibolay
approved these changes
Jan 4, 2023
lfkellogg
force-pushed
the
lk/rename-latch
branch
from
bef927c to
385d2cc
Compare
lfkellogg
added a commit
that referenced
this pull request
Jan 5, 2023
* Rename CountDownLatch and inline more of the setup to improve readability. * Use a latch for the other usage of awaitProgressEvents() * Refactor awaitProgressEvents into awaitCondition
kaibolay
pushed a commit
that referenced
this pull request
Jan 23, 2023
* Rename CountDownLatch and inline more of the setup to improve readability. * Use a latch for the other usage of awaitProgressEvents() * Refactor awaitProgressEvents into awaitCondition
lfkellogg
added a commit
that referenced
this pull request
Jan 25, 2023
* Rename CountDownLatch and inline more of the setup to improve readability. * Use a latch for the other usage of awaitProgressEvents() * Refactor awaitProgressEvents into awaitCondition
lfkellogg
added a commit
that referenced
this pull request
Jan 25, 2023
* Rename CountDownLatch and inline more of the setup to improve readability. * Use a latch for the other usage of awaitProgressEvents() * Refactor awaitProgressEvents into awaitCondition
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.