CONSOLE-5226: Update RTL Claude Skill#16446
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cajieh: This pull request references CONSOLE-5226 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cajieh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughUpdates to the RTL test skill documentation that refine test generation guidance for OCP Console. Changes include: adding an initial inspection step for component dependencies, clarifying the decision logic for Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 @.claude/skills/gen-rtl-test/SKILL.md:
- Around line 1104-1121: Strategy 3's guidance is incorrect — remove the "Wrap
test in act() when needed" section and its examples; instead update the examples
and text to emphasize awaiting userEvent calls (e.g., userEvent.setup() and
await user.click(...)) and using findBy*/waitFor for async dropdown interactions
(e.g., screen.findByText('Option 1') or waitFor(...)) and explicitly call out
that `@testing-library/user-event` v14+ handles act() internally and explicit
act() wrapping around userEvent is discouraged; ensure any references to act(),
the BAD/GOOD snippets, and the import line (import { ..., act } ...) are removed
or replaced with the corrected patterns.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e12549f6-291e-4390-98c0-6977aa74cf84
📒 Files selected for processing (1)
.claude/skills/gen-rtl-test/SKILL.md
📜 Review details
🔇 Additional comments (1)
.claude/skills/gen-rtl-test/SKILL.md (1)
28-29: LGTM!Also applies to: 50-84, 86-108, 591-606, 665-692, 769-778, 839-865, 1593-1594
| **Strategy 3: Wrap test in act() when needed** | ||
| ```typescript | ||
| // Import act from @testing-library/react | ||
| import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; | ||
| import { render, screen, waitFor, act } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| // ❌ BAD: Causes act() warning for dropdown/select interactions | ||
| const user = userEvent.setup(); | ||
| const dropdown = screen.getByText('Select Option'); | ||
| fireEvent.click(dropdown); | ||
| await user.click(dropdown); | ||
|
|
||
| // ✅ GOOD: Wrap in act() for complex interactions | ||
| await act(async () => { | ||
| fireEvent.click(dropdown); | ||
| await user.click(dropdown); | ||
| }); | ||
| const option = await screen.findByText('Option 1'); | ||
| fireEvent.click(option); | ||
| await user.click(option); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @testing-library/user-event v14+ require explicit act() wrapping for interactions?
💡 Result:
No. For @testing-library/user-event v14+, you generally should NOT explicitly wrap interactions in act. The docs and maintainers state that user-event utilities are already integrated with Testing Library’s act-handling; if you see act warnings, it typically means your environment/config isn’t using the same @testing-library/dom instance as the framework wrapper, or you have asynchronous work (often related to timers) that you need to wait for (not manually act-wrap the interaction). Evidence: - Maintainer guidance: “No, wrapping with act is neither required nor recommended.” and if await(user.anyAPI) yields act warnings but calling the interaction without waiting avoids them, that suggests you might not be using the same @testing-library/dom configuration instance as the framework wrapper [1]. - user-event docs emphasize using userEvent.setup and awaiting interactions (since they return Promises) rather than act-wrapping [2]. - Reports of “not wrapped in act” in v14 are usually resolved by awaiting user-event methods correctly; maintainers/users note v14 APIs are async and should be awaited (not act-wrapped) [3]. - There are edge cases where act appears to be needed due to fake timers or other asynchronous behavior, but these are exceptions tied to test setup/timers and can often be addressed by waiting (e.g., waitFor) or proper fake-timer handling, not by blanket act wrapping [4]. Practical takeaway: 1) Use userEvent.setup and always await userEvent methods (they return Promises in v14+) [2][5]. 2) If you still get act warnings, check that @testing-library/dom isn’t duplicated/resolved differently than your @testing-library/react wrapper expects [1] and handle fake timers via user-event’s advanceTimers option rather than act-wrapping [6][7]. If you tell me whether you’re using React Testing Library, Jest vs Vitest, and whether fake timers are enabled, I can suggest the exact setup pattern to eliminate act warnings.
Citations:
- 1: Is wrapping with act now required? testing-library/user-event#906
- 2: https://testing-library.com/docs/user-event/intro/
- 3: userEvent.type not working in v14 testing-library/user-event#1150
- 4: "act" is necessary with Fake Timers even though (IIUC) shouldn't be testing-library/user-event#1218
- 5: Do I need await before every userEvent call? testing-library/user-event#1050
- 6: https://stackoverflow.com/questions/74214740/how-to-use-user-event-library-in-conjunction-with-jests-fake-timers
- 7: Update to v14 breaks @testing-library/user-event on Vitest testing-library/react-testing-library#1197
🏁 Script executed:
#!/bin/bash
# Check lines 621 and 1104-1121 in the SKILL.md file
echo "=== Line 621 context (620-630) ==="
sed -n '620,630p' .claude/skills/gen-rtl-test/SKILL.md
echo ""
echo "=== Lines 1104-1121 (Strategy 3) ==="
sed -n '1104,1121p' .claude/skills/gen-rtl-test/SKILL.md
echo ""
echo "=== Broader context: lines 1070-1130 ==="
sed -n '1070,1130p' .claude/skills/gen-rtl-test/SKILL.md | cat -nRepository: openshift/console
Length of output: 3302
Strategy 3 presents outdated act() guidance that contradicts both line 621 and modern @testing-library/user-event best practices.
Line 621 correctly states act() is "rarely needed" because userEvent, findBy*, and waitFor already handle it internally. However, Strategy 3 presents wrapping userEvent in explicit act() as a solution for dropdown/select interactions.
This is incorrect. The web search confirms that @testing-library/user-event v14+ does not require and actively discourages explicit act() wrapping around user-event calls. When act() warnings appear with userEvent, the root cause is typically:
- Missing
awaiton userEvent methods (fixed by Strategies 1–2) - Unhandled async work or fake timers (addressed via
waitForor timer config, not act wrapping) - Environment/config issues
The BAD example in Strategy 3 (await user.click(dropdown)) doesn't fail because it's missing act(); it fails because it likely doesn't await properly or the async effect isn't resolved. The "GOOD" solution should be Strategy 1 (ensure you await) or Strategy 2 (use findBy*), not wrap in act().
Recommendation: Remove Strategy 3 entirely. If dropdowns are causing issues, the fix is one of the first two strategies, not explicit act wrapping.
🤖 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 @.claude/skills/gen-rtl-test/SKILL.md around lines 1104 - 1121, Strategy 3's
guidance is incorrect — remove the "Wrap test in act() when needed" section and
its examples; instead update the examples and text to emphasize awaiting
userEvent calls (e.g., userEvent.setup() and await user.click(...)) and using
findBy*/waitFor for async dropdown interactions (e.g., screen.findByText('Option
1') or waitFor(...)) and explicitly call out that `@testing-library/user-event`
v14+ handles act() internally and explicit act() wrapping around userEvent is
discouraged; ensure any references to act(), the BAD/GOOD snippets, and the
import line (import { ..., act } ...) are removed or replaced with the corrected
patterns.
cajieh
force-pushed
the
update-rtl-claude-skill
branch
from
6d8ebc1 to
a039d18
Compare
|
@cajieh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
CONSOLE-5226: Update RTL Claude Skill
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit