Improve code coverage#1132
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds comprehensive test coverage across multiple test files: new suites for FormParameterExtractor and RichGenerationReporter; expanded tests for GenerationOrchestrator, OutputPlanner, and ParameterExtractor private helpers; plus a docs update switching examples to TUnit. All changes are tests or docs only. ChangesTest Coverage Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1132 +/- ##
==========================================
+ Coverage 86.00% 94.20% +8.20%
==========================================
Files 39 39
Lines 2658 2658
==========================================
+ Hits 2286 2504 +218
+ Misses 271 58 -213
+ Partials 101 96 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs (3)
409-419: ⚡ Quick winStrengthen GetQueryAttribute test assertion.
The test at line 418 only verifies that the result is not null, which provides minimal coverage. Based on the context snippet showing
ParameterShared.GetQueryAttributereturns various query attribute formats, the test should verify the actual attribute string.💡 Suggested improvement
[Test] public void GetQueryAttribute_Returns_Query_Attribute() { var param = CreateParameterModel("q", "q"); var result = InvokePrivate<string>( "GetQueryAttribute", [typeof(CSharpParameterModel), typeof(RefitGeneratorSettings)], param, new RefitGeneratorSettings()); - result.Should().NotBeNull(); + result.Should().Be("Query"); }🤖 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/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs` around lines 409 - 419, The test GetQueryAttribute_Returns_Query_Attribute currently only asserts result is not null; change it to assert the exact expected query attribute string returned by GetQueryAttribute using the same setup (use CreateParameterModel("q","q") and InvokePrivate on "GetQueryAttribute" with types [typeof(CSharpParameterModel), typeof(RefitGeneratorSettings)] and new RefitGeneratorSettings()); replace the NotBeNull() check with an assertion that result equals the expected string (the exact value produced by ParameterShared.GetQueryAttribute for this input) so the test validates the attribute content rather than just non-nullity.
469-479: 💤 Low valueStrengthen GetQueryParameterType test assertion.
The test at line 478 only verifies that the result is not null. The test should verify the actual parameter type string returned.
💡 Suggested improvement
[Test] public void GetQueryParameterType_Returns_Query_Parameter_Type() { var param = CreateParameterModel("q", "q", "string"); var result = InvokePrivate<string>( "GetQueryParameterType", [typeof(ParameterModelBase), typeof(RefitGeneratorSettings)], param, new RefitGeneratorSettings()); - result.Should().NotBeNull(); + result.Should().Be("string"); }🤖 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/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs` around lines 469 - 479, The test GetQueryParameterType_Returns_Query_Parameter_Type currently only asserts result.Should().NotBeNull(); change it to assert the actual expected type string returned by GetQueryParameterType: after invoking InvokePrivate on GetQueryParameterType (using the created ParameterModel "q" with type "string"), replace the null check with an equality assertion such as result.Should().Be("string") (or the exact expected type string the method should return), so the test verifies the precise returned parameter type.
325-347: ⚡ Quick winStrengthen test assertions to verify actual behavior.
These tests currently only verify that methods return non-null values, which provides minimal coverage:
- Line 332:
ReplaceUnsafeCharactersshould assert the actual character transformation (e.g.,"unsafe-name!"→"unsafe_name_")- Line 346:
ReOrderNullableParametersshould verify that nullable parameters are actually reordered to the end of the list💡 Suggested assertion improvements
[Test] public void ReplaceUnsafeCharacters_Delegates_To_ParameterShared() { var result = InvokePrivate<string>( "ReplaceUnsafeCharacters", [typeof(string)], "unsafe-name!"); - result.Should().NotBeNullOrWhiteSpace(); + result.Should().Be("unsafe_name_"); } [Test] public void ReOrderNullableParameters_Delegates_To_OptionalParameterReorderer() { var parameterModels = new List<CSharpParameterModel>(); var result = InvokePrivate<List<string>>( "ReOrderNullableParameters", [typeof(List<string>), typeof(RefitGeneratorSettings), typeof(ICollection<CSharpParameterModel>)], new List<string> { "string a", "int? b = default" }, new RefitGeneratorSettings(), parameterModels); - result.Should().NotBeNull(); + result.Should().HaveCount(2); + result[0].Should().Be("string a"); + result[1].Should().Be("int? b = default"); }🤖 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/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs` around lines 325 - 347, The tests only assert non-null results; strengthen them to assert expected transformations and ordering: in the ReplaceUnsafeCharacters test (calling private method via InvokePrivate on "ReplaceUnsafeCharacters") assert that invoking with "unsafe-name!" returns the expected sanitized string (e.g., "unsafe_name_") rather than just NotBeNullOrWhiteSpace; in the ReOrderNullableParameters test (invoking "ReOrderNullableParameters" via InvokePrivate with the sample parameter list and parameterModels) assert that nullable parameters (e.g., "int? b = default") are moved to the end of the returned list and that the relative order of non-nullable parameters is preserved, instead of only checking NotBeNull; update the assertions to compare the actual returned List<string> to the expected ordering and values.src/Refitter.Tests/FormParameterExtractorTests.cs (1)
199-253: 💤 Low valueReflection-based test helpers are fragile.
The helper methods use
RuntimeHelpers.GetUninitializedObjectand access compiler-generated private backing fields. While this pattern works for testing NSwag's code generation models that lack public constructors, it creates maintenance risk:
- Compiler-generated field names like
"<Parameters>k__BackingField"may change- Bypasses any constructor validation logic
- Could break with NSwag library updates
Consider adding a comment documenting this fragility and the reason for using reflection (lack of public constructors).
🤖 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/Refitter.Tests/FormParameterExtractorTests.cs` around lines 199 - 253, The test helpers CreateEmptyOperationModel, CreateOperationModel and CreateFormDataParameterModel rely on RuntimeHelpers.GetUninitializedObject and private compiler-generated backing fields, which is fragile; add a concise comment above each helper explaining why reflection/uninitialized objects are used (no public constructors on NSwag models), the risks (compiler-generated field names and bypassed constructor logic), and a note to revisit if NSwag exposes public constructors or the backing field names change so future maintainers understand and can safely replace this approach.src/Refitter.Tests/GenerationOrchestratorTests.cs (2)
545-558: ⚡ Quick winUse more precise assertion patterns for boundary file size tests.
The wildcard patterns are too loose. Use exact values:
- justUnder1K.Should().Match("1023* B"); + justUnder1K.Should().Be("1023.0 B"); - exactly1K.Should().Match("1* KB"); + exactly1K.Should().Be("1.0 KB"); - justOver1K.Should().Match("1* KB"); + justOver1K.Should().Be("1.0 KB"); - justUnder1M.Should().Match("1024* KB"); + justUnder1M.Should().Be("1024.0 KB"); - exactly1M.Should().Match("1* MB"); + exactly1M.Should().Be("1.0 MB");🤖 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/Refitter.Tests/GenerationOrchestratorTests.cs` around lines 545 - 558, The assertions using wildcard Match patterns in GenerationOrchestratorTests (variables justUnder1K, exactly1K, justOver1K, justUnder1M, exactly1M) are too loose; update them to assert the exact expected strings instead of wildcards: assert justUnder1K equals "1023 B", exactly1K equals "1 KB", justOver1K equals "1 KB", justUnder1M equals "1024 KB", and exactly1M equals "1 MB" (use exact equality/assertion method rather than Match with wildcards).
520-530: ⚡ Quick winUse more precise assertion patterns for formatted file sizes.
The wildcard patterns like
"1* KB"are too loose and could match unintended values (e.g.,"12.3 KB"). Based on the FormatFileSize implementation (formats with one decimal place), use exact or more specific patterns:- bytesResult.Should().Match("0* B"); + bytesResult.Should().Be("0.0 B"); - kbResult.Should().Match("1* KB"); + kbResult.Should().Be("1.0 KB"); - mbResult.Should().Match("1* MB"); + mbResult.Should().Be("1.0 MB"); - gbResult.Should().Match("1* GB"); + gbResult.Should().Be("1.0 GB");🤖 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/Refitter.Tests/GenerationOrchestratorTests.cs` around lines 520 - 530, The assertions use loose wildcard patterns; update them to exact one-decimal-place checks to match FormatFileSize's output. For the invoked method (variable method), replace Match("0* B") with MatchRegex(@"^0\.0 B$") and replace Match("1* KB"/"1* MB"/"1* GB") with MatchRegex(@"^1\.0 KB$"), MatchRegex(@"^1\.0 MB$"), and MatchRegex(@"^1\.0 GB$") respectively so the tests assert an exact single-decimal formatted value (use method.Invoke as-is, only change the assertion patterns).
🤖 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/Refitter.Tests/FormParameterExtractorTests.cs`:
- Line 13: The project uses TUnit v1.47.0 and the test file
FormParameterExtractorTests.cs uses TUnit's [Test] attribute, but the coding
guidelines still reference xUnit's [Fact]; update the coding guidelines and any
test examples to adopt TUnit conventions: replace references to "[Fact]" with
"[Test]", show usage examples matching the [Test] attribute (including any
setup/teardown idioms if applicable), and note that the project uses TUnit
v1.47.0 so tests should follow TUnit's attribute and style (no code changes to
FormParameterExtractorTests.cs are required).
In `@src/Refitter.Tests/GenerationOrchestratorTests.cs`:
- Line 186: The test name
RunAsync_With_NoBanner_True_Still_Completes_Successfully contradicts its setup
(it sets NoBanner = false); rename the test method to
RunAsync_With_NoBanner_False_Still_Completes_Successfully and update the method
declaration and any references/usages (e.g., test display names or attributes)
to match the new name so the name accurately reflects the configuration being
tested.
- Around line 312-376: The test
RunAsync_With_UseIsoDateFormat_And_DateFormat_Shows_Warning is named to assert a
warning but only checks success; either rename it to reflect successful
completion or change the reporter to a spy and assert the warning call — create
or use a test reporter that implements the same contract as
SimpleGenerationReporter (or mock GenerationReporter) and verify that
ReportConfigurationWarnings or ReportAllPathsFilteredWarning on the reporter was
invoked with the expected arguments after calling
GenerationOrchestrator.RunAsync (reference the SimpleGenerationReporter and
GenerationOrchestrator.RunAsync names and the test method name when making the
change).
---
Nitpick comments:
In `@src/Refitter.Tests/FormParameterExtractorTests.cs`:
- Around line 199-253: The test helpers CreateEmptyOperationModel,
CreateOperationModel and CreateFormDataParameterModel rely on
RuntimeHelpers.GetUninitializedObject and private compiler-generated backing
fields, which is fragile; add a concise comment above each helper explaining why
reflection/uninitialized objects are used (no public constructors on NSwag
models), the risks (compiler-generated field names and bypassed constructor
logic), and a note to revisit if NSwag exposes public constructors or the
backing field names change so future maintainers understand and can safely
replace this approach.
In `@src/Refitter.Tests/GenerationOrchestratorTests.cs`:
- Around line 545-558: The assertions using wildcard Match patterns in
GenerationOrchestratorTests (variables justUnder1K, exactly1K, justOver1K,
justUnder1M, exactly1M) are too loose; update them to assert the exact expected
strings instead of wildcards: assert justUnder1K equals "1023 B", exactly1K
equals "1 KB", justOver1K equals "1 KB", justUnder1M equals "1024 KB", and
exactly1M equals "1 MB" (use exact equality/assertion method rather than Match
with wildcards).
- Around line 520-530: The assertions use loose wildcard patterns; update them
to exact one-decimal-place checks to match FormatFileSize's output. For the
invoked method (variable method), replace Match("0* B") with MatchRegex(@"^0\.0
B$") and replace Match("1* KB"/"1* MB"/"1* GB") with MatchRegex(@"^1\.0 KB$"),
MatchRegex(@"^1\.0 MB$"), and MatchRegex(@"^1\.0 GB$") respectively so the tests
assert an exact single-decimal formatted value (use method.Invoke as-is, only
change the assertion patterns).
In `@src/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs`:
- Around line 409-419: The test GetQueryAttribute_Returns_Query_Attribute
currently only asserts result is not null; change it to assert the exact
expected query attribute string returned by GetQueryAttribute using the same
setup (use CreateParameterModel("q","q") and InvokePrivate on
"GetQueryAttribute" with types [typeof(CSharpParameterModel),
typeof(RefitGeneratorSettings)] and new RefitGeneratorSettings()); replace the
NotBeNull() check with an assertion that result equals the expected string (the
exact value produced by ParameterShared.GetQueryAttribute for this input) so the
test validates the attribute content rather than just non-nullity.
- Around line 469-479: The test
GetQueryParameterType_Returns_Query_Parameter_Type currently only asserts
result.Should().NotBeNull(); change it to assert the actual expected type string
returned by GetQueryParameterType: after invoking InvokePrivate on
GetQueryParameterType (using the created ParameterModel "q" with type "string"),
replace the null check with an equality assertion such as
result.Should().Be("string") (or the exact expected type string the method
should return), so the test verifies the precise returned parameter type.
- Around line 325-347: The tests only assert non-null results; strengthen them
to assert expected transformations and ordering: in the ReplaceUnsafeCharacters
test (calling private method via InvokePrivate on "ReplaceUnsafeCharacters")
assert that invoking with "unsafe-name!" returns the expected sanitized string
(e.g., "unsafe_name_") rather than just NotBeNullOrWhiteSpace; in the
ReOrderNullableParameters test (invoking "ReOrderNullableParameters" via
InvokePrivate with the sample parameter list and parameterModels) assert that
nullable parameters (e.g., "int? b = default") are moved to the end of the
returned list and that the relative order of non-nullable parameters is
preserved, instead of only checking NotBeNull; update the assertions to compare
the actual returned List<string> to the expected ordering and values.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61faf943-933a-4276-a027-1c229494da88
📒 Files selected for processing (5)
src/Refitter.Tests/FormParameterExtractorTests.cssrc/Refitter.Tests/GenerationOrchestratorTests.cssrc/Refitter.Tests/OutputPlannerTests.cssrc/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cssrc/Refitter.Tests/RichGenerationReporterTests.cs
christianhelle
enabled auto-merge
June 13, 2026 15:25
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Refitter.Tests/GenerationOrchestratorTests.cs (1)
519-570: ⚡ Quick winExtract the duplicated reflection setup.
The reflection code to retrieve the
FormatFileSizemethod is duplicated betweenFormatFileSize_Small_Values(lines 522–527) andFormatFileSize_Exact_Boundaries(lines 547–552). Extract this to a private helper method or a cached field to eliminate duplication.♻️ Proposed refactor
+ private static MethodInfo GetFormatFileSizeMethod() => + typeof(GenerationOrchestrator).GetMethod( + "FormatFileSize", + BindingFlags.NonPublic | BindingFlags.Static, + null, + [typeof(long)], + null)!; + [Test] public void FormatFileSize_Small_Values() { - var method = typeof(GenerationOrchestrator).GetMethod( - "FormatFileSize", - BindingFlags.NonPublic | BindingFlags.Static, - null, - [typeof(long)], - null); - - method.Should().NotBeNull(); + var method = GetFormatFileSizeMethod(); var bytesResult = (string)method!.Invoke(null, [0L])!; bytesResult.Should().Match("0* B"); ... } [Test] public void FormatFileSize_Exact_Boundaries() { - var method = typeof(GenerationOrchestrator).GetMethod( - "FormatFileSize", - BindingFlags.NonPublic | BindingFlags.Static, - null, - [typeof(long)], - null); - - method.Should().NotBeNull(); + var method = GetFormatFileSizeMethod(); var justUnder1K = (string)method!.Invoke(null, [1023L])!; ... }🤖 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/Refitter.Tests/GenerationOrchestratorTests.cs` around lines 519 - 570, Extract the duplicated reflection logic that retrieves the non-public static MethodInfo for GenerationOrchestrator.FormatFileSize into a single private helper (e.g., GetFormatFileSizeMethod()) or a private static readonly cached MethodInfo field, and have both test methods FormatFileSize_Small_Values and FormatFileSize_Exact_Boundaries call that helper/field instead of repeating the BindingFlags.GetMethod block; ensure the helper returns a MethodInfo and the tests still assert method.Should().NotBeNull() before invoking it.
🤖 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 @.github/copilot-instructions.md:
- Line 114: Update the guidance line to reference the actual test namespaces
used in this repo instead of the stale Refitter.Tests.Scenarios; change it to
say that all new code must include unit tests following the pattern used in the
Refitter.Tests and Refitter.Tests.Parameters namespaces (or the primary test
namespace if only one is used) so readers are directed to the correct examples.
---
Nitpick comments:
In `@src/Refitter.Tests/GenerationOrchestratorTests.cs`:
- Around line 519-570: Extract the duplicated reflection logic that retrieves
the non-public static MethodInfo for GenerationOrchestrator.FormatFileSize into
a single private helper (e.g., GetFormatFileSizeMethod()) or a private static
readonly cached MethodInfo field, and have both test methods
FormatFileSize_Small_Values and FormatFileSize_Exact_Boundaries call that
helper/field instead of repeating the BindingFlags.GetMethod block; ensure the
helper returns a MethodInfo and the tests still assert
method.Should().NotBeNull() before invoking it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdc2b5a8-fad3-4ce8-a0e1-841ff491dbee
📒 Files selected for processing (2)
.github/copilot-instructions.mdsrc/Refitter.Tests/GenerationOrchestratorTests.cs
This was referenced Jul 12, 2026
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 by CodeRabbit