Replace Moq with NSubstitute for unit tests by Flaminel · Pull Request #566 · Cleanuparr/Cleanuparr

Flaminel · 2026-04-16T07:12:27Z

No description provided.

Flaminel · 2026-04-16T07:12:54Z

@greptileai review

Flaminel · 2026-04-16T07:13:01Z

/review

qodo-code-review · 2026-04-16T07:13:08Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0)

🐞\ ☼ Reliability (1)

📘\ ✧ Quality (1)

1. ~~xUnit Assert.* used~~ ☑ 📘 ▣

Description

Modified unit tests still use xUnit Assert.* assertions instead of Shouldly, which violates the
required xUnit + NSubstitute + Shouldly testing stack. This reduces consistency across the backend
test suite and breaks the mandated assertion style.

Code

code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[47]
+        Assert.Same(_sonarrClient, result);

Relevance

⭐⭐⭐ High
If repo mandates Shouldly, xUnit Assert.* violates required stack; change to Shouldly assertions.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 225592 requires assertions to be written with Shouldly (not Assert.*). The
modified test file contains updated assertion lines using Assert.Same(...) and
Assert.Throws(...).
Rule 225592: Use xUnit with NSubstitute and Shouldly in backend unit tests
code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[47-47]
code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[107-109]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests updated in this PR still use xUnit `Assert.*` assertions, but the compliance standard requires Shouldly assertions.
## Issue Context
This PR migrates mocks to NSubstitute; assertion style should also align with the standard stack (xUnit + NSubstitute + Shouldly).
## Fix Focus Areas
- code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[44-141]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Leaked Arg.Any matcher~~ ☑ 🐞 ☼

Description

ArrClientFactoryTests passes Arg.Any<float>() into a real (non-substitute) method call, which can
leave a pending NSubstitute argument specification on the thread and later cause
AmbiguousArgumentsException/flaky test failures. Use a real float value (e.g., 0f) instead of
Arg.Any outside substitute setups/verifications.

Code

code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[107]
+        var exception = Assert.Throws<NotImplementedException>(() => _factory.GetClient(unsupportedType, Arg.Any<float>()));

Relevance

⭐⭐⭐ High
Arg.Any used in real SUT call can leak pending specs and cause flaky AmbiguousArgumentsException;
use concrete float.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test calls the SUT with Arg.Any<float>() (not a substitute call), which creates an argument
matcher without a corresponding substitute invocation to consume it. This PR also introduces
SubstituteHelper specifically to clear leaked argument specifications and mentions
AmbiguousArgumentsException as the failure mode, corroborating that leaked specs are a known problem
in this suite.
code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[100-110]
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[5-19]
code/backend/Cleanuparr.Infrastructure.Tests/Features/DownloadClient/DelugeServiceFixture.cs[31-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Arg.Any<T>()` is being used as a normal argument value in a real method call. This can leak NSubstitute argument specifications into later tests and cause flaky `AmbiguousArgumentsException` failures.
### Issue Context
In `GetClient_UnsupportedType_ThrowsNotImplementedException`, the test is calling the real `ArrClientFactory.GetClient(...)` with `Arg.Any<float>()`. NSubstitute matchers should only be used inside substitute setup/verification calls.
### Fix Focus Areas
- code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[100-110]
### Suggested change
Replace `Arg.Any<float>()` with a concrete value such as `0f` (or `default(float)`), since the instanceVersion value is irrelevant to the exception assertion.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Logger ToString matching 🐞 ☼

Description

LoggerVerificationExtensions asserts logs by calling ToString() on the ILogger.Log state argument
and searching for a substring, which is formatting-dependent for structured logging. This can yield
false positives/negatives and make tests flaky when templates or state formatting change.

Code

code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/LoggerVerificationExtensions.cs[R60-66]

+    private static List<ICall> GetLogCalls<T>(ILogger<T> logger, LogLevel level, string message)
+    {
+        return logger.ReceivedCalls()
+            .Where(c => c.GetMethodInfo().Name == "Log")
+            .Where(c => c.GetArguments().Length > 0 && c.GetArguments()[0] is LogLevel l && l == level)
+            .Where(c => c.GetArguments().Length > 2 && c.GetArguments()[2]?.ToString()?.Contains(message) == true)
+            .ToList();

Relevance

⭐⭐⭐ High
ToString()-based log matching is brittle with structured logging; likely they’ll prefer asserting
template/state explicitly.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper filters ILogger.Log calls and matches messages using
c.GetArguments()[2]?.ToString()?.Contains(message), which depends on how
Microsoft.Extensions.Logging formats TState (often FormattedLogValues). Tests (e.g.,
BlacklistSynchronizerTests) now depend on this helper for log assertions.
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/LoggerVerificationExtensions.cs[60-66]
code/backend/Cleanuparr.Infrastructure.Tests/Features/BlacklistSync/BlacklistSynchronizerTests.cs[99-103]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Logger assertions rely on `state.ToString().Contains(...)`, which is brittle under structured logging and may break when message templates or state formatting changes.
## Issue Context
`ILogger.Log<TState>` commonly passes a structured `TState` that implements `IReadOnlyList<KeyValuePair<string, object?>>` and includes `"{OriginalFormat}"` for the template. Using `ToString()` for matching is not stable.
## Fix Focus Areas
- In `GetLogCalls`, inspect the `state` argument as `IReadOnlyList<KeyValuePair<string, object?>>` and match on:
- `"{OriginalFormat}"` (template) and/or
- specific structured keys/values
rather than `ToString()`.
- Optionally, adopt a test logger/sink (e.g., a custom `ILogger` implementation capturing structured state) for deterministic assertions.
### Fix Focus Areas (code)
- code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/LoggerVerificationExtensions.cs[60-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. ~~NSubstitute.Core internal usage~~ ☑ 🐞 ⚙

Description

SubstituteHelper.ClearPendingArgSpecs() calls into NSubstitute.Core ThreadContext internals,
coupling the tests to non-public/stable APIs and risking breakage on NSubstitute upgrades. Prefer
eliminating matcher leaks at the source (no Arg.* outside substitute calls) and keep any required
workaround clearly isolated/documented with version constraints.

Code

code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[R16-19]
+    public static void ClearPendingArgSpecs()
+    {
+        SubstitutionContext.Current.ThreadContext.DequeueAllArgumentSpecifications();
+    }

Relevance

⭐⭐ Medium
Using NSubstitute.Core ThreadContext internals is brittle, but team may accept workaround during
Moq→NSubstitute migration.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper directly references NSubstitute.Core and invokes
ThreadContext.DequeueAllArgumentSpecifications(), which is an implementation-detail surface (Core
namespace) and more likely to change than public Substitute/Arg APIs.

code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[1-19]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Tests now depend on `NSubstitute.Core` internals (`SubstitutionContext.Current.ThreadContext...`) to clear pending argument specs. This is brittle across NSubstitute upgrades.
### Issue Context
The helper exists to mitigate leaked argument specs. The best long-term fix is to remove the patterns that create leaks (e.g., using `Arg.Any<T>()` outside substitute configuration/verification). If the helper must remain, it should be clearly documented as version-coupled and kept narrowly scoped.
### Fix Focus Areas
- code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[1-19]
- code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs[100-110]
### Suggested change
1. Fix any matcher-leak sources (start with `ArrClientFactoryTests`).
2. Re-evaluate whether `SubstituteHelper.ClearPendingArgSpecs()` is still necessary everywhere.
3. If it remains necessary, add explicit documentation/comments about the NSubstitute version compatibility and keep usage centralized to fixture construction only.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. ClearPendingArgSpecs uses abbreviation 📘 ✧

Description

The newly introduced helper method name ClearPendingArgSpecs uses the non-obvious abbreviation
ArgSpecs, which violates the requirement to avoid ambiguous abbreviations in identifiers. This
reduces readability/maintainability and propagates the abbreviation into multiple call sites.

Code

code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[22]
+    public static void ClearPendingArgSpecs()

Relevance

⭐ Low
ArgSpecs is a well-known NSubstitute term; renaming is likely seen as pedantic for a test helper.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 225597 disallows ambiguous/non-standard abbreviations in identifiers unless
explicitly approved. The PR introduces ClearPendingArgSpecs and uses it in fixtures, spreading the
abbreviation into the changed code.
Rule 225597: Avoid ambiguous abbreviations in identifiers
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[22-22]
code/backend/Cleanuparr.Infrastructure.Tests/Features/DownloadClient/DelugeServiceFixture.cs[33-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ClearPendingArgSpecs` introduces the ambiguous abbreviation `ArgSpecs` in a public helper API and propagates it to multiple test fixtures.
## Issue Context
Compliance requires avoiding non-obvious abbreviations in identifiers unless explicitly documented/approved.
## Fix Focus Areas
- code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs[22-22]
- code/backend/Cleanuparr.Infrastructure.Tests/Features/DownloadClient/DelugeServiceFixture.cs[33-33]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

greptile-apps · 2026-04-16T07:17:29Z

Greptile Summary

This PR completes the migration from Moq to NSubstitute across all 68 test files in Cleanuparr.Infrastructure.Tests, in line with the project's stated convention of using NSubstitute for all new mocking. Moq is removed from the .csproj, and three new shared test-helper files are introduced: FakeHttpMessageHandler (a real HttpMessageHandler subclass, since NSubstitute cannot mock protected methods), LoggerVerificationExtensions (for asserting on ILogger<T> calls via NSubstitute's ReceivedCalls() API), and SubstituteHelper (to clear leaked arg-specs that can cause AmbiguousArgumentsException in xUnit's shared-fixture pattern).

Confidence Score: 5/5

Safe to merge — test-only change, no production code modified, migration is correct and complete.

All 68 changed files are in the test project. Moq is cleanly removed, NSubstitute patterns are used correctly throughout (Arg.Any, ArgAt, Received, ThrowsAsync, ReturnsForAnyArgs). The three new helper classes are well-designed. The only findings are P2 style suggestions: the DynamicInvoke exception-wrapping concern and the LoggerVerificationExtensions argument-guard suggestion. Neither blocks merge.

No files require special attention. The DynamicInvoke pattern in QBitServiceFixture (and its siblings) and the LoggerVerificationExtensions argument check are minor style suggestions only.

Important Files Changed

Filename	Overview
code/backend/Cleanuparr.Infrastructure.Tests/Cleanuparr.Infrastructure.Tests.csproj	Removes Moq 4.20.72 package reference; NSubstitute 5.3.0 was already present and remains.
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/FakeHttpMessageHandler.cs	New file: concrete HttpMessageHandler subclass replacing Moq for HTTP-layer tests; well-designed with request capture and configurable response/throw delegates.
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/LoggerVerificationExtensions.cs	New file: extension methods for verifying ILogger calls via NSubstitute ReceivedCalls(); argument index [2] (TState) correctly targets the formatted log message, but the split Length guards could be consolidated for clarity.
code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs	New file: clears leaked NSubstitute arg-specs via DequeueAllArgumentSpecifications() to prevent AmbiguousArgumentsException in xUnit shared-fixture pattern; uses a stable interface method, well-documented.
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/TestHelpers/JobHandlerFixture.cs	Migrated to NSubstitute; calls SubstituteHelper.ClearPendingArgSpecs() in constructor and ResetMocks() to prevent leaked arg-specs across shared xUnit fixtures.
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/Integration/IntegrationTestFixture.cs	New integration fixture wiring real services (EventPublisher, Striker, QueueItemRemover) with NSubstitute boundaries; DryRunInterceptor.InterceptAsync returns Task.CompletedTask (action not invoked), intentionally differing from unit-test fixtures that execute the delegate.
code/backend/Cleanuparr.Infrastructure.Tests/Features/DownloadClient/QBitServiceFixture.cs	Migrated to NSubstitute; DryRunInterceptor.InterceptAsync executes delegate via DynamicInvoke, with a minor caveat that synchronous delegate exceptions are wrapped in TargetInvocationException.
code/backend/Cleanuparr.Infrastructure.Tests/Features/BlacklistSync/BlacklistSynchronizerTests.cs	Uses FakeHttpMessageHandler for HTTP mocking and DynamicInvoke for interceptor; has a safer Task-type-check pattern compared to other fixtures.
code/backend/Cleanuparr.Infrastructure.Tests/Features/Jobs/MalwareBlockerTests.cs	Fully migrated to NSubstitute; uses Arg.Any(), ArgAt(), Received(), ThrowsAsync() correctly throughout.
code/backend/Cleanuparr.Infrastructure.Tests/Events/EventPublisherTests.cs	Migrated to NSubstitute; uses in-memory DB for real EventsContext and NSubstitute for hub/notification dependencies; covers DB persistence, SignalR, dry-run, and complex data scenarios.
code/backend/Cleanuparr.Infrastructure.Tests/Features/DownloadRemover/QueueItemRemoverTests.cs	Migrated; DryRunInterceptor.InterceptAsync setup returns Task.CompletedTask (action not run), intentional — QueueItemRemover calls DeleteQueueItemAsync directly, not through the interceptor.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Test File] --> B{Needs HTTP mocking?}
    B -->|Yes| C[FakeHttpMessageHandler\nCaptures requests\nConfigurable responses]
    B -->|No| D{Needs ILogger\nverification?}
    D -->|Yes| E[LoggerVerificationExtensions\nReceivedLogContaining\nDidNotReceiveLogContaining]
    D -->|No| F{Shared xUnit\nfixture?}
    F -->|Yes| G[SubstituteHelper.ClearPendingArgSpecs\nin fixture constructor]
    F -->|No| H[Standard NSubstitute\nSubstitute.For / Arg.Any\nReceived / ThrowsAsync]
    G --> H
    C --> H
    E --> H
    H --> I{DryRunInterceptor\nneeded?}
    I -->|Execute delegate\nunit tests| J[DynamicInvoke pattern\nQBit/Deluge/etc. fixtures]
    I -->|Skip action\nintegration tests| K[ReturnsForAnyArgs\nTask.CompletedTask]

_{Reviews (2): Last reviewed commit: "fixed a few more things" | Re-trigger Greptile}

codecov · 2026-04-16T07:28:46Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Flaminel · 2026-04-16T08:35:43Z

/review

qodo-code-review · 2026-04-16T08:35:51Z

Persistent review updated to latest commit 70b332a

Flaminel · 2026-04-16T08:50:03Z

@greptileai review

replaced Moq with NSubstitute for unit tests

2bd467a

greptile-apps Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread code/backend/Cleanuparr.Infrastructure.Tests/TestHelpers/SubstituteHelper.cs

qodo-code-review Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs Outdated

Comment thread code/backend/Cleanuparr.Infrastructure.Tests/Features/Arr/ArrClientFactoryTests.cs Outdated

fixed a few more things

70b332a

Flaminel merged commit ee5e7c0 into main Apr 16, 2026
13 checks passed

Flaminel deleted the replace_moq branch April 16, 2026 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace Moq with NSubstitute for unit tests#566

Replace Moq with NSubstitute for unit tests#566
Flaminel merged 2 commits into
mainfrom
replace_moq

Flaminel commented Apr 16, 2026

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

qodo-code-review Bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 16, 2026

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

qodo-code-review Bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Flaminel commented Apr 16, 2026

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

qodo-code-review Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

greptile-apps Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 16, 2026

Codecov Report

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

qodo-code-review Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flaminel commented Apr 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented Apr 16, 2026 •

edited

Loading

greptile-apps Bot commented Apr 16, 2026 •

edited

Loading

qodo-code-review Bot commented Apr 16, 2026 •

edited

Loading