test: Fix test case #650
test: Fix test case #650
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough将测试从 Enzyme 迁移到 React Testing Library，替换交互为 DOM 查询与 fireEvent/act；新增测试工具与 jest 假定时器；在测试环境中注入 MessageChannel 全局模拟；调整示例中图标为直接节点引用。 Changes
Sequence Diagram(s)sequenceDiagram
participant Test as 测试用例
participant RTL as React Testing Library
participant Component as TreeSelect
participant Timer as Jest Timer
rect rgb(240,248,255)
Note over Test,RTL: 渲染与打开流程
Test->>RTL: render(Component, props)
Test->>RTL: triggerOpen(container)
activate Timer
Timer-->>Component: advanceTimers (act)
Component-->>RTL: 下拉渲染完成
end
rect rgb(245,255,240)
Note over RTL,Component: 搜索与选择
RTL->>Component: search(container, "query")
Timer-->>Component: advanceTimers (过滤/异步)
Component-->>RTL: 更新列表
RTL->>Component: selectNode(index)
Component-->>Test: 触发 onSelect/onChange
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
⏰ 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). (2)
🔇 Additional comments (2)
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 |
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the testing infrastructure by refactoring existing test cases to leverage Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors a significant portion of the test suite to use React Testing Library instead of Enzyme, which is a great move for modernizing the tests. The introduction of shared utility functions in tests/util.tsx improves maintainability.
My review focuses on ensuring consistency and correctness during this large-scale refactoring. I've identified a couple of key areas for improvement:
- Timer Mocking: There are inconsistencies in how Jest's fake timers are managed across different test files. Some files are missing timer mocks entirely for tests that need them, which could lead to flaky tests. I've recommended a consistent approach using
beforeEachandafterEach. - Test Utilities: The new timer-based utilities could be made more efficient and robust by using
jest.runAllTimers()instead of advancing by a large, arbitrary amount of time. - Code Style: A minor comment about a non-English comment in the code.
Overall, this is a solid refactoring. Addressing these points will make the test suite more robust and consistent.
| }); | ||
|
|
||
| it('clear selected value and input value', () => { | ||
| jest.useFakeTimers(); |
There was a problem hiding this comment.
While adding fake timers here is correct for this test, several other tests in this file are also being refactored to use utilities like search and triggerOpen which rely on fake timers. Some of them are missing the timer setup (e.g., the 'remove in filtered tree' test). To ensure consistency and prevent future errors, it would be better to manage timers at the describe block level using beforeEach and afterEach, as done in tests/Select.multiple.spec.js.
This would involve:
- Adding
beforeEach(() => { jest.useFakeTimers(); });andafterEach(() => { jest.useRealTimers(); });at the top of thedescribe('TreeSelect.checkable', ...)block. - Removing individual
jest.useFakeTimers()andjest.useRealTimers()calls from this test and others.
| search(container, 'Title 1'); | ||
| expect( | ||
| container.querySelectorAll('.rc-tree-select-tree-treenode:not([aria-hidden="true"])'), | ||
| ).toHaveLength(1); |
There was a problem hiding this comment.
The search utility function relies on fake timers to work correctly, but timer mocking (jest.useFakeTimers()) is not enabled for this test file. This could lead to unreliable test results. Please add beforeEach and afterEach hooks to the main describe block to manage fake timers for all tests in this file, like this:
beforeEach(() => {
jest.useFakeTimers();
});
afterEach(() => {
jest.useRealTimers();
});| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllTimers(); | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
It's great that you're using beforeEach and afterEach to manage fake timers. However, jest.clearAllTimers() in afterEach is not strictly necessary when you are calling jest.useRealTimers(), as useRealTimers restores the original timer functions and clears any existing fake timers. You can simplify the afterEach block.
| beforeEach(() => { | |
| jest.useFakeTimers(); | |
| }); | |
| afterEach(() => { | |
| jest.clearAllTimers(); | |
| jest.useRealTimers(); | |
| }); | |
| afterEach(() => { | |
| jest.useRealTimers(); | |
| }); |
| const itemTitle = container.querySelector('.rc-tree-select-tree-title'); | ||
| const item = container.querySelector(`.${customClassNames.popup.item}`); | ||
|
|
||
| // 如果suffix还是找不到，就跳过这个检查 |
There was a problem hiding this comment.
This comment is in Chinese, while the rest of the codebase and PR are in English. For consistency, please translate it to English.
| // 如果suffix还是找不到，就跳过这个检查 | |
| // If suffix is still not found, skip this check |
| export function triggerOpen(element: HTMLElement) { | ||
| fireEvent.mouseDown(element.querySelector('.rc-tree-select')!); | ||
| act(() => { | ||
| jest.advanceTimersByTime(10000); |
There was a problem hiding this comment.
Advancing timers by a large, arbitrary number like 10000ms can make tests slower and might hide issues related to specific timer durations. It's generally better to use jest.runAllTimers() to flush all timers. This change should be applied to all jest.advanceTimersByTime(10000) calls in this file.
| jest.advanceTimersByTime(10000); | |
| jest.runAllTimers(); |
![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
🧹 Nitpick comments (1)
tests/util.tsx (1)
20-21: 建议将硬编码的计时器时间提取为常量。在多个工具函数中使用硬编码的
10000毫秒可能不够灵活。考虑将此值提取为命名常量（例如TIMER_ADVANCE_MS），以便于维护和调整。应用此差异以提取常量：
+const TIMER_ADVANCE_MS = 10000; + export function triggerOpen(element: HTMLElement) { fireEvent.mouseDown(element.querySelector('.rc-tree-select')!); act(() => { - jest.advanceTimersByTime(10000); + jest.advanceTimersByTime(TIMER_ADVANCE_MS); }); } export function expectOpen(element: HTMLElement, open = true) { act(() => { - jest.advanceTimersByTime(10000); + jest.advanceTimersByTime(TIMER_ADVANCE_MS); }); return expect(!!element.querySelector('.rc-tree-select-open')).toBe(open); } export function search(element: HTMLElement, value: string) { const input = element.querySelector('input')!; fireEvent.change(input, { target: { value } }); act(() => { - jest.advanceTimersByTime(10000); + jest.advanceTimersByTime(TIMER_ADVANCE_MS); }); }Also applies to: 27-27, 36-36
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/__snapshots__/Select.checkable.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.multiple.spec.js.snapis excluded by!**/*.snaptests/__snapshots__/Select.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
tests/Select.checkable.spec.tsx(16 hunks)tests/Select.multiple.spec.js(4 hunks)tests/Select.props.spec.js(10 hunks)tests/Select.spec.tsx(12 hunks)tests/Select.tree.spec.js(6 hunks)tests/setup.js(2 hunks)tests/util.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Select.checkable.spec.tsx (1)
tests/util.tsx (4)
triggerOpen(18-23)selectNode(3-6)search(32-38)clearSelection(40-43)
tests/Select.multiple.spec.js (1)
tests/util.tsx (5)
triggerOpen(18-23)selectNode(3-6)clearSelection(40-43)expectOpen(25-30)search(32-38)
tests/Select.props.spec.js (1)
tests/util.tsx (1)
search(32-38)
tests/Select.spec.tsx (1)
tests/util.tsx (3)
selectNode(3-6)triggerOpen(18-23)expectOpen(25-30)
⏰ 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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Socket Security: Project Report
🔇 Additional comments (10)
tests/setup.js (1)
52-76: LGTM! MessageChannel mock 实现正确。MessageChannel mock 的实现正确模拟了双向通信，使用
setTimeout异步传递消息。这对于不支持 MessageChannel API 的测试环境是必要的。tests/Select.tree.spec.js (1)
114-116: LGTM! 正确使用可选链和 textContent。使用可选链操作符
?.访问textContent可以安全处理元素不存在的情况，与 RTL 的最佳实践保持一致。tests/Select.spec.tsx (1)
407-422: LGTM! 正确使用新的测试工具。测试正确地使用了
triggerOpen和expectOpen工具函数来管理异步打开状态，并验证了滚动行为。tests/Select.multiple.spec.js (2)
11-18: LGTM! 假计时器的设置和清理正确。在
beforeEach中启用假计时器，在afterEach中清理和恢复真实计时器。这确保了测试之间的隔离性和确定性计时行为。
141-165: 测试迁移到 RTL 后逻辑清晰。测试正确地使用了新的工具函数（
triggerOpen、selectNode、clearSelection、expectOpen），并通过容器查询进行断言。代码注释清楚地解释了每个步骤。tests/Select.props.spec.js (2)
48-57: LGTM! 正确使用 search 工具和可见节点过滤。测试正确地使用了
search工具函数，并通过排除aria-hidden="true"的节点来统计可见节点，这与 RTL 的可访问性最佳实践一致。
138-148: 正确处理异步 treeData 更新。测试正确地使用
rerender更新 props，并通过过滤可见节点来验证搜索过滤的行为。这确保了测试只断言用户可见的内容。tests/Select.checkable.spec.tsx (3)
214-249: LGTM! 假计时器的局部使用正确。测试在开始时启用假计时器，在结束时恢复真实计时器。使用 RTL 工具函数和容器查询正确地验证了清除选择和输入值的行为。
319-363: 测试正确验证了过滤场景中的复选框行为。测试使用假计时器、
search和selectNode工具函数正确地模拟了在过滤结果中选择节点的场景，并验证了选择项的数量。
692-695: 使用过滤可见节点的辅助函数很好。
getTreeNode函数通过排除aria-hidden节点来获取可见的树节点，这是一个好的实践，确保测试只针对用户可见的元素进行断言。
| const prefix = container.querySelector('.rc-tree-select-prefix'); | ||
| const input = container.querySelector('.rc-tree-select-selection-search-input'); | ||
| const suffix = container.querySelector('.rc-tree-select-arrow'); | ||
| const input = container.querySelector('.rc-tree-select-input'); | ||
| const suffix = | ||
| container.querySelector('.rc-tree-select-content-item-suffix') || | ||
| container.querySelector('[class*="suffix"]'); | ||
| const itemTitle = container.querySelector('.rc-tree-select-tree-title'); | ||
| const item = container.querySelector(`.${customClassNames.popup.item}`); | ||
|
|
||
| // 如果suffix还是找不到，就跳过这个检查 | ||
| expect(prefix).toHaveClass(customClassNames.prefix); | ||
| expect(input).toHaveClass(customClassNames.input); | ||
| expect(suffix).toHaveClass(customClassNames.suffix); | ||
| if (suffix) { | ||
| expect(suffix).toHaveClass(customClassNames.suffix); | ||
| } | ||
| expect(itemTitle).toHaveClass(customClassNames.popup.itemTitle); | ||
|
|
||
| expect(prefix).toHaveStyle(customStyles.prefix); | ||
| expect(input).toHaveStyle(customStyles.input); | ||
| expect(suffix).toHaveStyle(customStyles.suffix); | ||
| if (suffix) { | ||
| expect(suffix).toHaveStyle(customStyles.suffix); | ||
| } | ||
| expect(itemTitle).toHaveStyle(customStyles.popup.itemTitle); | ||
| expect(item).toHaveStyle(customStyles.popup.item); | ||
| }); |
There was a problem hiding this comment.
条件断言可能隐藏潜在问题。
Lines 722-724 和 729-731 在 suffix 元素不存在时跳过断言。如果 suffix 元素预期应该存在（因为提供了 suffixIcon prop），那么跳过断言可能会隐藏真实的 bug。
考虑以下方案之一：
- 如果 suffix 必须存在，则断言其存在性：
- if (suffix) {
- expect(suffix).toHaveClass(customClassNames.suffix);
- }
+ expect(suffix).toBeTruthy();
+ expect(suffix).toHaveClass(customClassNames.suffix);- 或者使用更明确的选择器来确保找到正确的元素：
- const suffix =
- container.querySelector('.rc-tree-select-content-item-suffix') ||
- container.querySelector('[class*="suffix"]');
+ const suffix = container.querySelector('.rc-tree-select-suffix');
+ expect(suffix).toBeTruthy();
+ expect(suffix).toHaveClass(customClassNames.suffix);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/Select.spec.tsx around lines 711 to 734, the test currently skips
assertions when the suffix element is missing which can hide real bugs; change
the test to explicitly assert the suffix exists when suffixIcon prop is provided
(i.e., replace the conditional checks with an existence expectation) and then
run the style and class assertions on that found element, or if the suffix may
legitimately be absent, replace the permissive query with a stricter selector
that uniquely targets the expected suffix element (for example a specific
data-testid or exact class) and assert either its presence or absence
accordingly before checking classes/styles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/custom-icons.tsx (1)
73-78: 考虑移除重复代码并统一图标配置。变更后，
iconPropsFunction与iconProps（第 66-71 行）完全相同，造成了代码重复。此外，iconPropsFunction这个命名暗示其包含函数，但实际上现在包含的是直接引用，容易造成误解。建议移除
iconPropsFunction，在两个 TreeSelect 示例中都使用iconProps。可以应用以下修改来消除重复：
-const iconPropsFunction = { - suffixIcon, - clearIcon, - removeIcon, - switcherIcon, -}; - function Demo() { return ( <div className="custom-icon-demo">同时更新第 105 行：
- {...iconPropsFunction} + {...iconProps}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/custom-icons.tsx(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). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Socket Security: Project Report
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #650 +/- ##
=======================================
Coverage 99.83% 99.83%
=======================================
Files 17 17
Lines 603 619 +16
Branches 186 177 -9
=======================================
+ Hits 602 618 +16
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Tests
Chores
Examples