From 5c8dd64acf3286b77a6bc67ea91761a5d5d553f3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 17 Jul 2026 14:18:14 +0200 Subject: [PATCH 1/3] fix(264): record library-level LastScan after a successful local scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Local libraries permanently showed "Never scanned" in the SPA libraries hub regardless of successful scans, even as item counts updated. Root cause: ScanLocalLibraryHandler wrote only the path-level LibraryPath.LastScan (which gates the per-path refresh interval) and never the library-level Library.LastScan. The read API (GetAllMediaSourcesForApiHandler) populates the hub's scan-time badge from Library.LastScan, so that value stayed null forever. The three remote scanners (Jellyfin/Emby/Plex) already set the library-level value; only the local scanner did not. Both sides predate #202 — the SPA hub merely made the missing value visible. Mirror the remote scanners' semantics: record the library-level scan time only when the scan actually ran and every path that ran succeeded, so a skipped (unforced, interval not elapsed) or partially-failed scan does not claim a successful scan time. Also de-BOM the touched handler per the fix-as-you-touch charset gate (#311). fixes #264 --- .../ScanLocalLibraryHandlerTests.cs | 173 ++++++++++++++++++ .../Commands/ScanLocalLibraryHandler.cs | 16 +- 2 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs diff --git a/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs b/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs new file mode 100644 index 000000000..f702f75bd --- /dev/null +++ b/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs @@ -0,0 +1,173 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Scanner.Application.MediaSources; +using ErsatzTV.Scanner.Core.Interfaces; +using ErsatzTV.Scanner.Core.Interfaces.Metadata; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Scanner.Tests.Application.MediaSources; + +public class ScanLocalLibraryHandlerTests +{ + [TestFixture] + public class Handle + { + [SetUp] + public void SetUp() + { + // the handler validates that the configured ffmpeg/ffprobe paths exist on disk + _ffmpegPath = Path.GetTempFileName(); + _ffprobePath = Path.GetTempFileName(); + + _scannerProxy = Substitute.For(); + _libraryRepository = Substitute.For(); + _configElementRepository = Substitute.For(); + _movieFolderScanner = Substitute.For(); + + _library = new LocalLibrary + { + Id = 42, + Name = "Movies", + MediaKind = LibraryMediaKind.Movies, + MediaSourceId = 1, + Paths = [new LibraryPath { Id = 1, Path = "/movies" }] + }; + + _libraryRepository.GetLibrary(_library.Id).Returns(Some(_library).AsTask()); + + ConfigValue(ConfigElementKey.FFmpegPath, _ffmpegPath); + ConfigValue(ConfigElementKey.FFprobePath, _ffprobePath); + ConfigValue(ConfigElementKey.LibraryRefreshInterval, 0); + } + + [TearDown] + public void TearDown() + { + File.Delete(_ffmpegPath); + File.Delete(_ffprobePath); + } + + private string _ffmpegPath; + private string _ffprobePath; + private IScannerProxy _scannerProxy; + private ILibraryRepository _libraryRepository; + private IConfigElementRepository _configElementRepository; + private IMovieFolderScanner _movieFolderScanner; + private LocalLibrary _library; + + [Test] + public async Task Should_Set_Library_LastScan_After_Successful_Scan() + { + ScanResult(Right(Unit.Default)); + + Either result = await Handler().Handle( + new ScanLocalLibrary("http://ersatztv.example", _library.Id, true), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + // the regression: only the path-level LastScan was written, so the API's + // library-level scan time stayed null and the SPA showed "Never scanned" forever + _library.LastScan.ShouldNotBeNull(); + await _libraryRepository.Received(1).UpdateLastScan(_library); + await _libraryRepository.Received(1).UpdateLastScan(_library.Paths[0]); + } + + [Test] + public async Task Should_Not_Set_Library_LastScan_When_A_Path_Scan_Fails() + { + ScanResult(Left(new BaseError("scan failed"))); + + await Handler().Handle( + new ScanLocalLibrary("http://ersatztv.example", _library.Id, true), + CancellationToken.None); + + _library.LastScan.ShouldBeNull(); + await _libraryRepository.DidNotReceive().UpdateLastScan(Arg.Any()); + } + + [Test] + public async Task Should_Not_Set_Library_LastScan_When_A_Later_Path_Fails() + { + var goodPath = new LibraryPath { Id = 1, Path = "/movies" }; + var badPath = new LibraryPath { Id = 2, Path = "/more-movies" }; + _library.Paths = [goodPath, badPath]; + + _movieFolderScanner.ScanFolder( + goodPath, + _ffmpegPath, + _ffprobePath, + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(Right(Unit.Default).AsTask()); + _movieFolderScanner.ScanFolder( + badPath, + _ffmpegPath, + _ffprobePath, + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(Left(new BaseError("scan failed")).AsTask()); + + await Handler().Handle( + new ScanLocalLibrary("http://ersatztv.example", _library.Id, true), + CancellationToken.None); + + // a partially-scanned library must not claim a successful scan time + _library.LastScan.ShouldBeNull(); + await _libraryRepository.DidNotReceive().UpdateLastScan(Arg.Any()); + await _libraryRepository.Received(1).UpdateLastScan(goodPath); + } + + [Test] + public async Task Should_Not_Set_Library_LastScan_When_Scan_Is_Skipped() + { + ScanResult(Right(Unit.Default)); + _library.Paths[0].LastScan = DateTime.UtcNow; + ConfigValue(ConfigElementKey.LibraryRefreshInterval, 6); + + await Handler().Handle( + new ScanLocalLibrary("http://ersatztv.example", _library.Id, false), + CancellationToken.None); + + // nothing was scanned, so there is no new scan time to record + _library.LastScan.ShouldBeNull(); + await _libraryRepository.DidNotReceive().UpdateLastScan(Arg.Any()); + } + + private void ConfigValue(ConfigElementKey key, T value) => + _configElementRepository.GetValue( + Arg.Is(k => k.Key == key.Key), + Arg.Any()) + .Returns(Task.FromResult>(Some(value))); + + private void ScanResult(Either result) => + _movieFolderScanner.ScanFolder( + Arg.Any(), + _ffmpegPath, + _ffprobePath, + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(result.AsTask()); + + private ScanLocalLibraryHandler Handler() => + new( + _scannerProxy, + _libraryRepository, + _configElementRepository, + _movieFolderScanner, + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For(), + Substitute.For>()); + } +} diff --git a/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs b/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs index e2ec05a77..f24a68818 100644 --- a/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs +++ b/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs @@ -1,4 +1,4 @@ -using System.Diagnostics; +using System.Diagnostics; using ErsatzTV.Core; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Interfaces.Repositories; @@ -65,6 +65,7 @@ public class ScanLocalLibraryHandler : IRequestHandler Date: Fri, 17 Jul 2026 14:27:50 +0200 Subject: [PATCH 2/3] fix(264): log local path scan errors (review finding) + map Scanner.Tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review (MERGEABLE, no blockers) raised two items worth folding in rather than deferring: Medium — ScanLocalLibraryHandler silently swallowed path scan errors, while the three remote scanners it mirrors all log result.LeftToSeq(). That mattered less when a failed path only skipped the path-level LastScan, but the previous commit makes a failed path suppress the library-level scan time too — so the user would see exactly the #264 symptom ("Never scanned") with nothing in the log explaining why. That is a diagnosis dead-end of the same class as the bug being fixed, so log it here rather than file a follow-up. No test: the sibling Synchronize*LibraryByIdHandlerTests don't assert on logging either, and LogError is an extension method that NSubstitute can't cleanly verify. Low — docs/testing.md bills itself as the authoritative map of what each test project covers but omitted ErsatzTV.Scanner.Tests entirely (1471 pre-existing tests). This PR adds a file to that project, so add the row and include it in the per-PR verification gate. --- .../MediaSources/Commands/ScanLocalLibraryHandler.cs | 11 +++++++++++ docs/testing.md | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs b/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs index f24a68818..0ce27ca8c 100644 --- a/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs +++ b/ErsatzTV.Scanner/Application/MediaSources/Commands/ScanLocalLibraryHandler.cs @@ -149,6 +149,17 @@ public class ScanLocalLibraryHandler : IRequestHandler Date: Fri, 17 Jul 2026 14:35:09 +0200 Subject: [PATCH 3/3] test(264): pin the path-scan-error log; correct testing.md accuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the previous fix commit (986ccfaf, verdict MERGEABLE) raised one fair hit and one claim that did not survive checking. Fair hit — that commit's message asserted "LogError is an extension method that NSubstitute can't cleanly verify" as the reason for adding no test. That is FALSE, and this repo disproves it: ErsatzTV.Tests' ReleaseMemoryHandlerTests.ShouldHaveLogged asserts on ILogger via ReceivedCalls(), which works precisely because the LogError extension dispatches to the substituted ILogger.Log. Reusing that idiom here costs 4 lines, so pin the log the previous commit added. Verified non-vacuous: asserting a message the handler never logs fails the test. Not applied — the same review called the row's "1471 tests" misleading on the grounds that TranscodingTests is [Explicit] and contributes most of that count. TranscodingTests is indeed [Explicit], but filtering it out yields exactly 1471, so 1471 is already the runnable count and the row was accurate. Kept the number; documented TranscodingTests as [Explicit]/opt-in instead, since it was a genuine omission from a table that claims to be authoritative. Also rewrap the verification-gate paragraph the previous commit left over-long, and name the two always-run projects instead of "both". Deferred (filed separately): a canceled local scan now logs at ERROR per path via this log. It mirrors the remote handlers exactly, so diverging here would be the inconsistency, not the fix. --- .../MediaSources/ScanLocalLibraryHandlerTests.cs | 15 ++++++++++++++- docs/testing.md | 10 +++++----- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs b/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs index f702f75bd..f568166b1 100644 --- a/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs +++ b/ErsatzTV.Scanner.Tests/Application/MediaSources/ScanLocalLibraryHandlerTests.cs @@ -27,6 +27,7 @@ public class ScanLocalLibraryHandlerTests _libraryRepository = Substitute.For(); _configElementRepository = Substitute.For(); _movieFolderScanner = Substitute.For(); + _logger = Substitute.For>(); _library = new LocalLibrary { @@ -57,6 +58,7 @@ public class ScanLocalLibraryHandlerTests private ILibraryRepository _libraryRepository; private IConfigElementRepository _configElementRepository; private IMovieFolderScanner _movieFolderScanner; + private ILogger _logger; private LocalLibrary _library; [Test] @@ -88,6 +90,10 @@ public class ScanLocalLibraryHandlerTests _library.LastScan.ShouldBeNull(); await _libraryRepository.DidNotReceive().UpdateLastScan(Arg.Any()); + + // a failed path suppresses the library-level scan time, reproducing the #264 symptom; + // without this log the user would have nothing explaining why + ShouldHaveLogged("Error scanning local library path /movies: scan failed"); } [Test] @@ -168,6 +174,13 @@ public class ScanLocalLibraryHandlerTests Substitute.For(), Substitute.For(), Substitute.For(), - Substitute.For>()); + _logger); + + // asserts on the substituted ILogger.Log call the LogError extension dispatches to + // (same idiom as ErsatzTV.Tests' ReleaseMemoryHandlerTests.ShouldHaveLogged) + private void ShouldHaveLogged(string expectedMessage) => + _logger.ReceivedCalls() + .Any(call => call.GetArguments().ElementAtOrDefault(2)?.ToString() == expectedMessage) + .ShouldBeTrue(); } } diff --git a/docs/testing.md b/docs/testing.md index 8d2b51732..374afc96f 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -9,7 +9,7 @@ before adding tests, not just `docs/contributing.md` §8 (which now just points |---|---|---| | `ErsatzTV.Tests` | API controllers + MediatR handlers | In-memory SQLite fixture: a shared `SqliteConnection("Data Source=:memory:;Foreign Keys=False")` kept open + `EnsureCreatedAsync()` (**not** full migration replay) + `PRAGMA foreign_keys=OFF`, then seed; a tiny `IDbContextFactory` wraps `new TvContext(...)`. 828 tests currently. | | `ErsatzTV.Core.Tests` | Domain logic, scheduling, IPTV/XMLTV generation | References `ErsatzTV.Application` directly — there is no separate `Application.Tests` project. 542 tests + 1 skipped under `TZ=UTC` (the Block playout golden additionally skips under a non-UTC `TZ`; see Golden-file nets). | -| `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. 1471 tests. | +| `ErsatzTV.Scanner.Tests` | Library scanning: scan handlers, folder scanners, NFO readers | Handler tests substitute the folder scanners + `ILibraryRepository` and assert the resulting repository writes (e.g. `ScanLocalLibraryHandlerTests` pins which `LastScan` levels a scan records — ersatztv#264). Fakes/`Testably` back the file-system-facing scanners. 1471 tests. Additionally contains `Core/FFmpeg/TranscodingTests` — `[Explicit]` + `[Combinatorial]`, so it never runs in CI or a plain `dotnet test` (it needs real ffmpeg/hardware) and contributes 0 to that count; run it by name when touching the transcoding pipeline. | | `ErsatzTV.Architecture.Tests` | Layering rules via NetArchTest.eNhancedEdition | Core↛Infra/App/EF; FFmpeg↛all; App↛concrete providers. 5 tests. See `docs/contributing.md` §1. | | `ErsatzTV.FFmpeg.Tests` | FFmpeg command construction | Build a pipeline, assert the exact rendered arg string (`PipelineBuilderBaseTests.cs`). | | `web/` (vitest) | React SPA unit tests | 330 tests; run alongside typecheck + build (see below). | @@ -86,10 +86,10 @@ npm run build # tsc -b && vite build ## Per-PR verification gate -Before opening a PR: build the solution, run both .NET test projects (plus -`ErsatzTV.Scanner.Tests`, `ErsatzTV.Architecture.Tests` and `ErsatzTV.FFmpeg.Tests` if touched), and run the web test/lint/ -typecheck/build steps above. All must be green. A golden-file diff or an architecture-test -failure is a hard stop — fix the code, don't regenerate/relax the test. +Before opening a PR: build the solution, run `ErsatzTV.Tests` + `ErsatzTV.Core.Tests` (plus +`ErsatzTV.Scanner.Tests`, `ErsatzTV.Architecture.Tests` and `ErsatzTV.FFmpeg.Tests` if touched), +and run the web test/lint/typecheck/build steps above. All must be green. A golden-file diff or +an architecture-test failure is a hard stop — fix the code, don't regenerate/relax the test. ## See also -- 2.47.3