-
Notifications
You must be signed in to change notification settings - Fork 17
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(time-select;drop-times): add e2e UI test #63
Conversation
WalkthroughThis pull request introduces new test suites for the 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 (
|
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: 3
Outside diff range and nitpick comments (8)
tests/drop-times/xdesign.spec.ts (5)
1-3
: Consider translating the test suite description to English.The test suite description is currently in Chinese. For consistency and to improve readability for non-Chinese speaking developers, consider translating it to English.
Suggested translation:
test.describe('drop-times component xdesign specification', () => {
4-10
: Approve test setup and suggest translating the test title to English.The test setup, including error handling, navigation, and initial screenshot capture, looks good. However, for consistency, consider translating the test title to English.
Suggested translation:
test('Default--UI screenshot', async ({ page }) => {
12-21
: Approve hover and focus interactions and suggest translating comments to English.The hover and focus interactions, along with their respective screenshot captures, are well implemented. However, for consistency and improved readability, consider translating the Chinese comments to English.
Suggested translations:
// Screenshot on hover // ... // Screenshot on focus
23-32
: Approve time selection interaction, correct comment, and suggest translation to English.The time selection interaction and screenshot captures are well implemented. However, there are a few improvements to be made:
- The comment on line 23 is incorrect and duplicated from the previous section. It should describe the time selection action instead of focusing.
- For consistency and improved readability, translate the comments to English.
Suggested changes:
// Screenshot on time option hover const select = page.getByText('00:30') await select.hover() await expect(bigPic).toBeInViewport() await expect(bigPic).toHaveScreenshot('time-option-hover.png') // Screenshot after time selection await select.click() await expect(demo).toBeInViewport() await expect(demo).toHaveScreenshot('time-selected.png')
1-34
: Approve overall test structure and coverage, suggest additional test cases.The test provides excellent coverage of the drop-times component's UI behavior, capturing screenshots at various interaction stages. The structure is clear and follows a logical flow of user interactions.
To further enhance the test suite, consider adding the following test cases:
- Test with different time selections to ensure consistent behavior.
- Verify the behavior when an invalid time is entered manually.
- Test the component's responsiveness by changing the viewport size.
- Verify keyboard navigation and accessibility features.
tests/time-select/xdesign.spec.ts (3)
1-3
: Consider using English for test descriptions.The test suite description is currently in Chinese. For better maintainability and consistency, especially in international teams, consider using English for all test descriptions.
-test.describe('time-select组件xdesign规范', () => { +test.describe('time-select component xdesign specification', () => {
12-21
: Consider using more specific screenshot names.The current screenshot names ('hover.png', 'focus.png') are descriptive but could be more specific to the time-select component. This would help differentiate these screenshots from similar interactions in other components.
Consider renaming the screenshots to be more specific:
- await expect(demo).toHaveScreenshot('hover.png') + await expect(demo).toHaveScreenshot('time-select-hover.png') - await expect(bigPic).toHaveScreenshot('focus.png') + await expect(bigPic).toHaveScreenshot('time-select-focus.png')
23-32
: Use more specific screenshot names for time selection interactions.Similar to the previous suggestion, the screenshot names for time selection interactions could be more specific to the time-select component and the action being performed.
Consider renaming the screenshots:
- await expect(bigPic).toHaveScreenshot('focus-hover.png') + await expect(bigPic).toHaveScreenshot('time-select-option-hover.png') - await expect(demo).toHaveScreenshot('select.png') + await expect(demo).toHaveScreenshot('time-select-option-selected.png')
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (12)
tests/drop-times/xdesign.spec.ts-snapshots/basic-usage-chromium-win32.png
is excluded by!**/*.png
tests/drop-times/xdesign.spec.ts-snapshots/click-chromium-win32.png
is excluded by!**/*.png
tests/drop-times/xdesign.spec.ts-snapshots/focus-hover-chromium-win32.png
is excluded by!**/*.png
tests/drop-times/xdesign.spec.ts-snapshots/hover-chromium-win32.png
is excluded by!**/*.png
tests/drop-times/xdesign.spec.ts-snapshots/select-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/basic-usage-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/focus-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/focus-hover-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/hover-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/icon-hover-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/select-chromium-win32.png
is excluded by!**/*.png
tests/time-select/xdesign.spec.ts-snapshots/select-hover-chromium-win32.png
is excluded by!**/*.png
Files selected for processing (2)
- tests/drop-times/xdesign.spec.ts (1 hunks)
- tests/time-select/xdesign.spec.ts (1 hunks)
test('默认--UI截图', async ({ page }) => { | ||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
await page.goto('time-select#basic-usage') | ||
const demo = page.locator('#basic-usage .pc-demo') | ||
const bigPic = page.locator('#doc-layout div').filter({ hasText: '示例API基本用法详细用法参考如下示例。time-' }).nth(2) | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('basic-usage.png') |
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
Use English for test titles and consider more robust navigation.
-
The test title is in Chinese. For consistency and better maintainability, consider using English for all test titles.
-
The navigation to the specific component demo uses a fragment identifier, which might be brittle if the page structure changes. Consider using a more robust method to navigate to the specific demo, such as environment variables or configuration files for URLs.
- test('默认--UI截图', async ({ page }) => {
+ test('Default - UI screenshot', async ({ page }) => {
Consider refactoring the navigation to use a more robust method:
const BASE_URL = process.env.TEST_BASE_URL || 'http://your-default-url.com';
// ...
await page.goto(`${BASE_URL}/time-select#basic-usage`);
// 选中后清除图标截图 | ||
const selectInput = demo.getByRole('textbox', { name: ':00' }) | ||
await selectInput.hover() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('select-hover.png') | ||
|
||
// 清除图标hover时截图 | ||
const icon = page.getByLabel('示例', { exact: true }).locator('path').nth(1) | ||
await icon.hover() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('icon-hover.png') |
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
Improve screenshot names and icon selection method.
-
The screenshot names could be more specific to the time-select component and the action being performed.
-
The method of selecting the clear icon using
getByLabel
andnth
might be brittle if the page structure changes. Consider using a more robust selector method.
Rename the screenshots for clarity:
- await expect(demo).toHaveScreenshot('select-hover.png')
+ await expect(demo).toHaveScreenshot('time-select-selected-hover.png')
- await expect(demo).toHaveScreenshot('icon-hover.png')
+ await expect(demo).toHaveScreenshot('time-select-clear-icon-hover.png')
Consider using a more robust selector for the clear icon:
const clearIcon = demo.locator('.clear-icon-class');
// or
const clearIcon = demo.getByRole('button', { name: 'Clear' });
Replace the existing icon selection with the more robust method:
- const icon = page.getByLabel('示例', { exact: true }).locator('path').nth(1)
- await icon.hover()
+ const clearIcon = demo.locator('.clear-icon-class');
+ await clearIcon.hover()
import { expect, test } from '@playwright/test' | ||
|
||
test.describe('time-select组件xdesign规范', () => { | ||
test('默认--UI截图', async ({ page }) => { | ||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
await page.goto('time-select#basic-usage') | ||
const demo = page.locator('#basic-usage .pc-demo') | ||
const bigPic = page.locator('#doc-layout div').filter({ hasText: '示例API基本用法详细用法参考如下示例。time-' }).nth(2) | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('basic-usage.png') | ||
|
||
// hover时截图 | ||
const input = demo.getByRole('textbox', { name: '选择时间' }) | ||
await input.hover() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('hover.png') | ||
|
||
// 聚焦时截图 | ||
await input.focus() | ||
await expect(bigPic).toBeInViewport() | ||
await expect(bigPic).toHaveScreenshot('focus.png') | ||
|
||
// 聚焦时截图 | ||
const select = page.getByText('10:00') | ||
await select.hover() | ||
await expect(bigPic).toBeInViewport() | ||
await expect(bigPic).toHaveScreenshot('focus-hover.png') | ||
|
||
// 选中时截图 | ||
await select.click() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('select.png') | ||
|
||
// 选中后清除图标截图 | ||
const selectInput = demo.getByRole('textbox', { name: ':00' }) | ||
await selectInput.hover() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('select-hover.png') | ||
|
||
// 清除图标hover时截图 | ||
const icon = page.getByLabel('示例', { exact: true }).locator('path').nth(1) | ||
await icon.hover() | ||
await expect(demo).toBeInViewport() | ||
await expect(demo).toHaveScreenshot('icon-hover.png') | ||
}) | ||
}) |
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 adding more test cases for improved coverage.
While the current test provides good coverage of basic user interactions, consider adding the following test cases to improve overall coverage:
- Verify the selected time value is correctly displayed in the input field.
- Test error states (e.g., selecting an invalid time).
- Test edge cases (e.g., selecting the first and last available times).
- Test keyboard navigation and accessibility features.
Here's an example of how you might add a test to verify the selected time value:
test('Verify selected time value', async ({ page }) => {
// ... (setup code) ...
const timeOption = page.getByText('10:00');
await timeOption.click();
const input = demo.getByRole('textbox', { name: '选择时间' });
await expect(input).toHaveValue('10:00');
});
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
New Features
drop-times
component to validate UI behavior through various user interactions.time-select
component, ensuring visual accuracy during user interactions.Tests