Share repository test fixture plumbing#74
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShared test-fixture and fake refactor: RepositoryTestFixture gained specialized factories, async dispose with cache reset, expanded helpers; FakeInMemoryBlobContainerService moved/made public; many tests updated to use the fixture and adjusted using directives; generated benchmark report artifacts added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs (1)
86-92:⚠️ Potential issue | 🟠 MajorDispose
ChunkIndexServicebefore cache cleanup.At Line 91, cache deletion can run while
_indexis still alive. Dispose_indexfirst to avoid locked-file teardown failures.💡 Suggested fix
public void Dispose() { + _index.Dispose(); + if (Directory.Exists(_rootDirectory)) Directory.Delete(_rootDirectory, recursive: true); RepositoryTestFixture.ResetLocalCacheAsync(AccountName, _containerName).GetAwaiter().GetResult(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs` around lines 86 - 92, In Dispose(), ensure the ChunkIndexService instance (_index) is disposed before deleting on-disk files and before calling RepositoryTestFixture.ResetLocalCacheAsync; call _index?.Dispose() (and optionally set _index = null) prior to Directory.Delete(_rootDirectory, ...) and the ResetLocalCacheAsync(AccountName, _containerName) call to avoid locked-file teardown failures.
🧹 Nitpick comments (2)
src/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs (1)
13-13: Preferinternalvisibility for this test double.Making this type public broadens the assembly surface unnecessarily. Keep it
internaland useInternalsVisibleTofor consuming test assemblies if needed.As per coding guidelines: `src/**/*.cs`: Make non-test classes `internal`. Only make them `public` when they must be consumed by another non-test assembly. For test access, prefer InternalsVisibleTo.💡 Suggested change
-public sealed class FakeInMemoryBlobContainerService : IBlobContainerService +internal sealed class FakeInMemoryBlobContainerService : IBlobContainerService🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs` at line 13, Change the test double's visibility from public to internal: update the FakeInMemoryBlobContainerService declaration (which implements IBlobContainerService) to be internal instead of public; if other test projects need to access it, add the consuming test assembly to InternalsVisibleTo in the assembly info (or the csproj) so the internal type remains accessible to tests.src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs (1)
142-147: Normalize timestamp inputs to UTC before applying file metadata.The helper accepts arbitrary
DateTimekinds but writes UTC metadata; normalizing first avoids timezone-dependent test behavior.♻️ Proposed refactor
public string WriteFile(string relativePath, byte[] content, DateTime created, DateTime modified) { var full = WriteFile(relativePath, content); - File.SetCreationTimeUtc(full, created); - File.SetLastWriteTimeUtc(full, modified); + var createdUtc = created.Kind == DateTimeKind.Utc ? created : created.ToUniversalTime(); + var modifiedUtc = modified.Kind == DateTimeKind.Utc ? modified : modified.ToUniversalTime(); + File.SetCreationTimeUtc(full, createdUtc); + File.SetLastWriteTimeUtc(full, modifiedUtc); return full; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` around lines 142 - 147, The WriteFile overload currently takes DateTime created/modified but writes them with File.SetCreationTimeUtc/File.SetLastWriteTimeUtc without ensuring the inputs are UTC; update the method (WriteFile) to normalize both created and modified to UTC (e.g., created = created.ToUniversalTime(); modified = modified.ToUniversalTime();) before calling File.SetCreationTimeUtc and File.SetLastWriteTimeUtc so tests don't vary by DateTime.Kind or local timezone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 173-180: The DisposeAsync method currently calls
ResetLocalCacheAsync and if that throws the subsequent temp-root deletion
(_deleteTempRoot on _tempRoot) is skipped; wrap the cache reset in a try/finally
so the temp-root cleanup always runs: call Index.Dispose(), then try { await
ResetLocalCacheAsync(_account, _container); } finally { if
(Directory.Exists(_tempRoot)) _deleteTempRoot(_tempRoot); } so
_deleteTempRoot/_tempRoot cleanup is executed even if ResetLocalCacheAsync
fails.
---
Outside diff comments:
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs`:
- Around line 86-92: In Dispose(), ensure the ChunkIndexService instance
(_index) is disposed before deleting on-disk files and before calling
RepositoryTestFixture.ResetLocalCacheAsync; call _index?.Dispose() (and
optionally set _index = null) prior to Directory.Delete(_rootDirectory, ...) and
the ResetLocalCacheAsync(AccountName, _containerName) call to avoid locked-file
teardown failures.
---
Nitpick comments:
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 142-147: The WriteFile overload currently takes DateTime
created/modified but writes them with
File.SetCreationTimeUtc/File.SetLastWriteTimeUtc without ensuring the inputs are
UTC; update the method (WriteFile) to normalize both created and modified to UTC
(e.g., created = created.ToUniversalTime(); modified =
modified.ToUniversalTime();) before calling File.SetCreationTimeUtc and
File.SetLastWriteTimeUtc so tests don't vary by DateTime.Kind or local timezone.
In `@src/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs`:
- Line 13: Change the test double's visibility from public to internal: update
the FakeInMemoryBlobContainerService declaration (which implements
IBlobContainerService) to be internal instead of public; if other test projects
need to access it, add the consuming test assembly to InternalsVisibleTo in the
assembly info (or the csproj) so the internal type remains accessible to tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f838c9e-26b7-47a9-a4df-ea3dbd0085dd
📒 Files selected for processing (9)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceReadTests.cssrc/Arius.Core.Tests/Shared/ChunkStorage/ChunkStorageServiceUploadTests.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/SlowDownloadBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Arius.Benchmarks/raw/20260428T144526.126Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md`:
- Around line 13-15: The generated GitHub benchmark table in
src/Arius.Benchmarks/raw/.../results/*-report-github.md is triggering
markdownlint MD055/MD056 each regen; either update the generator that emits the
table (the report-github.md output) to produce a normalized Markdown table
(ensure the header separator row has the correct number of columns, consistent
pipe alignment and no stray/escaped pipe characters) or add an exclusion for
these generated files in your markdownlint config (disable MD055 and MD056 for
the src/Arius.Benchmarks/raw/**/results/*-report-github.md pattern via
.markdownlint.json/.markdownlint.yml) so the CI linter ignores auto-generated
reports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95026ac1-049f-45a1-8981-05e5d2890ff9
⛔ Files ignored due to path filters (3)
src/Arius.Benchmarks/raw/20260428T144526.126Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-144526.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T144526.126Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T144526.126Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
src/Arius.Benchmarks/benchmark-tail.mdsrc/Arius.Benchmarks/raw/20260428T144526.126Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T144526.126Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
✅ Files skipped from review due to trivial changes (2)
- src/Arius.Benchmarks/benchmark-tail.md
- src/Arius.Benchmarks/raw/20260428T144526.126Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.html
…hub.com/woutervanranst/Arius7 into feat/refactor-arius-core-tests-fixtures
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs (1)
189-195:⚠️ Potential issue | 🟠 MajorDisposeAsync bypasses lease-aware cache lifecycle and does not guarantee temp-root cleanup on reset failure.
At Line 192, cache reset runs unconditionally inside
RepositoryTestFixture.DisposeAsync. This bypasses the lease/preserve gating insrc/Arius.E2E.Tests/Fixtures/E2EFixture.cs(ShouldResetCacheOnDispose) and can delete shared cache while another fixture is still live. Also, if reset throws, Line 195 temp-root cleanup is skipped.🔧 Proposed fix
public sealed class RepositoryTestFixture : IAsyncDisposable { + private readonly bool _resetCacheOnDispose; + public RepositoryTestFixture( IBlobContainerService blobContainer, IEncryptionService encryption, ChunkIndexService index, IChunkStorageService chunkStorage, FileTreeService fileTreeService, SnapshotService snapshot, string tempRoot, string localRoot, string restoreRoot, string account, string containerName, - Action<string>? deleteTempRoot = null) + Action<string>? deleteTempRoot = null, + bool resetCacheOnDispose = true) { BlobContainer = blobContainer; Encryption = encryption; Index = index; @@ _account = account; _container = containerName; _deleteTempRoot = deleteTempRoot ?? (path => Directory.Delete(path, recursive: true)); + _resetCacheOnDispose = resetCacheOnDispose; _mediator = Substitute.For<IMediator>(); } public async ValueTask DisposeAsync() { Index.Dispose(); - await ResetLocalCacheAsync(_account, _container); - - if (Directory.Exists(_tempRoot)) - _deleteTempRoot(_tempRoot); + try + { + if (_resetCacheOnDispose) + await ResetLocalCacheAsync(_account, _container); + } + finally + { + if (Directory.Exists(_tempRoot)) + _deleteTempRoot(_tempRoot); + } } }Then call the shared fixture from E2E with cache-reset-on-dispose disabled, so E2E lease logic remains the single owner of cache reset decisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` around lines 189 - 195, DisposeAsync currently calls ResetLocalCacheAsync unconditionally and skips temp-root deletion if reset throws; change DisposeAsync (the method in RepositoryTestFixture) to honor a configurable flag/lease gate (e.g., add or check an existing boolean like cacheResetOnDispose or preserveLease) so it only calls ResetLocalCacheAsync(_account, _container) when allowed, and ensure temp-root cleanup always runs by moving Directory.Exists/_deleteTempRoot(_tempRoot) into a finally block (or catching exceptions from ResetLocalCacheAsync and still running _deleteTempRoot), also preserve the existing Index.Dispose() call; finally, when using this shared fixture from E2EFixture, instantiate or call it with cache-reset-on-dispose disabled so E2E's lease logic remains the single owner of cache reset decisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 189-195: DisposeAsync currently calls ResetLocalCacheAsync
unconditionally and skips temp-root deletion if reset throws; change
DisposeAsync (the method in RepositoryTestFixture) to honor a configurable
flag/lease gate (e.g., add or check an existing boolean like cacheResetOnDispose
or preserveLease) so it only calls ResetLocalCacheAsync(_account, _container)
when allowed, and ensure temp-root cleanup always runs by moving
Directory.Exists/_deleteTempRoot(_tempRoot) into a finally block (or catching
exceptions from ResetLocalCacheAsync and still running _deleteTempRoot), also
preserve the existing Index.Dispose() call; finally, when using this shared
fixture from E2EFixture, instantiate or call it with cache-reset-on-dispose
disabled so E2E's lease logic remains the single owner of cache reset decisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bbc9a14-11a7-455c-8228-abab50be3697
📒 Files selected for processing (5)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs
- src/Arius.Core.Tests/Features/ListQuery/ListQueryHandlerTests.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs (1)
251-257:⚠️ Potential issue | 🟡 MinorEnsure temp-root cleanup still runs when cache reset fails.
On Line 254, an exception from
ResetLocalCacheAsyncwill skip Line 256-257, leaking temp directories. Wrap cache reset intry/finallyso_deleteTempRootalways executes.🧹 Proposed fix
public async ValueTask DisposeAsync() { Index.Dispose(); - await ResetLocalCacheAsync(_account, _container); - - if (Directory.Exists(_tempRoot)) - _deleteTempRoot(_tempRoot); + try + { + await ResetLocalCacheAsync(_account, _container); + } + finally + { + if (Directory.Exists(_tempRoot)) + _deleteTempRoot(_tempRoot); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` around lines 251 - 257, DisposeAsync currently calls Index.Dispose(), then awaits ResetLocalCacheAsync(_account, _container) and only after that deletes _tempRoot, so if ResetLocalCacheAsync throws the temp root is leaked; change DisposeAsync to wrap the await ResetLocalCacheAsync(_account, _container) call in a try/finally block and move the _deleteTempRoot(_tempRoot) call into the finally so _deleteTempRoot always runs, keeping Index.Dispose() as-is and preserving the await on ResetLocalCacheAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Around line 251-257: DisposeAsync currently calls Index.Dispose(), then awaits
ResetLocalCacheAsync(_account, _container) and only after that deletes
_tempRoot, so if ResetLocalCacheAsync throws the temp root is leaked; change
DisposeAsync to wrap the await ResetLocalCacheAsync(_account, _container) call
in a try/finally block and move the _deleteTempRoot(_tempRoot) call into the
finally so _deleteTempRoot always runs, keeping Index.Dispose() as-is and
preserving the await on ResetLocalCacheAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1aa5bc0e-12f0-4260-8731-749c93ec64f1
📒 Files selected for processing (1)
src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
…hub.com/woutervanranst/Arius7 into feat/refactor-arius-core-tests-fixtures
…hub.com/woutervanranst/Arius7 into feat/refactor-arius-core-tests-fixtures
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
=======================================
Coverage 78.28% 78.28%
=======================================
Files 70 70
Lines 4932 4932
Branches 679 679
=======================================
Hits 3861 3861
Misses 907 907
Partials 164 164
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced Apr 29, 2026
This was referenced May 8, 2026
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
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.
Summary
RepositoryTestFixturewith in-memory setup, list-query handler creation, timestamped file writes, and repository cache cleanup.Test Plan
dotnet test --project src/Arius.Core.Tests/Arius.Core.Tests.csprojslopwatch analyzeSummary by CodeRabbit
Tests
Benchmarks