-
-
Notifications
You must be signed in to change notification settings - Fork 254
Add missing tests of BitButtonGroup component (#11646) #11647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing tests of BitButtonGroup component (#11646) #11647
Conversation
WalkthroughA new test class Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes This is a straightforward test addition with three well-defined test cases covering documented functionality. The tests follow standard patterns for component testing and validate existing features rather than introducing new logic. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/BlazorUI/Bit.BlazorUI.Tests/Components/Buttons/BitButtonGroupTests.cs (2)
48-51: Consider using more explicit button selection.While
Find(".bit-btg-itm")works, usingFindAll(".bit-btg-itm")[0]orFindAll("button")[0]would make it more explicit that you're testing the first button.Apply this diff for clarity:
- var btn = component.Find(".bit-btg-itm"); + var btn = component.FindAll(".bit-btg-itm")[0]; Assert.IsNotNull(btn);
80-82: Use exact assertion for toggled button count.In a single-toggle scenario, there should be exactly one toggled button. Using
>= 1is too permissive and might miss bugs where multiple buttons are incorrectly toggled.Apply this diff:
var toggled = comp.FindAll(".bit-btg-chk"); - Assert.IsTrue(toggled.Count >= 1); + Assert.AreEqual(1, toggled.Count); Assert.IsTrue(toggled[0].TextContent.Contains("B"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
src/BlazorUI/Bit.BlazorUI.Tests/Components/Buttons/BitButtonGroupTests.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build and test
🔇 Additional comments (2)
src/BlazorUI/Bit.BlazorUI.Tests/Components/Buttons/BitButtonGroupTests.cs (2)
10-28: LGTM! Clean rendering test.The test correctly verifies that items are rendered as buttons with the expected text content.
76-77: The original review comment is based on a misunderstanding of the component's intended behavior.The OnInitializedAsync method calls UpdateItemToggle when DefaultToggleKey is set during initialization, and UpdateItemToggle invokes OnToggleChange. This is intentional design, not a bug or unusual behavior. The component is designed to trigger OnToggleChange callbacks both during initialization (when DefaultToggleKey is applied) and on user interaction. The test correctly validates this expected behavior, and the demo pages document this usage pattern as part of the public API.
The test code at lines 76-77 is correct and requires no changes.
closes #11646
Summary by CodeRabbit