Test suite - #85
Merged
Merged
Conversation
Nothing ran this package's tests. bcc-media-app's Semaphore `Test` block fires when `/submodules/` changes but only runs that app's own suite, so the single Dart test here and the six Robolectric tests were unenforced. Mirrors the sibling workflow in bccm_flutter (pub get -> analyze -> test, Flutter 3.44.0 pinned). Uses `--no-fatal-infos` because the package carries ~20 pre-existing deprecation infos; warnings and errors are still fatal, which required dropping one redundant null assertion in player_controller. Also adds the shared test infrastructure the following commits build on: a hand-written FakeBccmPlayerInterface (better than the mockito mocks where a test needs real state management, since `stateNotifier` is a field on the abstract class) and pigeon model builders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
handlePlaybackEnded took a MediaItem and ignored it, so it never pushed onto history — while skipToNext did. The result was that skipping forward manually let you skip back, but letting a track finish did not. Falls back to the current media item when the native side passes null, and only records when there is actually a next item to move to, matching skipToNext. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Test suite + CI, and the bugs it found
bccm_playerhad one Dart test, and nothing in CI ran it. bcc-media-app's SemaphoreTestblock fires on/submodules/changes but only runs that app's own suite — so regressions here reached BCC Media, Bible Kids, Play and connect-live unchallenged.This adds 215 tests and a GitHub Actions workflow (pub get → analyze → test), and fixes the 11 bugs the tests turned up.
Coverage targets where native hands off to Dart, plus the queue — which
doc/contributing/audio-support-plan.mdstep 3 wants covered before it moves native. Queue tests are written against theQueueManagercontract rather than the internals, so they survive that move.Bugs fixed
Each has its own commit, paired with the test that proves it.
User-visible
seekableRangeStartMs, the drag handler didn't — on a 100s–200s DVR window the thumb sat at halfway while dragging there jumped to the window start.ShuffleQueueList._orderedItemswas only ever written bysetItems, so toggling shuffle resurrected consumed items.handlePlaybackEndedignored its argument, so you could skip back after skipping forward but not after letting a track finish.moveQueueItemthrewRangeErroron stale indices — trivially hit by a reorderable list racing a queue update.Silent
OneAsyncAtATimecalledcompleteErroron aCompleternothing awaits, anduseTimelinecallsreset()on dispose. Surfaced in error reporting as a baredisposedstring with no useful stack.positionMs ?? 0 / 500parses aspositionMs ?? (0 / 500), so the intended 500 ms throttle never happened.playerEventStream— the only one of seven callbacks that didn't publish, socontroller.eventsnever saw a cast handover.PlayerStateNotifier.dispose(force: true)double-disposed and threw.onDisposere-enters it.getFormattedDurationthrew on NaN, which would take the controls overlay down. Unreachable today only becausesafeDoubleguards upstream.setNextUpmutated the caller's list.iconColorwas unreachable whenprimaryColorwas set.fillWithDefaultsreadprimaryColor ?? iconColor ?? defaults; nowiconColor ?? primaryColor ?? defaults. Anyone setting both now gets theiconColorthey asked for.Also in here
flutter analyzeruns strict (infos fatal). That meant clearing 25 deprecations —withOpacity,Color.value,cacheExtent,whereNotNull,onReorder. One narrow inline exception:dart:htmlin the web player.doc/contributing/basics.mdgained a "Running tests" section, including the previously undocumented Gradle command for the existing Kotlin tests.BccmPlayerControllerturned up no bugs: theinitializememoisation, the disposed-mid-flight window andswapPlayerNotifier's listener swap all hold.Follow-ups, not in this PR
dart:htmltopackage:web. ~120 lines of DOM code that only a real web build can verify — the last thing between us and an unqualified strict analyze._SettingsBottomSheet. Pure logic trapped in a widget; the biggest remaining coverage win.tv_controls.dartignores the seekable range entirely, reimplementinguseTimelineinline. DVR-unaware on TV rather than broken.playerProviderFor/primaryPlayerProviderleak aPlayerStateNotifier(with a live 1 s timer) per rebuild.