Improve Arius.Explorer test coverage#64
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a SLOP Watch baseline and example config, updates docs and a test project dependency, injects ILogger and a test-injectable TimeProvider into production view models/settings, replaces polling waits with event-driven test helpers, and adds many new and expanded unit tests across Explorer components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #64 +/- ##
==========================================
+ Coverage 72.75% 78.69% +5.93%
==========================================
Files 66 66
Lines 4831 4840 +9
Branches 654 655 +1
==========================================
+ Hits 3515 3809 +294
+ Misses 1171 875 -296
- Partials 145 156 +11
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:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs (2)
146-149: Avoid sync-over-async (GetAwaiter().GetResult()) in a test that can simply beasync.
OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepositoryis otherwise sync, but blocking onWaitForAsync(...).GetAwaiter().GetResult()defeats the deterministic event-driven wait you just introduced and is inconsistent with the other tests here that areasync Task. Under a synchronization context this can deadlock; even without one, it ties up a thread-pool thread for up to the timeout window.♻️ Proposed fix
- public void OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository() + public async Task OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository() { ... - WaitForAsync(viewModel, () => + await WaitForAsync(viewModel, () => viewModel.ContainerNames.Count == 1 && viewModel.ContainerName == "valid-container-123" && - !viewModel.IsLoading).GetAwaiter().GetResult(); + !viewModel.IsLoading);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs` around lines 146 - 149, The test OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository should not block on WaitForAsync(...).GetAwaiter().GetResult(); instead make the test method async Task, remove the synchronous GetResult call, and await the WaitForAsync call (await WaitForAsync(viewModel, () => ...);). Update the test method signature to async Task OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository and ensure any test framework attributes remain; this preserves the event-driven wait in WaitForAsync and avoids sync-over-async deadlocks.
244-286:WaitForAsyncswallowsOperationCanceledExceptionon timeout, making timeouts look like success.When
condition()is still false at timeout,WaitToReadAsync(cancellationTokenSource.Token)will throwOperationCanceledException, which propagates out ofWaitForAsync— but because the surrounding tests then immediately assert against the very state the wait was supposed to establish, the assertion failure (rather than a clear "wait timed out") is what you'll see. More importantly, if a later writer happens to signal between the cancel and the next loop iteration, it's easy to misread flaky failures.Consider catching
OperationCanceledExceptioninside the loop and throwing a descriptiveTimeoutException(or asserting with Shouldly) so timeouts are unambiguous, e.g.:catch (OperationCanceledException) when (cancellationTokenSource.IsCancellationRequested) { throw new TimeoutException($"Condition not met within {timeoutMilliseconds} ms."); }Minor: the initial
Signal(signal)at line 266 is redundant since you already evaluatedcondition()at line 246 and re-evaluate it at the top of thewhile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs` around lines 244 - 286, The WaitForAsync helper currently allows OperationCanceledException from signal.Reader.WaitToReadAsync to escape and be misinterpreted as a failed assertion; update WaitForAsync (the method and its loop that calls signal.Reader.WaitToReadAsync) to catch OperationCanceledException when cancellationTokenSource.IsCancellationRequested and rethrow a descriptive TimeoutException (e.g. include timeoutMilliseconds), and remove the redundant initial Signal(signal) call before entering the read loop; keep the existing registrations/removals for OnPropertyChanged and OnContainerNamesChanged and ensure the cancellationTokenSource is used for the WaitToReadAsync call.src/Arius.Explorer.Tests/Settings/RecentRepositoryManagerTests.cs (1)
124-128: Clamping the clock can mask off-by-one errors in tests.
CreateClocksilently returns the last value forever after the array is exhausted. That's convenient, but in tests likeRecentRepositoryManager_TouchOrAdd_RespectsRecentLimit(3 supplied values, 3 calls) it means a future change that adds a 4thTouchOrAddcall would silently reuse the same timestamp instead of failing the test, potentially hiding ordering bugs. Consider throwing (orShouldFail-ing) when the clock is overdrawn so tests fail loudly when expectations drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer.Tests/Settings/RecentRepositoryManagerTests.cs` around lines 124 - 128, The CreateClock helper currently clamps and reuses the last timestamp when calls exceed the provided values, which can mask off-by-one errors; update CreateClock (used by tests like RecentRepositoryManager_TouchOrAdd_RespectsRecentLimit) to instead throw an exception (or call the test framework's fail helper) when the clock is overdrawn so tests fail loudly if more ticks are requested than supplied. Locate the CreateClock method and replace the Math.Min clamping logic with a bounds check that throws (or signals test failure) when index >= values.Length, otherwise return values[index++].src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs (1)
26-27: MakeRepositoryExplorerViewModelandShowMessageBoxinternal, per coding guidelines.The class is marked
publicbut not used outside theArius.Explorerassembly. Per guidelines, non-test classes should beinternalunless consumed by another assembly. Changepublic partial classtointernal partial classandpublic statictointernal static, then addInternalsVisibleToinAssemblyInfo.csso tests can access it.Alternatively, inject
ShowMessageBoxas a constructor dependency (e.g.,Func<...>) to remove the mutable static state entirely—this aligns with the existingIDialogServicepattern and is more testable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs` around lines 26 - 27, Change the accessibility of RepositoryExplorerViewModel from public partial class to internal partial class and change the ShowMessageBox field from public static to internal static, then add an InternalsVisibleTo entry for the test assembly in AssemblyInfo.cs so tests can access the type; alternatively (preferred for testability) remove the mutable static ShowMessageBox and accept a Func<string,string,MessageBoxButton,MessageBoxImage,MessageBoxResult> showMessageBox parameter on the RepositoryExplorerViewModel constructor (or use the existing IDialogService), store it on an instance field, and update all usages to call the instance delegate instead of the static.src/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cs (1)
409-425: MoveFakeRepositorySessioninto the localFakes/folder.This fake is reused across the test suite in this file, so placing it beside the tests under
RepositoryExplorer/Fakes/keeps test doubles consistent and reusable. As per coding guidelines,**/*.Tests/**/*.cs: Put reusable test doubles inFakes/. Put scenario-specific test doubles in a localFakes/subfolder beside the tests that use them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cs` around lines 409 - 425, Move the FakeRepositorySession test double out of the test file and into the test project's shared Fakes folder used by RepositoryExplorer tests: create a new file containing the same FakeRepositorySession class (implementing IRepositorySession, with Mediator, Repository, ConnectCalls, ConnectAsync and Dispose members) in the test namespace so other tests can reuse it, remove the duplicate nested definition from RepositoryExplorerViewModelTests, and update any tests to reference the single shared FakeRepositorySession type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 120-121: Update AGENTS.md to remove or correct the non-existent
coverage settings path: either add the missing file
src/Arius.Explorer.Tests/coverage.settings.xml to the repo or replace that
reference with the actual coverage configuration used by the project; ensure the
guidance uses the correct flag (--coverage vs --collect:"XPlat Code Coverage")
and that the file path and flag examples in AGENTS.md accurately reflect the
current project setup.
In
`@src/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cs`:
- Around line 144-149: Add an assertion that the chosen repository was actually
passed to the session connect flow: after executing
viewModel.OpenChooseRepositoryDialogCommand and the existing assertions, verify
that the fake session's mediator had ConnectAsync invoked with the selected
repository (e.g. assert
FakeRepositorySession.Mediator.Received(1).ConnectAsync(repository) or
equivalent), ensuring the session handoff occurred rather than relying on
pre-populated FakeRepositorySession.Mediator.
- Around line 28-32: The static delegate
RepositoryExplorerViewModel.ShowMessageBox is being overridden in tests and not
restored afterwards, causing cross-test static state leakage; modify the test
fixture by capturing the original delegate in ResetMessageBox (or a new
[Before(Test)] method) into a private field (e.g., originalShowMessageBox) and
add an [After(Test)] teardown method that restores
RepositoryExplorerViewModel.ShowMessageBox = originalShowMessageBox so any
overrides made in tests (the ones that currently mutate ShowMessageBox) are
reverted after each test.
In `@src/Arius.Explorer/ChooseRepository/ChooseRepositoryViewModel.cs`:
- Line 4: ChooseRepositoryViewModel currently uses System.Diagnostics.Trace for
logging; replace it with constructor-injected
ILogger<ChooseRepositoryViewModel>: add a constructor parameter
ILogger<ChooseRepositoryViewModel> logger, store it in a private readonly field
(e.g., _logger), and remove System.Diagnostics.Trace usages—import
Microsoft.Extensions.Logging and replace Trace.TraceError/TraceInformation calls
in methods of ChooseRepositoryViewModel with _logger.LogError,
_logger.LogInformation or _logger.LogWarning as appropriate so logs flow through
the app's Serilog-backed logging pipeline.
---
Nitpick comments:
In `@src/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cs`:
- Around line 146-149: The test
OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository should
not block on WaitForAsync(...).GetAwaiter().GetResult(); instead make the test
method async Task, remove the synchronous GetResult call, and await the
WaitForAsync call (await WaitForAsync(viewModel, () => ...);). Update the test
method signature to async Task
OpenRepositoryCommand_WhenAllFieldsAreValid_IsEnabledAndBuildsRepository and
ensure any test framework attributes remain; this preserves the event-driven
wait in WaitForAsync and avoids sync-over-async deadlocks.
- Around line 244-286: The WaitForAsync helper currently allows
OperationCanceledException from signal.Reader.WaitToReadAsync to escape and be
misinterpreted as a failed assertion; update WaitForAsync (the method and its
loop that calls signal.Reader.WaitToReadAsync) to catch
OperationCanceledException when cancellationTokenSource.IsCancellationRequested
and rethrow a descriptive TimeoutException (e.g. include timeoutMilliseconds),
and remove the redundant initial Signal(signal) call before entering the read
loop; keep the existing registrations/removals for OnPropertyChanged and
OnContainerNamesChanged and ensure the cancellationTokenSource is used for the
WaitToReadAsync call.
In
`@src/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cs`:
- Around line 409-425: Move the FakeRepositorySession test double out of the
test file and into the test project's shared Fakes folder used by
RepositoryExplorer tests: create a new file containing the same
FakeRepositorySession class (implementing IRepositorySession, with Mediator,
Repository, ConnectCalls, ConnectAsync and Dispose members) in the test
namespace so other tests can reuse it, remove the duplicate nested definition
from RepositoryExplorerViewModelTests, and update any tests to reference the
single shared FakeRepositorySession type.
In `@src/Arius.Explorer.Tests/Settings/RecentRepositoryManagerTests.cs`:
- Around line 124-128: The CreateClock helper currently clamps and reuses the
last timestamp when calls exceed the provided values, which can mask off-by-one
errors; update CreateClock (used by tests like
RecentRepositoryManager_TouchOrAdd_RespectsRecentLimit) to instead throw an
exception (or call the test framework's fail helper) when the clock is overdrawn
so tests fail loudly if more ticks are requested than supplied. Locate the
CreateClock method and replace the Math.Min clamping logic with a bounds check
that throws (or signals test failure) when index >= values.Length, otherwise
return values[index++].
In `@src/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cs`:
- Around line 26-27: Change the accessibility of RepositoryExplorerViewModel
from public partial class to internal partial class and change the
ShowMessageBox field from public static to internal static, then add an
InternalsVisibleTo entry for the test assembly in AssemblyInfo.cs so tests can
access the type; alternatively (preferred for testability) remove the mutable
static ShowMessageBox and accept a
Func<string,string,MessageBoxButton,MessageBoxImage,MessageBoxResult>
showMessageBox parameter on the RepositoryExplorerViewModel constructor (or use
the existing IDialogService), store it on an instance field, and update all
usages to call the instance delegate instead of the static.
🪄 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: 0923f1d6-ba01-459d-9892-f53e9ee92336
📒 Files selected for processing (18)
.slopwatch/baseline.json.slopwatch/config.json.exampleAGENTS.mdsrc/Arius.Explorer.Tests/Arius.Explorer.Tests.csprojsrc/Arius.Explorer.Tests/ChooseRepository/ChooseRepositoryViewModelTests.cssrc/Arius.Explorer.Tests/Infrastructure/RepositorySessionTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/FileItemViewModelTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/RepositoryExplorerViewModelTests.cssrc/Arius.Explorer.Tests/RepositoryExplorer/TreeNodeViewModelTests.cssrc/Arius.Explorer.Tests/Settings/ApplicationSettingsTests.cssrc/Arius.Explorer.Tests/Settings/RecentRepositoryManagerTests.cssrc/Arius.Explorer.Tests/Settings/RepositoryOptionsTests.cssrc/Arius.Explorer.Tests/Shared/Converters/BytesToReadableSizeConverterTests.cssrc/Arius.Explorer.Tests/Shared/Extensions/DataProtectionExtensionsTests.cssrc/Arius.Explorer.Tests/Usings.cssrc/Arius.Explorer/ChooseRepository/ChooseRepositoryViewModel.cssrc/Arius.Explorer/RepositoryExplorer/RepositoryExplorerViewModel.cssrc/Arius.Explorer/Settings/ApplicationSettings.cs
|
@CodeRabbit resume review |
|
✅ Actions performedReviews resumed. |
Summary
Verification
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores