-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(test): 优化测试套件 #14
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
Conversation
WalkthroughThis pull request systematically refactors test suites across nine UI components to improve maintainability. Each test file introduces helper functions (e.g., createWrapper, createShallowWrapper) to centralize component mounting logic, replacing repeated direct mount/shallowMount calls with standardized wrapper factories that consolidate prop/slot provisioning and reduce boilerplate throughout the tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ccui/ui/slider/test/slider.test.ts (3)
437-450:input change with invalid valuetest no longer exercises an invalid-path scenarioThe test name suggests verifying Slider’s behavior when
CInputNumberemits an invalid value, but the body only asserts thatCInputNumberexists — something already covered by other tests. As written, this is effectively a no-op and can mislead future readers about what’s covered.Consider either:
- Emitting a clearly invalid value (per your component’s spec) and asserting the expected clamping/ignore behavior, or
- Removing/renaming the test if there’s no distinct invalid-input behavior to assert.
490-536: Input control button tests are too weak for their namesThe three tests:
event - input increase buttonevent - input decrease buttonevent - input buttons disabled at boundariescurrently only assert that
CInputNumberexists. They don’t simulate clicks on the +/- controls, don’t assert emitted values, and don’t verify disabled states at bounds, so they don’t actually cover the behaviors implied by their names and are largely redundant with other existence checks.It would be better to either:
- Drive the actual interactions (e.g., trigger increase/decrease events or button clicks and assert the resulting
update:modelValue/clamping/disabled behavior), or- Remove these tests or narrow their descriptions if the intended scenarios are covered elsewhere.
As-is, they give a false sense of coverage around the input control buttons.
615-621: Test name does not matchpersistentprop valueIn
it('props - persistent false with showTooltip false', ...), the props setpersistent: truewhile the description says “persistent false”. That mismatch is confusing and can mislead future maintainers about the scenario under test.Either flip the prop to
persistent: falseor adjust the test name to reflect that this case is “persistent true + showTooltip false”.
🧹 Nitpick comments (6)
packages/ccui/ui/slider/test/slider.test.ts (2)
15-23:createMockRecthelper nicely centralizes rect mockingExtracting the DOMRect-like object into
createMockRectreduces duplication and keeps click/drag tests consistent. If you ever start relying onx/yfields, you can extend this helper without touching individual tests.
36-46: Synchronousprops - modelValuetest is correct and clearerThis test only inspects
wrapper.vm.currentValue, so dropping the unnecessaryasyncwrapper is appropriate and keeps the test simpler.You might also consider introducing a
createWrapperhelper for Slider (similar to other suites in this PR) to DRY up repeatedmount(Slider, { props })andwrapper.unmount()calls, but it’s not required for correctness.packages/ccui/ui/input/test/input.test.ts (4)
14-21: Helper wrappers keep tests DRY and focusedCentralizing
mount/shallowMountintocreateWrapper/createShallowWrapperreads well and reduces boilerplate. If you ever want a bit more type safety, you could lightly type thepropsparameter, e.g.:-function createWrapper(props = {}) { +function createWrapper(props: Record<string, any> = {}) { return mount(Input, { props }) } -function createShallowWrapper(props = {}) { +function createShallowWrapper(props: Record<string, any> = {}) { return shallowMount(Input, { props }) }Not required, but it can help catch prop name typos in tests.
25-79: Nice consolidation of simple prop/DOM tests ontocreateShallowWrapperSwitching the “dom / type / size / placeholder / disabled / readonly / clearable / prepend / append” tests to
createShallowWrapperkeeps them very concise while still asserting the same behaviour (including thedisabledclass logic vians.m('disabled').substring(1)).One minor nit: only the
'dom'test explicitly callswrapper.unmount(), while the others rely on test-utils cleanup. For consistency, consider either:
- dropping explicit
unmount()everywhere and relying on the global cleanup, or- adding
wrapper.unmount()to the remaining tests as well.Purely stylistic — behaviour is fine as-is.
100-110: Combined focus/blur test is compact and still readableMerging focus and blur into
it('emits focus and blur events', ...)with a shared wrapper and separate spies looks good and keeps the flow linear: focus → assertion → blur → assertion. The tradeoff in diagnostics (a single test covers both events) seems acceptable given the reduced duplication.
126-144: Password visibility toggle test is solid; optional stronger assertionThe password visibility test drives the icon toggling via clicks and asserts the correct BEM element switches between
password-hiddenandpassword-visible, which is good coverage for the visual affordance.If you want slightly stronger guarantees, you could also assert the
<input>typeattribute flips between'password'and'text'alongside the icon expectations, but that’s an optional enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/ccui/ui/button/test/button.test.ts(2 hunks)packages/ccui/ui/calendar/test/calendar.test.ts(1 hunks)packages/ccui/ui/input-number/test/input-number.test.ts(10 hunks)packages/ccui/ui/input/test/input.test.ts(1 hunks)packages/ccui/ui/popover/test/popover.test.ts(16 hunks)packages/ccui/ui/rate/test/rate.test.ts(3 hunks)packages/ccui/ui/slider/test/slider.test.ts(10 hunks)packages/ccui/ui/timeline/test/timeline.test.ts(3 hunks)packages/ccui/ui/tooltip/test/tooltip.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/ccui/ui/popover/test/popover.test.ts (1)
packages/ccui/ui/popover/index.ts (1)
Popover(8-8)
packages/ccui/ui/rate/test/rate.test.ts (1)
packages/ccui/ui/rate/index.ts (1)
Rate(8-8)
packages/ccui/ui/timeline/test/timeline.test.ts (1)
packages/ccui/ui/timeline/index.ts (2)
Timeline(10-10)TimelineItem(10-10)
packages/ccui/ui/input-number/test/input-number.test.ts (1)
packages/ccui/ui/input-number/index.ts (1)
InputNumber(8-8)
🔇 Additional comments (20)
packages/ccui/ui/input-number/test/input-number.test.ts (3)
6-11: LGTM! Clean abstraction for test setup.The helper function effectively reduces boilerplate and centralizes mounting logic. This makes tests more readable and easier to maintain, aligning well with the PR's standardization goals.
165-171: Good addition for decimal step coverage.This test nicely complements the existing step test by verifying decimal increments with precision, ensuring fractional steps work correctly.
14-183: Excellent systematic refactoring!All tests have been consistently migrated to use the helper function while preserving their original logic and coverage. The refactoring successfully reduces boilerplate (21% line reduction) without compromising test quality or coverage.
packages/ccui/ui/rate/test/rate.test.ts (1)
10-17: LGTM! Clean helper function implementation.The test helpers follow a consistent pattern with the rest of the PR, centralizing component mounting logic and providing a clear API for props and slots configuration.
packages/ccui/ui/tooltip/test/tooltip.test.ts (2)
6-31: LGTM! Well-designed helper functions.The helpers provide sensible defaults (
teleported: falseand a default trigger button) that work for most test scenarios while allowing overrides. This significantly reduces boilerplate across the test suite.
96-107: Excellent use of parameterized testing.Using
it.eachto test multiple theme variations reduces duplication and makes it easy to add new themes in the future.packages/ccui/ui/button/test/button.test.ts (2)
21-40: LGTM! Consistent helper implementation.The helpers follow the same pattern as other component tests in this PR, providing a default slot content while allowing customization.
85-115: Excellent comprehensive click event testing.This test thoroughly covers all click scenarios: normal clicks, disabled state preventing clicks, and loading state preventing clicks. The use of
addEventListeneris appropriate for testing native DOM event behavior and aligns with the Vue 3 compatibility improvements mentioned in the PR objectives.packages/ccui/ui/timeline/test/timeline.test.ts (2)
6-22: LGTM! Smart dual-helper approach.Providing separate helpers for
TimelineandTimelineItemis a good design choice. ThecreateItemWrapperincludes sensible defaults (timestamp and content) that work for most tests while remaining flexible for customization.
60-69: Great use of parameterized testing.Converting multiple type tests to a data-driven approach with
it.eachmakes the test suite more maintainable and easier to extend with new types.packages/ccui/ui/popover/test/popover.test.ts (2)
6-31: LGTM! Consistent helper implementation.The helpers mirror the approach used in tooltip.test.ts with sensible defaults that work for most scenarios while allowing flexibility.
111-122: Excellent parameterization.Using
it.eachfor theme and arrow visibility tests reduces duplication and improves maintainability.Also applies to: 139-150
packages/ccui/ui/calendar/test/calendar.test.ts (2)
13-27: LGTM! Helper handles global component registration.The helper appropriately includes global component registration for
CButton, which is necessary for Calendar's internal button usage. The defaultmodelValueensures the component is always in a valid state.
65-87: Good consolidation of navigation tests.Merging the month navigation tests into a single flow improves readability while maintaining comprehensive coverage. The approach of re-fetching buttons before each interaction (lines 70, 75, 80) is correct, as the DOM updates between clicks.
packages/ccui/ui/slider/test/slider.test.ts (4)
2-2: Vitest import cleanup is appropriateRemoving
beforeEachfrom the vitest import aligns with the per-test mount/unmount pattern now used; no behavioral impact.
240-250: UpdatedshowStopsexpectation looks consistent with defaultsWith an implied 0–100 range and
step: 10, expecting 11 stops (including both endpoints) is internally consistent with the component behavior.
264-279: Click event test correctly reusescreateMockRectUsing
vi.spyOn(...getBoundingClientRect).mockReturnValue(createMockRect())keeps the click-position math under test while avoiding hard-coded rectangles in multiple places. Assertions on bothupdate:modelValueandinputemissions remain intact.
317-329: Drag event test keeps behavior, now with shared rect helperReusing
createMockRectforsliderRef.getBoundingClientRectpreserves the drag behavior under test while centralizing the geometry setup. Checkingwrapper.vm.isDraggingaftermousedownstill verifies the drag entry path.packages/ccui/ui/input/test/input.test.ts (2)
81-98: Event tests forinputandchangeare clearer withcreateWrapperUsing
createWrapperplusonInput/onChangehandlers and driving viasetValue/trigger('change')mirrors real consumer usage nicely. The expectations are straightforward (toBeCalled()), and the explicitwrapper.unmount()keeps these full-mount tests tidy.
112-124: Clearable behaviour test covers both callback and DOM stateThe
clearablescenario is well covered: you assert thatonClearis invoked and that the underlying<input>value is reset to''after clicking the clear icon. UsingcreateWrapperwith the minimal prop set (clearable,modelValue,onClear) keeps the intent very clear.
| expect(hide).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('aRIA 属性', async () => { |
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.
Fix typo in test name.
The test name contains "aRIA" which should be "ARIA" (all caps).
Apply this diff:
- it('aRIA 属性', async () => {
+ it('ARIA 属性', async () => {📝 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.
| it('aRIA 属性', async () => { | |
| it('ARIA 属性', async () => { |
🤖 Prompt for AI Agents
In packages/ccui/ui/popover/test/popover.test.ts around line 259, the test name
contains a typo "aRIA 属性" — change the test description string to use the
correct casing "ARIA 属性" (replace "aRIA" with "ARIA") so the test name reads
"ARIA 属性". Ensure only the visible test title is edited and no other code or
formatting is changed.
| }, | ||
| }) | ||
|
|
||
| it('aRIA 属性', async () => { |
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.
Fix typo in test name.
The test name contains "aRIA" which should be "ARIA" (all caps) as it's an acronym for Accessible Rich Internet Applications.
Apply this diff:
- it('aRIA 属性', async () => {
+ it('ARIA 属性', async () => {📝 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.
| it('aRIA 属性', async () => { | |
| it('ARIA 属性', async () => { |
🤖 Prompt for AI Agents
In packages/ccui/ui/tooltip/test/tooltip.test.ts around line 249, the test title
contains a typo "aRIA 属性"; change the test name to use the correct uppercase
acronym "ARIA" (e.g., replace "aRIA 属性" with "ARIA 属性") so the test description
correctly reflects the ARIA acronym.
🧪 优化 CCUI 测试套件
📝 概述
优化了 CCUI 组件库的测试套件,通过引入辅助函数、合并重复测试、使用
it.each等方式,显著提升了代码质量和可维护性。✨ 主要改进
代码优化
优化的文件
tooltip.test.ts- 488→272 行 (-44%)calendar.test.ts- 274→145 行 (-47%)button.test.ts- 242→165 行 (-32%)timeline.test.ts- 198→128 行 (-35%)input-number.test.ts- 292→230 行 (-21%)input.test.ts- 206→154 行 (-25%)rate.test.ts- 159→110 行 (-31%)popover.test.ts- 已优化🔧 技术改进
1. 引入辅助函数
为每个测试文件添加了
createWrapper和createShallowWrapper辅助函数,统一默认配置,减少重复代码。2. 使用 it.each 合并相似测试
将多个相似测试合并为参数化测试,提高可读性和可维护性。
3. 合并关联测试
将相关的测试用例合并为完整的工作流测试,例如:
4. 移除重复测试
识别并移除了真正重复的测试用例:
input-number.test.ts中的重复精度测试input-number.test.ts中的重复边界测试5. 代码质量提升
$on→addEventListener)nextTick使用✅ 测试结果
📊 影响范围
🔍 Review 重点
it.each的使用是否恰当类型: 测试优化
影响: 测试代码
风险: 低
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.