Skip to content
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.

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

Address flaky tests by updating episode manager coroutine handling for archiveAllFilesInList #1334

Closed

Conversation

mchowning
Copy link
Contributor

Description

This is fixing some recent test flakiness we've seen in the AutoArchiveTest class due to the recent change that started launching a coroutine from the EpisodeManagerImpl::archiveAllInList method (#1327).

Warning
This PR is currently targetting the release/7.46 branch.

I've been wanting to convert more of our manager classes to use properly suspending functions instead of having regular functions that launch coroutines (which then call other regular functions that launch new coroutines, etc.). Making this kind of change will make help us avoid hard to reproduce/fix race conditions due to the order the coroutines run.

The recent test flakiness we had due to the archiveAllInList method change seemed like another example of how this is hurting us, so I'm taking this opportunity to do this as a bit of a small POC/first-step toward the changes I would like to make. In particular, this PR focuses on making the archiveAllInList method a proper suspended function and, conveniently enough, the tests don't seem to be flaky anymore (I'm scared that me saying that is basically guaranteeing that they will fail the next time CI runs 🤞 ).

I intentionally limited the changes to the minimum ones necessary to make archiveAllInList a proper suspended function, which is why everywhere else I just changed the new suspend function calls to be called in a runBlocking {...} block. If I were doing this as a broader refactor, I would remove those as well, but since we might want to merge this change to a release branch I didn't want to do that in this PR.

I've got this currently targetng the release/7.46 branch since that is where we have the test flakiness, but I think I'm probably leaning toward just merging this to main instead to minimize the risk of breakages on the release branch. In fact, I'm kind of thinking we should perhaps revert https://github.com/Automattic/pocket-casts-android/pull/1327 on the release/7.46 branch, and merge that, along with these changes, to main. That way, if everything goes according to plan, neither branch will be flaky.

Anyway, take a look, and let me know what you think both about this general approach (since I'm currently planning to do more of this next week), and about how we should approach the flakiness on release/7.46.

Testing Instructions

  1. Go to a podcast screen
  2. Tap the three dot overflow menu
  3. Tap archive all
  4. Confirm you want to archive all
  5. Observe that all the episodes are archived.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@ashiagr
Copy link
Contributor

ashiagr commented Sep 4, 2023

I'm including these to merge/release-7.46-to-main so that our main branch is not affected by tests flakiness. Also, they will be included in the next beta release rather than a production one to minimize the risk.

@ashiagr ashiagr closed this Sep 4, 2023
@MiSikora MiSikora deleted the update/episode-manager-coroutine-handling branch April 12, 2024 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants