Phase 1 (#159): add DotNetWorkQueue.Tests.Shared assertion helper#170
Conversation
…er in sln Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…AreEquivalent Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…istration Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a new ChangesShared Test AssertHelper Utility
Estimated code review effort: 2 (Simple) | ~15 minutes Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Source/DotNetWorkQueue.Tests.Shared/AssertHelper.cs (2)
84-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AllSatisfydiscards the original exception's type and stack trace.The catch block converts every exception into an
Assert.Failmessage string, losing the original exception's type and stack trace. This applies uniformly to genuine assertion failures and to unrelated bugs in the delegate (e.g. aNullReferenceException), making failures harder to diagnose from the test output alone. Preserving the caught exception as the inner exception keeps the index/value context while retaining full diagnostic info.🐛 Proposed fix
catch (Exception ex) { - Assert.Fail($"AllSatisfy failed for element at index {index} (value: {item}): {ex.Message}"); + throw new AssertFailedException( + $"AllSatisfy failed for element at index {index} (value: {item}): {ex.Message}", ex); }🤖 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 `@Source/DotNetWorkQueue.Tests.Shared/AssertHelper.cs` around lines 84 - 97, The AllSatisfy helper in AssertHelper currently swallows the original exception details by turning every failure into a plain Assert.Fail message. Update the catch block inside AllSatisfy to preserve the caught exception as the underlying/inner exception while still reporting the index and value context, so failures from both assertion mismatches and unexpected bugs retain their original type and stack trace.
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tolerance-check logic between the two
AreCloseoverloads.Both overloads compute a delta and run the identical
Math.Abs(delta.Ticks) > tolerance.Tickscheck with the same message format. Consider extracting a shared private helper that operates on the computedTimeSpandelta to avoid maintaining two copies of the same logic.♻️ Suggested refactor
public static void AreClose(DateTime expected, DateTime actual, TimeSpan tolerance) { - var delta = actual - expected; - if (Math.Abs(delta.Ticks) > tolerance.Ticks) - { - Assert.Fail($"Expected {actual:O} to be within {tolerance} of {expected:O}, but the difference was {delta}."); - } + AssertWithinTolerance(actual - expected, tolerance, expected.ToString("O"), actual.ToString("O")); } ... public static void AreClose(DateTimeOffset expected, DateTimeOffset actual, TimeSpan tolerance) { - var delta = actual - expected; - if (Math.Abs(delta.Ticks) > tolerance.Ticks) - { - Assert.Fail($"Expected {actual:O} to be within {tolerance} of {expected:O}, but the difference was {delta}."); - } + AssertWithinTolerance(actual - expected, tolerance, expected.ToString("O"), actual.ToString("O")); } + + private static void AssertWithinTolerance(TimeSpan delta, TimeSpan tolerance, string expectedFormatted, string actualFormatted) + { + if (Math.Abs(delta.Ticks) > tolerance.Ticks) + { + Assert.Fail($"Expected {actualFormatted} to be within {tolerance} of {expectedFormatted}, but the difference was {delta}."); + } + }Also applies to: 56-63
🤖 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 `@Source/DotNetWorkQueue.Tests.Shared/AssertHelper.cs` around lines 40 - 47, Extract the duplicated tolerance check in AssertHelper.AreClose overloads into a shared private helper that takes the computed TimeSpan delta plus the expected/actual values needed for the failure message. Update both AreClose methods to calculate their delta and delegate the Math.Abs(delta.Ticks) > tolerance.Ticks check and Assert.Fail formatting to that helper, so the logic is maintained in one place across the DateTime and the other AreClose overload.Source/DotNetWorkQueue.Tests.Shared.Tests/AssertHelperTests.cs (1)
97-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for
AllSatisfy's null-argument guards.
AssertHelper.AllSatisfythrowsArgumentNullExceptionfor a nullcollectionor nullassertion(AssertHelper.cs lines 74-82), but this region has no tests exercising either path. Since this is a shared public helper other test suites will depend on, adding these two cases would lock in the contract.🤖 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 `@Source/DotNetWorkQueue.Tests.Shared.Tests/AssertHelperTests.cs` around lines 97 - 119, Add two tests in the AllSatisfy region of AssertHelperTests to cover the null-argument guards in AssertHelper.AllSatisfy: one that passes a null collection and one that passes a null assertion delegate, both expecting ArgumentNullException. Use the existing AssertHelper.AllSatisfy test patterns so the contract is locked in for the shared 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 `@Source/DotNetWorkQueue.Tests.Shared.Tests/AssertHelperTests.cs`:
- Around line 97-119: Add two tests in the AllSatisfy region of
AssertHelperTests to cover the null-argument guards in AssertHelper.AllSatisfy:
one that passes a null collection and one that passes a null assertion delegate,
both expecting ArgumentNullException. Use the existing AssertHelper.AllSatisfy
test patterns so the contract is locked in for the shared helper.
In `@Source/DotNetWorkQueue.Tests.Shared/AssertHelper.cs`:
- Around line 84-97: The AllSatisfy helper in AssertHelper currently swallows
the original exception details by turning every failure into a plain Assert.Fail
message. Update the catch block inside AllSatisfy to preserve the caught
exception as the underlying/inner exception while still reporting the index and
value context, so failures from both assertion mismatches and unexpected bugs
retain their original type and stack trace.
- Around line 40-47: Extract the duplicated tolerance check in
AssertHelper.AreClose overloads into a shared private helper that takes the
computed TimeSpan delta plus the expected/actual values needed for the failure
message. Update both AreClose methods to calculate their delta and delegate the
Math.Abs(delta.Ticks) > tolerance.Ticks check and Assert.Fail formatting to that
helper, so the logic is maintained in one place across the DateTime and the
other AreClose overload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f78b614f-c578-4ef1-b0c2-b26d003162ef
📒 Files selected for processing (5)
Source/DotNetWorkQueue.Tests.Shared.Tests/AssertHelperTests.csSource/DotNetWorkQueue.Tests.Shared.Tests/DotNetWorkQueue.Tests.Shared.Tests.csprojSource/DotNetWorkQueue.Tests.Shared/AssertHelper.csSource/DotNetWorkQueue.Tests.Shared/DotNetWorkQueue.Tests.Shared.csprojSource/DotNetWorkQueue.sln
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #170 +/- ##
==========================================
+ Coverage 87.48% 87.54% +0.05%
==========================================
Files 1023 1023
Lines 33863 33863
Branches 2864 2864
==========================================
+ Hits 29625 29645 +20
+ Misses 3361 3345 -16
+ Partials 877 873 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Phase 1 of #159 — FluentAssertions → MSTest migration foundation
First increment of #159 (replace FluentAssertions with built-in MSTest assertions). Test-only, no production code, no version bump. This phase adds the shared assertion helper that later per-project migration PRs depend on — it performs no migration itself.
What's here
DotNetWorkQueue.Tests.Shared— a plain multi-target (net10.0;net8.0) class library (deliberately not a test-SDK project, to avoid double test discovery). References onlyMSTest.TestFramework+CompareNETObjects(both already pinned). ExposesAssertHelper:AreClose(DateTime/DateTimeOffset, TimeSpan tolerance)— for the datetimeBeCloseTosites.AllSatisfy<T>(IEnumerable<T>, Action<T>)— runs an assertion per element, reports the failing index.AreEquivalent<T>(expected, actual, bool ignoreCollectionOrder = true)— object-graph equivalence via CompareNETObjects, surfacingDifferencesStringon mismatch.DotNetWorkQueue.Tests.Shared.Tests— MSTest SDK project with 12 tests proving pass + fail paths for every helper (including tolerance boundary,AllSatisfyfailing-index message, and abyte[]negative-control that proves byte-order significance is preserved whenignoreCollectionOrder:false). Fail paths useAssert.ThrowsExactly<AssertFailedException>(MSTest 4.x removedThrowsException).DotNetWorkQueue.slnonly.Verification
TreatWarningsAsErrors.dotnet test→ 12/12 pass on both TFMs.FluentAssertions(6.12.2) untouched inDirectory.Packages.props; no existing test project migrated; no production assembly changed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores