Remove reflection-based non-public method testing#1167
Conversation
…TypeResolverTests Delete IsValueType_Returns_True_For_All_Known_Mappings (47 args) and IsValueType_Returns_False_For_Reference_Types (47 args). Both tested the private static method directly via reflection. All code paths are already covered by the existing public Resolve() tests in the same file.
Delete 3 tests that called the private MergeIfMissingOrThrowOnConflict method via reflection. The same scenarios (equivalent duplicates, collisions, composed schemas) are already covered by tests using the public InvokeMerge helper which calls Merger.Merge() internally.
All 10 tests used RuntimeHelpers.GetUninitializedObject + BindingFlags to construct NSwag type instances. The form-data code paths are exercised by existing scenario integration tests (IdentifierCorrectnessTests, MultiPartFormDataTests, FormDataParameterCasingTests).
…ivateCoverageTests Delete 15 tests that used reflection-based CreateParameterModel/CreateOperationModel helpers (RuntimeHelpers.GetUninitializedObject + BindingFlags) to construct NSwag types. Also remove the helper methods and unused reflection imports. Keep 20 tests that test ParameterShared utility methods directly via InternalsVisibleTo using only NJsonSchema types and simple objects.
…ration test Remove GetDefaultValueForParameter_Uses_Sanitized_Variable_Name_Fallback which used RuntimeHelpers.GetUninitializedObject + BindingFlags to construct NSwag types. Replace with two integration tests that exercise the same fallback path via a real OpenAPI spec using parameter name 'filter#tag' (NSwag doesn't sanitize #, so VariableName mismatch triggers fallback). Also remove 5 unused imports (System.Reflection, System.Runtime.CompilerServices, NJsonSchema, NSwag, NSwag.CodeGeneration.CSharp.Models).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReflection-based test helpers and all test methods depending on them are deleted from three test files. In ChangesTest Suite Reflection Removal and Integration Replacement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/Refitter.Tests/Scenarios/IdentifierCorrectnessTests.cs`:
- Around line 643-675: The test only validates the unsanitized parameter name
scenario for OpenAPI 3.0, but the testing guidelines require coverage for both
OpenAPI 2.0 and 3.0 specifications. Add a Swagger 2.0 spec constant (similar to
SpecWithUnsanitizedParameterName but in Swagger 2.0 format with appropriate
parameter definition structure) and create a corresponding test method that
validates the same unsanitized parameter name behavior for Swagger 2.0,
including compile checks and assertions equivalent to the OpenAPI 3.0 test
variant.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c954b891-530e-4a80-be8f-997504a288ff
📒 Files selected for processing (5)
src/Refitter.Tests/CustomCSharpTypeResolverTests.cssrc/Refitter.Tests/FormParameterExtractorTests.cssrc/Refitter.Tests/OpenApi/OpenApiDocumentFactoryMergeTests.cssrc/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cssrc/Refitter.Tests/Scenarios/IdentifierCorrectnessTests.cs
💤 Files with no reviewable changes (4)
- src/Refitter.Tests/FormParameterExtractorTests.cs
- src/Refitter.Tests/CustomCSharpTypeResolverTests.cs
- src/Refitter.Tests/OpenApi/OpenApiDocumentFactoryMergeTests.cs
- src/Refitter.Tests/Parameters/ParameterExtractorPrivateCoverageTests.cs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
- Coverage 95.04% 94.82% -0.23%
==========================================
Files 80 80
Lines 3553 3553
==========================================
- Hits 3377 3369 -8
- Misses 68 75 +7
- Partials 108 109 +1
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:
|
christianhelle
changed the title
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
1 participant
Summary
Removes all tests that use .NET reflection (
BindingFlags.NonPublic,RuntimeHelpers.GetUninitializedObject) to access non-public members inRefitter.Core. Replaces them with public-API integration tests or deletes where coverage is already provided.Changes
Step 1:
CustomCSharpTypeResolverTests.cs— Delete 94 redundant reflection testsRemoved
IsValueType_Returns_True_For_All_Known_Mappings(47 args) andIsValueType_Returns_False_For_Reference_Types(47 args). Both tested aprivate staticmethod directly. All three code paths are already covered by the existing publicResolve()tests in the same file.Step 2:
OpenApiDocumentFactoryMergeTests.cs— Delete 3 reflection tests + helperRemoved 3 tests that called
MergeIfMissingOrThrowOnConflictvia reflection. The same scenarios (equivalent duplicates, collisions, composed schemas) are already covered by tests using the publicMerger.Merge()path.Step 3:
FormParameterExtractorTests.cs— Delete entire fileAll 10 tests used
RuntimeHelpers.GetUninitializedObject+BindingFlagsto construct NSwag types. The form-data code paths are exercised by existing scenario integration tests.Step 4:
ParameterExtractorPrivateCoverageTests.cs— Delete reflection-dependent testsRemoved 15 tests that depended on reflection-based
CreateParameterModel/CreateOperationModelhelpers. Kept 20 tests that testParameterSharedutility methods viaInternalsVisibleTowithout reflection.Step 5:
IdentifierCorrectnessTests.cs— Replace with integration testRemoved reflection test that manually constructed
CSharpParameterModelviaRuntimeHelpers.GetUninitializedObject. Replaced with two integration tests using a real OpenAPI spec with parameter namefilter#tag(NSwag doesn't sanitize#, triggering the same fallback).Verification
dotnet build -c Release— 0 warnings, 0 errorsdotnet test --project src/Refitter.Tests/Refitter.Tests.csproj -c Release— 2188 passed, 0 failedSummary by CodeRabbit
FormParameterExtractorTestssuite and trimmed other parameter and OpenAPI merge test coverage.