Strengthen the test suite: 298 → 365 tests - #72
Merged
Conversation
Reviewed the whole suite for coverage gaps, redundancy, brittleness, and maintainability. The suite was already in good shape (fast, deterministic, no external dependencies), so this keeps those properties -- still ~3s -- and targets the concentrated weaknesses. Coverage gaps closed. Several surfaces were unreachable rather than merely untested, mostly for want of a small seam the codebase already uses elsewhere (clipLoader, backgroundYield, savePathPicker): - ClipExporter.ExportAsync had zero tests. An optional runFfmpeg delegate lets its failure and script-cleanup paths run without spawning ffmpeg. - PackageManager had no test file at all; ExtractFFmpegBin now has one. - Nine of thirteen converters were untested, including their null and wrong-type branches. - VideoPlayerController's transport commands were never called by any test. - MainWindowViewModel's scan empty/error states and keyboard transport had no coverage, and deleting the clip that is actually playing deadlocked on Dispatcher.Invoke, so RunOnUiThread got a uiInvoker seam. - Mp4DurationReader's box-parsing branches. - New CultureInvarianceTests pins the four load-bearing InvariantCulture arguments. Dropping one silently returns zero clips under ar-SA or mis-dates every clip by 543 years under th-TH, and no existing test could see it, because every test runs under the machine's own culture. Fixture and reliability work: - TestClipFiles wrote 60s chunks spaced 60s apart, which made probed duration and nominal spacing indistinguishable in every timeline calculation. It now takes per-chunk durations. - FakeClipMediaSourceBuilder exposed raw Lists that tests read while Build() appended off-thread; the bookkeeping is now private behind its lock. - FfconcatMediaSourceBuilderTests no longer leaks a playlist file per test into %TEMP%, and CamStorageTests no longer scans the build output directory. Removed or merged the tests that duplicated a sibling's code path or asserted framework behavior, and fixed several that passed for the wrong reason -- notably two ClipPlaylist tests whose central assertions already held before the act ran. The three production bug fixes this review turned up, and the CI changes, are in a follow-up PR so this one stays test-only. The three seams above are the exception: the tests cannot compile without them, and each defaults to the existing behavior byte-for-byte.
danielchalmers
force-pushed
the
test-suite-improvements
branch
from
August 1, 2026 21:49
aa3b15e to
efd5739
Compare
Merged
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.
A review pass over the whole suite for coverage gaps, redundancy, brittleness, and maintainability. Test-only — the three production bugs this review turned up, and the CI changes, are in #73 on top of this.
The suite was already in good shape — fast, deterministic, no external dependencies, thoughtful fixtures — so this preserves those properties and targets the weaknesses that were actually concentrated somewhere. 365 tests, still ~3s, green in Debug and Release, stable over repeated runs.
Coverage gaps closed
Several surfaces weren't merely untested, they were unreachable — mostly for want of a small seam the codebase already uses elsewhere (
clipLoader,backgroundYield,savePathPicker).ClipExporter.ExportAsyncrunFfmpegdelegate; the not-installed and partial-output-cleanup paths coveredPackageManagerExtractFFmpegBincoveredConverterParameterbranchesVideoPlayerControllertransportPlay/Next/Previous/GoToClipnever calledDispatcher.Invoke, so all 5 delete tests used a null controlleruiInvokerseam; the real path is now testedclipLoaderwas enough)Mp4DurationReaderbox parsingCultureInvarianceTestsThat last one is worth calling out: every test runs under the machine's own culture, where invariant and current formats coincide, so no test could detect a dropped
InvariantCultureargument. Dropping one silently returns zero clips forar-SAusers (UmAlQura rejects the filename outright) or mis-dates every clip by 543 years underth-TH.Fixtures and reliability
TestClipFileswrote uniform 60s chunks spaced 60s apart. Probed duration and nominal spacing were therefore indistinguishable in every timeline calculation — the running sum, the tail computation, and gap detection were mutually interchangeable. It now takes per-chunk durations, so chunk-offset arithmetic is observable.FakeClipMediaSourceBuilderexposed rawLists that tests indexed whileBuild()appended from a pool thread. The bookkeeping is now private behind its existing lock and handed out as snapshots.FfconcatMediaSourceBuilderTestsno longer leaks a playlist file per test into%TEMP%, andCamStorageTestsno longer recursively scans the build output directory.Removed, merged, and fixed
Tests removed or merged for duplicating a sibling's exact code path, and one that asserted
System.Text.Json's default behavior rather than ours. Several others passed for the wrong reason and were fixed — notably twoClipPlaylisttests whose central assertions already held before the act ran (MoveTo(0)on an empty playlist is a no-op, soCurrentIndex.ShouldBe(-1)was asserting the constructor).The three seams
This PR is test-only with one deliberate exception:
ClipExportergains an optionalrunFfmpegdelegate,MainWindowViewModelgains an optionaluiInvoker, andPackageManager.ExtractFFmpegBingoesprivate→internal. The tests literally cannot compile without them, and each defaults to the existing behavior byte-for-byte, so the app is unchanged.Deliberately deferred
CamEvent's two JSON paths disagree on aZ-suffixed timestamp — strict yieldsKind=Utc, lenient yields host-local, and which path runs depends on an unrelated field. Latent (Tesla doesn't emit offsets today) but real.ClipTimeline.GetPosition/ToAbsolutePosition/GetChunkandCamClip.Summaryhave no production callers — only tests keep them alive.MainWindowViewModelTests.csandVideoPlayerControllerTests.cs— better as a pure-movement change that doesn't collide with these additions..ffconcatfiles still leak into%TEMP%per run from two other test files. The durable fix is an internalPlaylistDirectoryoverride; the app never prunes that directory either.