refactor: adopt typed filesystem paths across tests#91
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:
📝 WalkthroughWalkthroughThis PR migrates tests, fixtures, fakes and helpers from string/path APIs to typed LocalDirectory/RelativePath/RelativeFileSystem; rekeys in-memory blob fakes to RelativePath, adds TestTempRoots and BlobPathsExtensions, and updates related Snapshot/Repository APIs and many tests. ChangesTyped filesystem & test infra migration
Sequence Diagram(s)(omitted — migration is broad and not a single sequential control-flow change) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cs (1)
108-109: ⚡ Quick winUse
RelativePath.Root / prefixinstead ofRelativePath.Parse(prefix.ToString()).
prefixis already a typedPathSegmentfromShard.PrefixOf(). Composing it directly via the/operator avoids the stringify/parse round-trip:Suggested fix
-var l2Path = RepositoryPaths.GetChunkIndexCacheRoot(Account, containerName).Resolve(RelativePath.Parse(prefix.ToString())); +var l2Path = RepositoryPaths.GetChunkIndexCacheRoot(Account, containerName).Resolve(RelativePath.Root / prefix);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cs` around lines 108 - 109, The code builds l2Path by composing RepositoryPaths.GetChunkIndexCacheRoot(Account, containerName) with RelativePath.Parse(prefix.ToString()), but prefix is already a PathSegment from Shard.PrefixOf(contentHash); replace the parse round-trip with direct composition using RelativePath.Root / prefix so that l2Path is created by resolving RelativePath.Root / prefix instead of parsing prefix.ToString().src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs (1)
49-49: ⚡ Quick winExpose
FileTreeCacheDirectoryasLocalDirectoryinstead ofstring.This currently forces a
LocalDirectory -> string -> LocalDirectoryround-trip (see downstreamLocalDirectory.Parse(env.FileTreeCacheDirectory)), which weakens the type-safety this PR is introducing.Suggested diff
diff --git a/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs b/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs @@ - public string FileTreeCacheDirectory => _fileTreeCacheDirectory.ToString(); + public LocalDirectory FileTreeCacheDirectory => _fileTreeCacheDirectory;diff --git a/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs b/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs @@ - await using var stagingSession = await FileTreeStagingSession.OpenAsync(LocalDirectory.Parse(env.FileTreeCacheDirectory)); + await using var stagingSession = await FileTreeStagingSession.OpenAsync(env.FileTreeCacheDirectory);As per coding guidelines, "Avoid stringify/parse round-trips. Preserve strong types until a real foreign boundary such as console output, configuration, serialization, or external SDK calls."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs` at line 49, Change the FileTreeCacheDirectory property to return a LocalDirectory instead of string to avoid stringify/parse round-trips: replace the getter signature for FileTreeCacheDirectory to return the existing _fileTreeCacheDirectory (as a LocalDirectory) and update callers that currently do LocalDirectory.Parse(env.FileTreeCacheDirectory) to use the property directly; ensure any tests or consumers expecting a string are adjusted to call .ToString() only when crossing a foreign boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs`:
- Line 350: The test currently calls Directory.Delete(cacheDir.ToString(),
recursive: true) which can throw if the directory is already removed; replace
that raw delete with the existing helper DeleteDirectoryIfExists(cacheDir) to
safely guard against missing directories and match the file's cleanup
pattern—locate the call in FileTreeBuilderTests (the line invoking
Directory.Delete) and swap it for the DeleteDirectoryIfExists(cacheDir) helper
method.
---
Nitpick comments:
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs`:
- Line 49: Change the FileTreeCacheDirectory property to return a LocalDirectory
instead of string to avoid stringify/parse round-trips: replace the getter
signature for FileTreeCacheDirectory to return the existing
_fileTreeCacheDirectory (as a LocalDirectory) and update callers that currently
do LocalDirectory.Parse(env.FileTreeCacheDirectory) to use the property
directly; ensure any tests or consumers expecting a string are adjusted to call
.ToString() only when crossing a foreign boundary.
In `@src/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cs`:
- Around line 108-109: The code builds l2Path by composing
RepositoryPaths.GetChunkIndexCacheRoot(Account, containerName) with
RelativePath.Parse(prefix.ToString()), but prefix is already a PathSegment from
Shard.PrefixOf(contentHash); replace the parse round-trip with direct
composition using RelativePath.Root / prefix so that l2Path is created by
resolving RelativePath.Root / prefix instead of parsing prefix.ToString().
🪄 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: 88ef845a-f3bc-4729-95fa-12b7b54234ab
📒 Files selected for processing (32)
src/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/FileTree/Fakes/UnsortedSnapshotBlobContainerService.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsTests.cssrc/Arius.Core.Tests/Shared/Snapshot/SnapshotSerializerTests.cssrc/Arius.Core/Shared/Snapshot/SnapshotService.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cssrc/Arius.E2E.Tests/E2ETests.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cssrc/Arius.Integration.Tests/ChunkIndex/ChunkIndexServiceIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/GcmIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ListQueryIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreCostModelTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreDispositionTests.cssrc/Arius.Integration.Tests/Pipeline/RestorePointerTimestampTests.cssrc/Arius.Integration.Tests/Pipeline/RoundtripTests.cssrc/Arius.Integration.Tests/Shared/FileTree/FileTreeBuilderIntegrationTests.cssrc/Arius.Tests.Shared/AssemblyMarker.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cssrc/Arius.Tests.Shared/RepositoryPathStrings.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
💤 Files with no reviewable changes (3)
- src/Arius.Core/Shared/Snapshot/SnapshotService.cs
- src/Arius.Tests.Shared/RepositoryPathStrings.cs
- src/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #91 +/- ##
==========================================
- Coverage 78.77% 78.52% -0.25%
==========================================
Files 78 78
Lines 5277 5300 +23
Branches 722 731 +9
==========================================
+ Hits 4157 4162 +5
- Misses 928 939 +11
- Partials 192 199 +7
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:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Shared/RepositoryTestFixtureTests.cs`:
- Around line 21-37: The test double-disposes the fixture:
RepositoryTestFixture.CreateWithEncryptionAsync is awaited into "fixture" with
"await using", so remove the explicit await fixture.DisposeAsync() call (the
DisposeAsync invocation on the fixture) to avoid running cleanup twice; rely on
the language-scoped disposal provided by "await using" and keep the remaining
assertions after the scope as-is.
In `@src/Arius.E2E.Tests/Fixtures/E2EFixtureTests.cs`:
- Around line 9-41: The test class E2EFixtureTests and its test method
DisposeAsync_PreservesLocalCache_WhenRequested (which uses
E2EFixture.ResetLocalCacheAsync, E2EFixture.CreateAsync,
PreserveLocalCacheAsync, DisposeAsync) should be moved out of
src/Arius.E2E.Tests into an appropriate unit-test project (e.g.,
Arius.Tests.Shared.Tests or a dedicated fixtures test project); create a new
test file in that project, update the namespace and test project references to
include any required test helpers or FakeInMemoryBlobContainerService, and
ensure the target test project's csproj references the production assembly
containing E2EFixture and RepositoryPaths so the test compiles and runs.
In `@src/Arius.Tests.Shared/AssemblyMarker.cs`:
- Line 9: Change the visibility of the marker type AssemblyMarker from public to
internal: locate the declaration "AssemblyMarker" (the sealed marker class) and
replace its public modifier with internal so tests access it via
InternalsVisibleTo; ensure no external non-test consumers require it before
changing.
🪄 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: cff7dc05-374e-455b-b4f8-085af535675a
📒 Files selected for processing (26)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/RepositoryTestFixtureTests.cssrc/Arius.Core/AssemblyMarker.cssrc/Arius.E2E.Tests/E2ETests.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Fixtures/E2EFixtureTests.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowRunner.cssrc/Arius.E2E.Tests/Workflows/RepresentativeWorkflowState.cssrc/Arius.E2E.Tests/Workflows/Steps/ArchiveTierLifecycleStep.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cssrc/Arius.E2E.Tests/Workflows/Steps/MaterializeVersionStep.cssrc/Arius.Integration.Tests/Pipeline/ContainerCreationTests.cssrc/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cssrc/Arius.Integration.Tests/Pipeline/GcmIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/ListQueryIntegrationTests.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cssrc/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreCostModelTests.cssrc/Arius.Integration.Tests/Pipeline/RestoreDispositionTests.cssrc/Arius.Integration.Tests/Pipeline/RestorePointerTimestampTests.cssrc/Arius.Integration.Tests/Pipeline/RoundtripTests.cssrc/Arius.Tests.Shared/AssemblyMarker.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
✅ Files skipped from review due to trivial changes (1)
- src/Arius.Core/AssemblyMarker.cs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Arius.Integration.Tests/Pipeline/RestoreCostModelTests.cs
- src/Arius.Integration.Tests/Pipeline/RehydrationStateTests.cs
- src/Arius.Integration.Tests/Pipeline/RestorePointerTimestampTests.cs
- src/Arius.Integration.Tests/Pipeline/RecoveryScriptTests.cs
- src/Arius.E2E.Tests/E2ETests.cs
- src/Arius.Integration.Tests/Pipeline/RestoreDispositionTests.cs
- src/Arius.Integration.Tests/Pipeline/CrashRecoveryTests.cs
- src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs
- src/Arius.Integration.Tests/Pipeline/GcmIntegrationTests.cs
- src/Arius.Integration.Tests/Pipeline/ListQueryIntegrationTests.cs
- src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs
Keep repository-relative existence checks on RelativeFileSystem so test code follows the same typed path boundaries as the rest of the codebase. Preserve LocalDirectory.Resolve for true host-path boundaries such as File.OpenRead and directory lifecycle work.
Keep in-memory blob test doubles on typed blob paths instead of string keys so blob lookups, listings, and collision state stay inside the existing RelativePath boundary.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs`:
- Around line 34-37: The current check-then-write on the ConcurrentDictionary
_blobs in UploadAsync and the OpenWriteAsync callback is non-atomic and can
cause TOCTOU races; change the logic so that when overwrite == false you use
_blobs.TryAdd(blobName, new StoredBlob(...)) and throw
BlobAlreadyExistsException if TryAdd returns false, while preserving direct
assignment (_blobs[blobName] = ...) when overwrite == true; apply the same
TryAdd+throw pattern inside the OpenWriteAsync callback where a new StoredBlob
is added so the add is atomic.
🪄 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: dfc51704-bbe0-4ef7-8d0f-76829fce4848
📒 Files selected for processing (2)
src/Arius.Core.Tests/Fakes/FakeSeededBlobContainerService.cssrc/Arius.Tests.Shared/Storage/FakeInMemoryBlobContainerService.cs
Keep synthetic dataset state keyed by RelativePath so workflow helpers and disk assertions can compare repository-relative files without stringify/parse round trips.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cs (1)
17-17: ⚡ Quick winAvoid string-based path logic in typed-path assertions.
The pointer-file filter and ordering currently depend on
RelativePath.ToString(), which reintroduces string-path semantics in core test logic. Prefer aRelativePath-aware helper/comparer for suffix checks and ordering so the assertion path stays strongly typed end-to-end.As per coding guidelines, "Avoid stringify/parse round-trips. Preserve strong types until a real foreign boundary such as console output, configuration, serialization, or external SDK calls." and "Prefer RelativePath for repository-relative paths, subtree roots, and prefixes that may contain multiple segments."
Also applies to: 24-25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cs` at line 17, The code is using relativePath.ToString().EndsWith(".pointer.arius", ...) which reintroduces string-path logic; replace this with a RelativePath-aware check and ordering using the RelativePath value itself. Create/inline a helper such as HasPointerFileSuffix(RelativePath relativePath) (or a small RelativePathSuffixComparer) and use that in the if condition instead of ToString().EndsWith, and update any sorting logic at the other occurrences (lines 24–25) to use the same RelativePath-aware comparator so suffix checks and ordering remain strongly typed and avoid string round-trips; reference the local variable relativePath and the includePointerFiles flag when applying the new helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cs`:
- Line 17: The code is using relativePath.ToString().EndsWith(".pointer.arius",
...) which reintroduces string-path logic; replace this with a
RelativePath-aware check and ordering using the RelativePath value itself.
Create/inline a helper such as HasPointerFileSuffix(RelativePath relativePath)
(or a small RelativePathSuffixComparer) and use that in the if condition instead
of ToString().EndsWith, and update any sorting logic at the other occurrences
(lines 24–25) to use the same RelativePath-aware comparator so suffix checks and
ordering remain strongly typed and avoid string round-trips; reference the local
variable relativePath and the includePointerFiles flag when applying the new
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d87fe3b-93b9-4796-80e3-1c93119ce064
📒 Files selected for processing (4)
src/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryState.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryStateAssertions.cssrc/Arius.E2E.Tests/Workflows/Steps/Helpers.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Arius.E2E.Tests/Workflows/Steps/Helpers.cs
- src/Arius.E2E.Tests/Datasets/SyntheticRepositoryMaterializer.cs
…ranst/Arius7 into remove-blobpathstrings
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Arius.Integration.Tests/Pipeline/PipelineFixture.cs (1)
143-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale
DisposeAsyncXML summary.Line 143-144 still states chunk-index cache deletion, but
DisposeAsyncnow only delegates toRepository.DisposeAsync(). Please align the summary with actual behavior to avoid misleading maintenance/debugging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Integration.Tests/Pipeline/PipelineFixture.cs` around lines 143 - 144, Update the XML summary for the PipelineFixture.DisposeAsync method to reflect its current behavior: remove the mention of deleting the fixture's temporary directory and repository-specific chunk-index cache and state that DisposeAsync simply delegates to Repository.DisposeAsync(); reference the DisposeAsync method and the Repository.DisposeAsync() call in the summary so the comment matches the implementation.
🧹 Nitpick comments (1)
src/Arius.E2E.Tests/Fixtures/E2EFixture.cs (1)
71-85: 💤 Low value
PreserveRequestedflag is set but never consumed.
PreserveLocalCacheAsyncsetsPreserveRequested = truein the lease struct, but this flag is never read. The cache-reset-on-dispose logic that previously consumed this flag was removed fromDisposeAsync.Either remove this dead code or restore the logic that consumes the flag during disposal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs` around lines 71 - 85, PreserveLocalCacheAsync currently sets the PreserveRequested flag on the lease (via GetRepositoryCacheKey and RepositoryCacheLeases under RepositoryCacheLeaseLock) but nothing reads it; update the disposal path in DisposeAsync to check the lease for PreserveRequested and skip resetting/clearing the cache when true (or alternatively remove the PreserveRequested write in PreserveLocalCacheAsync if you prefer to drop the feature). Specifically, either remove the unused assignment to RepositoryCacheLeases[cacheKey].PreserveRequested in PreserveLocalCacheAsync, or restore logic in DisposeAsync to look up RepositoryCacheLeases[cacheKey], respect lease.PreserveRequested, and avoid cache reset when set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs`:
- Line 258: The test double-disposes the staging session: remove the explicit
await stagingSession1.DisposeAsync() call since stagingSession1 is already
scoped by the await using at its creation; locate the await using declaration
for stagingSession1 and delete the redundant DisposeAsync invocation so disposal
is handled only by the using scope (do not change the await using itself or
other cleanup logic).
In `@src/Arius.Core/Shared/FileSystem/RelativeFileSystem.cs`:
- Around line 207-211: DeleteDirectory(RelativePath, bool) in RelativeFileSystem
now calls Directory.Delete(_root.Resolve(path), recursive) directly which will
throw DirectoryNotFoundException for missing paths; to restore previous no-op
behavior, check existence first and only call Directory.Delete if
Directory.Exists(fullPath) (use _root.Resolve(path) to get fullPath) or
document/propagate the new behavior if intentional. Ensure you update the
DeleteDirectory method in class RelativeFileSystem and keep the recursive
semantics when performing the deletion.
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs`:
- Around line 111-115: DisposeAsync currently only calls Repository.DisposeAsync
and awaits Task.CompletedTask, but never decrements the static lease counter
LiveFixtureCount so HasActiveLease stays true; update DisposeAsync to decrement
LiveFixtureCount (preferably with thread-safe Interlocked.Decrement(ref
LiveFixtureCount)) after disposing the repository and remove the redundant await
Task.CompletedTask so the method simply awaits Repository.DisposeAsync() then
decrements the counter to restore correct lease behavior for ResetLocalCache.
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Line 204: The property ResetLocalCacheOnDispose is unused; update DisposeAsync
to respect it by invoking the index cache reset before disposal when the flag is
true: inside DisposeAsync, add "if (ResetLocalCacheOnDispose) await
Index.ResetLocalCacheAsync()" (or call the appropriate
Index.ClearLocalCache/ResetLocalCache synchronous API if async variant doesn't
exist), then proceed to dispose Index; alternatively, if there is no index
cache-reset API or you prefer explicit cleanup, remove the
ResetLocalCacheOnDispose property and its assignments in the factory methods to
avoid misleading callers.
In `@src/Arius.Tests.Shared/TestTempRoots.cs`:
- Around line 19-21: The current cleanup in TestTempRoots.cs builds tempRoot
from FolderName and deletes the entire shared Path.GetTempPath()/FolderName,
which can collide across parallel test runs; change the logic so that
Setup/constructor for the TestSession (or wherever FolderName is used) creates a
per-session subfolder (e.g., combine FolderName with a unique session id or
GUID) and use that as tempRoot, track created session roots (e.g., a list in the
TestSession or TestTempRoots class), and update the teardown/After(TestSession)
logic to only delete the tracked per-session folder(s) (referencing FolderName,
tempRoot and the After(TestSession) teardown hook) rather than deleting the
whole shared folder tree.
---
Outside diff comments:
In `@src/Arius.Integration.Tests/Pipeline/PipelineFixture.cs`:
- Around line 143-144: Update the XML summary for the
PipelineFixture.DisposeAsync method to reflect its current behavior: remove the
mention of deleting the fixture's temporary directory and repository-specific
chunk-index cache and state that DisposeAsync simply delegates to
Repository.DisposeAsync(); reference the DisposeAsync method and the
Repository.DisposeAsync() call in the summary so the comment matches the
implementation.
---
Nitpick comments:
In `@src/Arius.E2E.Tests/Fixtures/E2EFixture.cs`:
- Around line 71-85: PreserveLocalCacheAsync currently sets the
PreserveRequested flag on the lease (via GetRepositoryCacheKey and
RepositoryCacheLeases under RepositoryCacheLeaseLock) but nothing reads it;
update the disposal path in DisposeAsync to check the lease for
PreserveRequested and skip resetting/clearing the cache when true (or
alternatively remove the PreserveRequested write in PreserveLocalCacheAsync if
you prefer to drop the feature). Specifically, either remove the unused
assignment to RepositoryCacheLeases[cacheKey].PreserveRequested in
PreserveLocalCacheAsync, or restore logic in DisposeAsync to look up
RepositoryCacheLeases[cacheKey], respect lease.PreserveRequested, and avoid
cache reset when set.
🪄 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: 09be2f59-1d5d-415d-9c1c-cd59667481db
📒 Files selected for processing (19)
.agents/skills/csharp-coding-standards/SKILL.md.agents/skills/csharp-tunit/SKILL.mdsrc/Arius.Benchmarks/ArchiveStepBenchmarks.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Features/RestoreCommand/RestoreCommandHandlerTests.cssrc/Arius.Core.Tests/Shared/FileSystem/RelativeFileSystemTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Core.Tests/Shared/RepositoryPathsTests.cssrc/Arius.Core.Tests/Shared/Storage/BlobPathsTests.cssrc/Arius.Core/Shared/FileSystem/LocalDirectory.cssrc/Arius.Core/Shared/FileSystem/RelativeFileSystem.cssrc/Arius.Core/Shared/RepositoryPaths.cssrc/Arius.E2E.Tests/Fixtures/E2EFixture.cssrc/Arius.E2E.Tests/Workflows/Steps/ResetCacheStep.cssrc/Arius.Integration.Tests/Pipeline/PipelineFixture.cssrc/Arius.Tests.Shared/BlobPathsExtensions.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cssrc/Arius.Tests.Shared/TestTempRoots.cs
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/csharp-coding-standards/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Arius.Core/Shared/RepositoryPaths.cs
- src/Arius.Core/Shared/FileSystem/LocalDirectory.cs
- src/Arius.Benchmarks/ArchiveStepBenchmarks.cs
- src/Arius.Core.Tests/Shared/RepositoryPathsTests.cs
- src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs (1)
204-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
ResetLocalCacheOnDisposeinDisposeAsyncor remove it.
ResetLocalCacheOnDisposeis required but not consumed, so callers can’t rely on the behavior implied by the API contract.Proposed fix
public ValueTask DisposeAsync() { + if (ResetLocalCacheOnDispose) + DeleteLocalCacheDirectory(AccountName, ContainerName); + Index.Dispose(); return ValueTask.CompletedTask; }Also applies to: 250-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs` at line 204, RepositoryTestFixture declares the required property ResetLocalCacheOnDispose but DisposeAsync does not use it; update the RepositoryTestFixture.DisposeAsync method to check the ResetLocalCacheOnDispose flag and perform the local-cache reset/cleanup when true (call the existing reset/clear method or code path that clears the fixture's cache), or if you intended not to support this behavior remove the required modifier from ResetLocalCacheOnDispose (and any similar required bool properties) so the API surface matches implementation; reference ResetLocalCacheOnDispose and the DisposeAsync method in RepositoryTestFixture when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cs`:
- Line 35: The test incorrectly calls ShouldNotBeNull() on the Task instead of
the awaited lookup result in ArchiveRecoveryTests; change both occurrences (the
LookupAsync(contentHash) assertions) to await the task first and assert on the
returned value (e.g., either var result = await
fixture.Index.LookupAsync(contentHash); result.ShouldNotBeNull(); or (await
fixture.Index.LookupAsync(contentHash)).ShouldNotBeNull()), updating the
assertions at the two locations that reference LookupAsync in
ArchiveRecoveryTests.
---
Duplicate comments:
In `@src/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs`:
- Line 204: RepositoryTestFixture declares the required property
ResetLocalCacheOnDispose but DisposeAsync does not use it; update the
RepositoryTestFixture.DisposeAsync method to check the ResetLocalCacheOnDispose
flag and perform the local-cache reset/cleanup when true (call the existing
reset/clear method or code path that clears the fixture's cache), or if you
intended not to support this behavior remove the required modifier from
ResetLocalCacheOnDispose (and any similar required bool properties) so the API
surface matches implementation; reference ResetLocalCacheOnDispose and the
DisposeAsync method in RepositoryTestFixture when making the change.
🪄 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: 2b1357f2-8305-415c-b42c-6820c9e100f8
📒 Files selected for processing (5)
src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveRecoveryTests.cssrc/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cssrc/Arius.Core.Tests/Shared/FileSystem/RelativeFileSystemTests.cssrc/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cssrc/Arius.Tests.Shared/Fixtures/RepositoryTestFixture.cs
💤 Files with no reviewable changes (1)
- src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveTestEnvironment.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Arius.Core.Tests/Shared/FileTree/FileTreeServiceTests.cs
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
Testing