-
Notifications
You must be signed in to change notification settings - Fork 273
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
test(site): fix icon not correct test e2e #2445
Conversation
WalkthroughThe pull request introduces modifications to various Playwright test files, primarily focusing on the validation of SVG icons within action menus, dropdowns, and other UI components. Key changes include updates to assertions related to SVG path attributes and visibility checks, enhancing the accuracy of the tests. The overall structure of the tests remains largely unchanged, with adjustments made to specific checks and expected values. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
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: 1
🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/action-menu/card-mode.spec.ts (1)
15-16
: Add comments to explain the SVG path pattern.The test logic is correct, but it would be helpful to add comments explaining what this SVG path represents and why we're validating it this way.
+ // Validate the "more" icon's SVG path. This pattern matches the three-dot menu icon. const SvgPathReg = /^M2\.3 7\.78v.+219-1\.17Z$/
examples/sites/demos/pc/app/button-group/show-more.spec.ts (2)
Line range hint
1-27
: Consider enhancing test robustness and readability.While the test implementation is functional, here are some suggestions to improve it:
- Add test descriptions for each assertion block using
test.describe()
and nestedtest()
blocks- Consider extracting selectors into constants at the top of the file
- Add error messages to assertions for better debugging
Here's a suggested refactor:
import { test, expect } from '@playwright/test' const SELECTORS = { demo: '#show-more', buttonGroup: '.tiny-button-group', moreButtonSvg: 'svg path' } test.describe('Button Group Show More', () => { test.beforeEach(async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('button-group#show-more') }) test('should handle button clicks correctly', async ({ page }) => { const demo = page.locator(SELECTORS.demo) const buttonGroup = demo.locator(SELECTORS.buttonGroup) // Your existing click tests... }) test('should display correct more button icon', async ({ page }) => { const demo = page.locator(SELECTORS.demo) const buttonGroup = demo.locator(SELECTORS.buttonGroup) const moreButton = buttonGroup.getByRole('button').nth(3) const moreButtonSvg = moreButton.locator(SELECTORS.moreButtonSvg).first() const moreSvgPathReg = /^M2\.3 7\.78v.+219-1\.17Z$/ await expect(moreButtonSvg, 'More button should have correct SVG path').toHaveAttribute('d', moreSvgPathReg) }) })
Line range hint
1-27
: Consider adding visual regression tests for icon validation.Instead of relying solely on SVG path validation which can be brittle, consider adding visual regression tests for icon appearance.
Visual regression testing tools like
playwright-visual-comparisons
can provide more reliable icon validation while being less dependent on specific SVG implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- examples/sites/demos/pc/app/action-menu/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/action-menu/card-mode.spec.ts (1 hunks)
- examples/sites/demos/pc/app/action-menu/icon.spec.ts (1 hunks)
- examples/sites/demos/pc/app/button-group/show-more.spec.ts (1 hunks)
- examples/sites/demos/pc/app/date-picker/clear.spec.ts (2 hunks)
- examples/sites/demos/pc/app/dropdown/basic-usage.spec.ts (0 hunks)
💤 Files with no reviewable changes (1)
- examples/sites/demos/pc/app/dropdown/basic-usage.spec.ts
🔇 Additional comments (6)
examples/sites/demos/pc/app/date-picker/clear.spec.ts (3)
Line range hint
1-31
: LGTM! Test coverage is comprehensive.The test effectively covers all key scenarios:
- Default clear icon visibility and functionality
- Hidden clear icon behavior
- Custom clear icon visibility and clearing operation
The test structure follows good practices with clear scenario separation and meaningful assertions.
24-24
: Ensure consistent path validation across custom and default icons.The same regex pattern is used for both default and custom clear icons. This suggests they share the same SVG structure, which might not be the intended behavior for a custom icon.
#!/bin/bash # Description: Verify if custom clear icons should have different SVG paths # Test: Search for custom clear icon implementations rg -B 5 'customClearIcon|custom-clear' examples/sites/demos/pc/app/date-picker/
15-15
: Verify the SVG path selector change.The selector for the clear icon's SVG path has changed from
nth(1)
tonth(0)
. While the regex pattern provides more flexible validation, we should verify this selector change is intentional.✅ Verification successful
SVG path selector change is consistent with component implementation
The change from
nth(1)
tonth(0)
is correct as the test validates the clear icon SVG path across different scenarios (default and custom clear icons). The test is consistent with the component implementation which uses the same clear icon structure in both cases, as evidenced by:
- Both test assertions use the same selector and regex pattern
- The component uses the same clear icon structure for both default and custom cases
- The test successfully validates the clear functionality after the selector change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the SVG structure in other date picker clear icon tests # to ensure consistency in path selection # Test: Search for similar clear icon path validations rg -A 5 'clearIcon.*locator.*path.*nth' examples/sites/demos/pc/app/date-picker/Length of output: 175
Script:
#!/bin/bash # Let's try a different approach to find date picker clear icon tests # First, let's look for all files related to date picker clear functionality fd "clear" examples/sites/demos/pc/app/date-picker/ # Then let's examine the content of these files rg -l "clearIcon" examples/sites/demos/pc/app/date-picker/ # Finally, let's look for SVG path related tests in these files rg "toHaveAttribute.*d.*" examples/sites/demos/pc/app/date-picker/Length of output: 852
Script:
#!/bin/bash # Let's examine the content of the clear.spec.ts file to understand the full context cat examples/sites/demos/pc/app/date-picker/clear.spec.ts # Also let's check the Vue components to see the SVG structure cat examples/sites/demos/pc/app/date-picker/clear.vue cat examples/sites/demos/pc/app/date-picker/clear-composition-api.vueLength of output: 3086
examples/sites/demos/pc/app/button-group/show-more.spec.ts (1)
25-26
: Verify the new SVG path pattern across different environments.The new regex pattern
^M2\.3 7\.78v.+219-1\.17Z$
appears to be more specific than the previous one. However, SVG paths can sometimes vary across different browsers or rendering engines.Let's verify this pattern against other icon tests in the codebase:
examples/sites/demos/pc/app/action-menu/basic-usage.spec.ts (1)
19-19
: LGTM! Improved icon visibility testing approach.The updated assertion provides a more reliable way to verify icon presence by checking the visibility of the icon container rather than specific SVG attributes. This makes the test more resilient to implementation details of the icon while still ensuring the icon is properly rendered.
examples/sites/demos/pc/app/action-menu/icon.spec.ts (1)
27-27
: LGTM! Verify action menu index.The change to select the second action menu (index 1) aligns with the test's purpose of checking text-only display. The subsequent assertions correctly verify that icons are hidden and text is visible.
Let's verify the component structure to ensure we're selecting the correct action menu:
✅ Verification successful
Based on the search results, I can now verify the action menu structure in the codebase:
From
icon.vue
, we can see there are two action menus in the demo:<tiny-action-menu class="custom-icon" ...> // First menu - icon only <tiny-action-menu :options="options1" :show-icon="false" ...> // Second menu - text only
The test file
icon.spec.ts
correctly uses:const actionMenu = wrap.locator('.tiny-action-menu').nth(0) // First test - icon only const actionMenu = wrap.locator('.tiny-action-menu').nth(1) // Second test - text onlyThe change from nth(2) to nth(1) is correct.
The second action menu (index 1) is indeed the text-only menu as shown in the source code. The test assertions align with this by checking that icons are hidden and text is visible. The change fixes a bug where the test was previously targeting the wrong menu (index 2) which didn't exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of action menu components in the demo # Expected: Find multiple action-menu elements with different display modes # Search for action menu related test files and their structure rg -A 5 "tiny-action-menu" examples/sites/demos/pc/app/action-menu/Length of output: 26898
const SvgPathReg = /^M2\.3 7\.78v.+219-1\.17Z$/ | ||
await expect(moreItem.locator('.tiny-svg path').first()).toHaveAttribute('d', SvgPathReg) |
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.
🛠️ Refactor suggestion
Consider making the SVG validation more resilient.
The current regex pattern with .+
is quite permissive and the hard-coded values might make the test brittle to minor UI changes. Consider focusing on essential characteristics of the SVG that indicate it's a "more" icon.
-const SvgPathReg = /^M2\.3 7\.78v.+219-1\.17Z$/
-await expect(moreItem.locator('.tiny-svg path').first()).toHaveAttribute('d', SvgPathReg)
+// Validate that the SVG represents a "more" icon by checking for essential path characteristics
+const isMoreIconPath = (path: string) => {
+ // Check if path starts with M and ends with Z (closed path)
+ return /^M.*Z$/.test(path);
+}
+const pathAttr = await moreItem.locator('.tiny-svg path').first().getAttribute('d');
+expect(isMoreIconPath(pathAttr || '')).toBeTruthy();
Committable suggestion was skipped due to low confidence.
* fix(build): fix build error * fix(docs): 增加上传文件类型说明 (#2439) * fix(tag): update tag's size when only icon, update one place of button's icon size (#2440) * fix(date-picker): [date-picker,select] fix radius bugs for month-table (#2442) * docs(site): fix demos and test e2e (#2444) * test(site): fix icon not correct test e2e (#2445) --------- Co-authored-by: chenxi-20 <[email protected]> Co-authored-by: 申君健 <[email protected]> Co-authored-by: MomoPoppy <[email protected]> Co-authored-by: gimmyhehe <[email protected]>
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
These changes enhance the reliability of the testing process, ensuring that the UI components behave as expected.