From 3c7b1a5d85c9f02367a451bead5f1d89f1b3f722 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Thu, 6 Aug 2026 16:08:14 +0700 Subject: [PATCH] Cover resource service guard and error paths The resource suite covered happy paths but almost none of the guard/error branches. Adds coverage for: - RemoteResourceNotEnabledException across every guarded entry point (built a disabled-config ResourceService directly). - FileNotFoundException for AddLocalResource / AddExistingRemoteResource. - EntityNotFoundException for DownloadResource with an unknown id. - ArgumentException for uploading a missing / already-uploaded resource. - UploadPendingResources no-op when nothing is pending, and the finally-block durability guarantee: successful uploads persist even when a later upload throws (new ThrowOnUploadCall hook on the mock makes the failure point deterministic regardless of iteration order). - DeleteResource sets DeletedAt to the commit time. - HarmonyResource throws when both local and remote resources are null. Co-Authored-By: Claude Opus 4.8 --- .../ResourceTests/RemoteResourcesTests.cs | 129 ++++++++++++++++++ .../ResourceTests/RemoteServiceMock.cs | 11 ++ 2 files changed, 140 insertions(+) diff --git a/src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs b/src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs index d0573f4..e43a963 100644 --- a/src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs +++ b/src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs @@ -1,6 +1,10 @@ using System.Runtime.CompilerServices; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using SIL.Harmony.Config; +using SIL.Harmony.Db; using SIL.Harmony.Resource; using SIL.Harmony.Sample; @@ -240,6 +244,131 @@ public async Task GetResource_IgnoresDeletedRemoteResourceWhenLocalResourceExist }); } + /// + /// A ResourceService whose config has resources disabled, built directly so the guard in + /// ValidateResourcesSetup can be exercised (the sample kernel always enables resources). + /// + private ResourceService DisabledResourceService() + { + var factory = _services.GetRequiredService(); + var logger = _services.GetRequiredService>>(); + return new ResourceService(factory, Options.Create(new HarmonyConfig()), DataModel, logger); + } + + [Fact] + public async Task GuardedMethods_ThrowWhenResourcesNotEnabled() + { + var service = DisabledResourceService(); + var file = CreateFile("disabled"); + var id = Guid.NewGuid(); + var metadata = new MediaMetadata("disabled.txt", "text/plain", 8); + + await FluentActions.Awaiting(() => service.AddExistingRemoteResource(file, _localClientId, id, "remote-1")) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.AddLocalResource(file, _localClientId)) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.SetResourceMetadata(id, _localClientId, metadata)) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.ListResourcesPendingUpload()) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.ListResourcesPendingDownload()) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.UploadPendingResources(_localClientId, _remoteServiceMock)) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.UploadPendingResource(id, _localClientId, _remoteServiceMock)) + .Should().ThrowAsync(); + await FluentActions.Awaiting(() => service.DownloadResource(id, _remoteServiceMock)) + .Should().ThrowAsync(); + } + + [Fact] + public async Task DownloadResource_ThrowsEntityNotFound_WhenResourceIdUnknown() + { + await FluentActions.Awaiting(() => _resourceService.DownloadResource(Guid.NewGuid(), _remoteServiceMock)) + .Should().ThrowAsync(); + } + + [Fact] + public async Task UploadPendingResourceById_ThrowsArgument_WhenResourceMissing() + { + await FluentActions.Awaiting(() => _resourceService.UploadPendingResource(Guid.NewGuid(), _localClientId, _remoteServiceMock)) + .Should().ThrowAsync().WithMessage("*unable to find local resource*"); + } + + [Fact] + public async Task UploadPendingResource_ThrowsWhenResourceAlreadyUploaded() + { + //AddLocalResource with a remote service both adds and uploads, so the resource is Local + Remote + var uploaded = await _resourceService.AddLocalResource(CreateFile("already-uploaded"), _localClientId, + resourceService: _remoteServiceMock); + uploaded.Local.Should().BeTrue(); + uploaded.Remote.Should().BeTrue(); + + await FluentActions.Awaiting(() => _resourceService.UploadPendingResource(uploaded, _localClientId, _remoteServiceMock)) + .Should().ThrowAsync().WithMessage("*not pending upload*"); + } + + [Fact] + public async Task AddLocalResource_ThrowsFileNotFound_WhenPathMissing() + { + await FluentActions.Awaiting(() => _resourceService.AddLocalResource(Path.GetFullPath("does-not-exist.txt"), _localClientId)) + .Should().ThrowAsync(); + } + + [Fact] + public async Task AddExistingRemoteResource_ThrowsFileNotFound_WhenPathMissing() + { + await FluentActions.Awaiting(() => _resourceService.AddExistingRemoteResource(Path.GetFullPath("missing-remote.txt"), + _localClientId, Guid.NewGuid(), "remote-1")) + .Should().ThrowAsync(); + } + + [Fact] + public async Task UploadPendingResources_IsNoOp_WhenNothingPending() + { + var commitsBefore = DbContext.Commits.Count(); + + await _resourceService.UploadPendingResources(_localClientId, _remoteServiceMock); + + DbContext.Commits.Count().Should().Be(commitsBefore, "no commit should be written when there is nothing to upload"); + } + + [Fact] + public async Task UploadPendingResources_PersistsSuccessfulUploads_WhenALaterUploadThrows() + { + await SetupLocalFile("durability1", "durability1"); + await SetupLocalFile("durability2", "durability2"); + (await _resourceService.ListResourcesPendingUpload()).Should().HaveCount(2); + //fail the second upload; the first must still be persisted by the finally block + _remoteServiceMock.ThrowOnUploadCall(2); + + await FluentActions.Awaiting(() => _resourceService.UploadPendingResources(_localClientId, _remoteServiceMock)) + .Should().ThrowAsync(); + + (await _resourceService.ListResourcesPendingUpload()) + .Should().ContainSingle("the resource uploaded before the failure must not remain pending"); + } + + [Fact] + public async Task DeleteResource_SetsDeletedAtToCommitTime() + { + var (resourceId, _) = await SetupRemoteResource("to-delete"); + + await _resourceService.DeleteResource(_localClientId, resourceId); + + var resource = await DataModel.GetLatest>(resourceId); + resource.Should().NotBeNull(); + var lastCommit = await DbContext.Commits.DefaultOrder().LastAsync(TestContext.Current.CancellationToken); + resource!.DeletedAt.Should().Be(lastCommit.DateTime); + } + + [Fact] + public void HarmonyResource_ThrowsWhenBothResourcesNull() + { + var action = () => new HarmonyResource(null, null); + action.Should().Throw(); + } + [Fact] public void HarmonyResource_ThrowsWhenLocalAndRemoteIdsDoNotMatch() { diff --git a/src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs b/src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs index 7b523e9..5e8bbb7 100644 --- a/src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs +++ b/src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs @@ -30,10 +30,21 @@ public Task DownloadResource(string remoteId, string localResour } private readonly Queue _throwOnUpload = new(); + private int _uploadCallCount; + private int? _throwOnUploadCall; + + /// + /// Throw on the Nth call to (1-based), regardless of which file it is. + /// Lets a multi-resource upload fail deterministically at a known point without depending on iteration order. + /// + public void ThrowOnUploadCall(int callNumber) => _throwOnUploadCall = callNumber; public async Task> UploadResource(Guid resourceId, string localPath, MediaMetadata? metadata = null) { await Task.Yield();//yield back to the scheduler to emulate how exceptions are thrown + _uploadCallCount++; + if (_throwOnUploadCall == _uploadCallCount) + throw new Exception($"Simulated upload failure on call {_uploadCallCount}"); if (_throwOnUpload.TryPeek(out var throwOnUpload)) { if (throwOnUpload == localPath)