refactor(github): generalize property fetching, fix cancellation swallowing - #35
Merged
Merged
Conversation
…te cancellation-test comment code-review on PR #35 found four issues: - fetchPlayStoreAnnouncement() used .mapCatching { it.toPlayStoreAnnouncementRemoteConfig() }, which catches Throwable, not just Exception — silently turning a real bug or an OutOfMemoryError in that pure mapping into an ordinary Result.failure. The pre-refactor code built the config inside a catch (e: Exception) branch, so an Error would have propagated as intended. Switched to .map (non-catching), matching that behavior. - The cancellation test's own comment claimed cancelling before the call starts 'exercises exactly' the catch (e: CancellationException) { throw e } inside fetchRawProperties — it doesn't: withContext's pre-flight ensureActive() throws before the function body runs, same issue found on PR #28's JellyfinApiService tests. Unlike that OkHttp case, HttpURLConnection has no interceptor seam to throw an exception mid-call, so there's currently no way to write a test that actually reaches this catch. Reworded the comment to say so plainly instead of claiming coverage it doesn't have. - A third finding (HttpURLConnection is fundamentally not cancellable; the real fix is migrating to the app's injectable OkHttpClient) was already disclosed in this PR's own KDoc as an accepted, deferred limitation — left as-is, not re-litigated. Verified: compiles, full JVM suite green (398 tests, same 5 pre-existing baseline failures as master, none new).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Staged internally first (was chore/p9-generic-properties-fetch on PixelPlayerHQ#2828, closed — see that PR's closing comment). Will be proposed upstream again once the downloads feature it's part of is further along.