test(dialog-box): add ui-test #51
Conversation
WalkthroughA new test suite has been added for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
tests/dialog-box/xdesign.spec.ts (1)
4-12: LGTM! Consider adding a more specific selector for the dialog button.The test case for basic usage is well-structured and covers the essential aspects of the dialog-box component. Good job on including error handling and visual verification.
Consider using a more specific selector for the dialog button. Instead of:
await demo.getByRole('button', { name: /Dialog/ }).click()You could use:
await demo.getByRole('button', { name: 'Open Dialog', exact: true }).click()This would make the test more robust against potential changes in button text or multiple buttons containing "Dialog".
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
tests/dialog-box/xdesign.spec.ts-snapshots/basic-usage-chromium-win32.pngis excluded by!**/*.pngtests/dialog-box/xdesign.spec.ts-snapshots/center-chromium-win32.pngis excluded by!**/*.pngtests/dialog-box/xdesign.spec.ts-snapshots/open-close-events-chromium-win32.pngis excluded by!**/*.pngtests/dialog-box/xdesign.spec.ts-snapshots/right-dialog-chromium-win32.pngis excluded by!**/*.png
Files selected for processing (1)
- tests/dialog-box/xdesign.spec.ts (1 hunks)
| test('布局示例--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#open-close-events') | ||
| const demo = page.locator('#open-close-events') | ||
| await demo.getByRole('button', { name: '弹出与关闭事件' }).click() | ||
| const body = demo.locator('.tiny-dialog-box') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('open-close-events.png') | ||
| }) |
There was a problem hiding this comment.
Align test name with the tested functionality and consider adding event tests.
The test structure is consistent with the previous test, which is good. However, there are a couple of points to consider:
-
The test name "布局示例" (Layout example) doesn't match the section being tested ("open-close-events"). Consider renaming the test to reflect the actual functionality being tested.
-
Despite the section name "open-close-events", the test doesn't actually verify any open or close events. Consider adding assertions to check if these events are fired correctly.
Here's a suggested improvement:
test('Open and close events--UI screenshot', async ({ page }) => {
page.on('pageerror', (exception) => expect(exception).toBeNull())
await page.goto('dialog-box#open-close-events')
const demo = page.locator('#open-close-events')
// Set up event listeners
const openEventPromise = demo.evaluate(() => new Promise(resolve => {
document.querySelector('.tiny-dialog-box').addEventListener('open', resolve, { once: true })
}))
const closeEventPromise = demo.evaluate(() => new Promise(resolve => {
document.querySelector('.tiny-dialog-box').addEventListener('close', resolve, { once: true })
}))
// Open dialog
await demo.getByRole('button', { name: '弹出与关闭事件' }).click()
await openEventPromise
const body = demo.locator('.tiny-dialog-box')
await expect(body).toBeInViewport()
await expect(body).toHaveScreenshot('open-close-events-open.png')
// Close dialog
await demo.getByRole('button', { name: 'Close' }).click()
await closeEventPromise
await expect(body).not.toBeInViewport()
// Verify final state
await expect(page).toHaveScreenshot('open-close-events-closed.png')
})This improved version tests both the open and close events, as well as the visibility of the dialog box.
| test('右侧弹窗--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#right-dialog') | ||
| const demo = page.locator('#right-dialog') | ||
| await demo.getByRole('button', { name: '右侧弹窗' }).click() | ||
| const body = demo.locator('.is-right-slide') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('right-dialog.png') | ||
| }) |
There was a problem hiding this comment.
LGTM! Consider adding a position check for the right-side dialog.
The test case for the right-side dialog is well-structured and consistent with the previous tests. Good job on using the specific '.is-right-slide' class to target the right-side dialog.
To further improve the test, consider adding an assertion to verify that the dialog is actually positioned on the right side of the screen. You can do this by checking the bounding box of the dialog element. Here's a suggested addition:
const dialogBox = demo.locator('.is-right-slide');
const boundingBox = await dialogBox.boundingBox();
const pageWidth = await page.evaluate(() => window.innerWidth);
expect(boundingBox.x).toBeGreaterThan(pageWidth / 2);This assertion checks that the x-coordinate of the dialog box is in the right half of the screen, confirming its right-side positioning.
| // dialog-box#center | ||
| test('标题居中--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#center') | ||
| const demo = page.locator('#center') | ||
| await demo.getByRole('button', { name: /Dialog/ }).click() | ||
| const body = demo.locator('.tiny-dialog-box') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('center.png') | ||
| }) |
There was a problem hiding this comment.
Add specific assertion for centered title and remove unnecessary comment.
The test case for the centered title dialog is structurally consistent with the previous tests, which is good. However, there are a couple of improvements that can be made:
-
The test name suggests it's checking for a centered title, but there's no specific assertion for this. Consider adding an assertion to verify the title centering.
-
The comment on line 34 (
// dialog-box#center) is unnecessary as it's just repeating the URL. It can be removed to reduce clutter.
Here's a suggested improvement:
test('Centered title--UI screenshot', async ({ page }) => {
page.on('pageerror', (exception) => expect(exception).toBeNull())
await page.goto('dialog-box#center')
const demo = page.locator('#center')
await demo.getByRole('button', { name: /Dialog/ }).click()
const body = demo.locator('.tiny-dialog-box')
await expect(body).toBeInViewport()
// Check if the title is centered
const title = body.locator('.tiny-dialog-box__header');
const titleBox = await title.boundingBox();
const bodyBox = await body.boundingBox();
expect(titleBox.x + titleBox.width / 2).toBeCloseTo(bodyBox.x + bodyBox.width / 2, 1);
await expect(body).toHaveScreenshot('center.png')
})This improved version adds an assertion to check if the title is actually centered within the dialog box, and removes the unnecessary comment.
| import { expect, test } from '@playwright/test' | ||
|
|
||
| test.describe('dialog-box组件xdesign规范', () => { | ||
| test('基本用法--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#basic-usage') | ||
| const demo = page.locator('#basic-usage') | ||
| await demo.getByRole('button', { name: /Dialog/ }).click() | ||
| const body = demo.locator('.tiny-dialog-box') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('basic-usage.png') | ||
| }) | ||
|
|
||
| test('布局示例--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#open-close-events') | ||
| const demo = page.locator('#open-close-events') | ||
| await demo.getByRole('button', { name: '弹出与关闭事件' }).click() | ||
| const body = demo.locator('.tiny-dialog-box') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('open-close-events.png') | ||
| }) | ||
|
|
||
| test('右侧弹窗--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#right-dialog') | ||
| const demo = page.locator('#right-dialog') | ||
| await demo.getByRole('button', { name: '右侧弹窗' }).click() | ||
| const body = demo.locator('.is-right-slide') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('right-dialog.png') | ||
| }) | ||
|
|
||
| // dialog-box#center | ||
| test('标题居中--UI截图', async ({ page }) => { | ||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
| await page.goto('dialog-box#center') | ||
| const demo = page.locator('#center') | ||
| await demo.getByRole('button', { name: /Dialog/ }).click() | ||
| const body = demo.locator('.tiny-dialog-box') | ||
| await expect(body).toBeInViewport() | ||
| await expect(body).toHaveScreenshot('center.png') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Consider adding setup/teardown and improving test descriptions.
The overall structure of the test suite is good, with consistent error handling and use of screenshots for visual verification. However, there are a few suggestions for improvement:
-
Consider adding setup and teardown code using
test.beforeEach()andtest.afterEach(). This could handle common operations like navigating to the page and setting up error handling, reducing repetition in individual tests. -
Make test descriptions more consistent and informative. They should clearly indicate what aspect of the dialog-box is being tested.
-
Consider parameterizing the tests to reduce code duplication.
Here's an example of how you could refactor the suite:
import { expect, test } from '@playwright/test'
test.describe('dialog-box component xdesign specification', () => {
test.beforeEach(async ({ page }) => {
page.on('pageerror', (exception) => expect(exception).toBeNull())
await page.goto('dialog-box')
})
const testCases = [
{ name: 'Basic usage', selector: '#basic-usage', buttonText: 'Dialog', screenshotName: 'basic-usage.png' },
{ name: 'Open and close events', selector: '#open-close-events', buttonText: '弹出与关闭事件', screenshotName: 'open-close-events.png' },
{ name: 'Right-side dialog', selector: '#right-dialog', buttonText: '右侧弹窗', screenshotName: 'right-dialog.png', additionalClass: '.is-right-slide' },
{ name: 'Centered title', selector: '#center', buttonText: 'Dialog', screenshotName: 'center.png' },
]
for (const { name, selector, buttonText, screenshotName, additionalClass } of testCases) {
test(`${name} - UI screenshot`, async ({ page }) => {
const demo = page.locator(selector)
await demo.getByRole('button', { name: buttonText }).click()
const body = demo.locator(additionalClass ? `.tiny-dialog-box${additionalClass}` : '.tiny-dialog-box')
await expect(body).toBeInViewport()
await expect(body).toHaveScreenshot(screenshotName)
})
}
})This refactored version reduces code duplication, improves consistency, and makes it easier to add new test cases in the future.
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