Add test coverage for prerendering closed generic components#20
Conversation
Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
WalkthroughThese changes add comprehensive test coverage for closed generic components (like Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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
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: 2
🤖 Fix all issues with AI agents
In `@src/Components/Endpoints/test/EndpointHtmlRendererTest.cs`:
- Around line 837-838: The assertions check the wrong generic instantiation:
update the two assertions that reference typeof(GenericComponent<string>) to
match the rendered component GenericComponent<int> by using
typeof(GenericComponent<int>) for both the Assembly and FullName comparisons
against preambleMarker; ensure the symbol preambleMarker remains used and only
the typeof(...) generic type argument is changed so the test verifies the actual
rendered type.
In `@src/Components/Server/test/Circuits/ServerComponentDeserializerTest.cs`:
- Around line 142-144: The assertion for the second descriptor's sequence is
wrong: update the test in ServerComponentDeserializerTest (the
CanParseMultipleMarkers test that inspects descriptors[1] / secondDescriptor) to
expect Sequence = 1 instead of 0 to match how CreateMarkers increments sequences
(or the marker creation logic that sets sequence numbers); change the assertion
for secondDescriptor.Sequence accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Components/Endpoints/test/EndpointHtmlRendererTest.cssrc/Components/Endpoints/test/TestComponents/GenericComponent.razorsrc/Components/Server/test/Circuits/ServerComponentDeserializerTest.cs
🔇 Additional comments (8)
src/Components/Endpoints/test/TestComponents/GenericComponent.razor (1)
1-6: LGTM!Clean implementation of a generic test component with proper null handling.
src/Components/Endpoints/test/EndpointHtmlRendererTest.cs (3)
721-736: LGTM!Test correctly validates rendering a closed generic component with an
inttype parameter.
738-775: LGTM!Test correctly validates server-mode rendering of a closed generic component with comprehensive marker and descriptor assertions.
777-814: LGTM!Test correctly validates server-mode prerendering of a closed generic component with proper preamble/epilogue marker verification.
src/Components/Server/test/Circuits/ServerComponentDeserializerTest.cs (4)
4-4: LGTM!Import correctly added to support
CultureInfo.InvariantCultureusage in the new test.
79-105: LGTM!Tests correctly validate parsing markers for closed generic components with both value type (
int) and reference type (string) type parameters.
107-125: LGTM!Test correctly validates parameter parsing for closed generic components. The use of
Convert.ToInt64appropriately handles potential numeric type variations from JSON deserialization.
590-596: LGTM!Well-structured generic test component following the existing
TestComponentpattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| Assert.Equal(typeof(GenericComponent<string>).Assembly.GetName().Name, preambleMarker.Assembly); | ||
| Assert.Equal(typeof(GenericComponent<string>).FullName, preambleMarker.TypeName); |
There was a problem hiding this comment.
Type mismatch between rendered component and assertions.
The test renders GenericComponent<int> (line 825) but asserts against GenericComponent<string> (lines 837-838). This is incorrect and will cause false test results.
🐛 Proposed fix
- Assert.Equal(typeof(GenericComponent<string>).Assembly.GetName().Name, preambleMarker.Assembly);
- Assert.Equal(typeof(GenericComponent<string>).FullName, preambleMarker.TypeName);
+ Assert.Equal(typeof(GenericComponent<int>).Assembly.GetName().Name, preambleMarker.Assembly);
+ Assert.Equal(typeof(GenericComponent<int>).FullName, preambleMarker.TypeName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Assert.Equal(typeof(GenericComponent<string>).Assembly.GetName().Name, preambleMarker.Assembly); | |
| Assert.Equal(typeof(GenericComponent<string>).FullName, preambleMarker.TypeName); | |
| Assert.Equal(typeof(GenericComponent<int>).Assembly.GetName().Name, preambleMarker.Assembly); | |
| Assert.Equal(typeof(GenericComponent<int>).FullName, preambleMarker.TypeName); |
🤖 Prompt for AI Agents
In `@src/Components/Endpoints/test/EndpointHtmlRendererTest.cs` around lines 837 -
838, The assertions check the wrong generic instantiation: update the two
assertions that reference typeof(GenericComponent<string>) to match the rendered
component GenericComponent<int> by using typeof(GenericComponent<int>) for both
the Assembly and FullName comparisons against preambleMarker; ensure the symbol
preambleMarker remains used and only the typeof(...) generic type argument is
changed so the test verifies the actual rendered type.
| var secondDescriptor = descriptors[1]; | ||
| Assert.Equal(typeof(GenericTestComponent<string>).FullName, secondDescriptor.ComponentType.FullName); | ||
| Assert.Equal(0, secondDescriptor.Sequence); |
There was a problem hiding this comment.
Incorrect sequence assertion for second descriptor.
The existing CanParseMultipleMarkers test (lines 162-164) correctly asserts that the second descriptor has Sequence = 1. However, this new test asserts Sequence = 0 for the second descriptor, which appears incorrect based on how CreateMarkers increments sequences.
🐛 Proposed fix
var secondDescriptor = descriptors[1];
Assert.Equal(typeof(GenericTestComponent<string>).FullName, secondDescriptor.ComponentType.FullName);
- Assert.Equal(0, secondDescriptor.Sequence);
+ Assert.Equal(1, secondDescriptor.Sequence);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var secondDescriptor = descriptors[1]; | |
| Assert.Equal(typeof(GenericTestComponent<string>).FullName, secondDescriptor.ComponentType.FullName); | |
| Assert.Equal(0, secondDescriptor.Sequence); | |
| var secondDescriptor = descriptors[1]; | |
| Assert.Equal(typeof(GenericTestComponent<string>).FullName, secondDescriptor.ComponentType.FullName); | |
| Assert.Equal(1, secondDescriptor.Sequence); |
🤖 Prompt for AI Agents
In `@src/Components/Server/test/Circuits/ServerComponentDeserializerTest.cs`
around lines 142 - 144, The assertion for the second descriptor's sequence is
wrong: update the test in ServerComponentDeserializerTest (the
CanParseMultipleMarkers test that inspects descriptors[1] / secondDescriptor) to
expect Sequence = 1 instead of 0 to match how CreateMarkers increments sequences
(or the marker creation logic that sets sequence numbers); change the assertion
for secondDescriptor.Sequence accordingly.
Benchmark PR from qodo-benchmark#86
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.