-
Notifications
You must be signed in to change notification settings - Fork 271
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
feat(modal): [modal] add e2e-test and update modal demo of problem #2567
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import { test, expect } from '@playwright/test' | ||
|
||
test('基本用法', async ({ page }) => { | ||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
await page.goto('modal#basic-usage') | ||
|
||
const modal = page.locator('.tiny-modal__status-wrapper svg').first() | ||
const content = page.locator('.tiny-modal__content') | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use more robust selectors for testing. Current selectors rely on CSS classes which can be fragile. Consider using test-specific attributes: - const modal = page.locator('.tiny-modal__status-wrapper svg').first()
- const content = page.locator('.tiny-modal__content')
+ const modal = page.locator('[data-testid="modal-status-icon"]')
+ const content = page.locator('[data-testid="modal-content"]') You'll need to add these data attributes to your modal component: <template>
<svg data-testid="modal-status-icon">...</svg>
<div data-testid="modal-content">...</div>
</template> |
||
|
||
// 基本提示框 | ||
await page.getByRole('button', { name: '基本提示框' }).click() | ||
await page.getByRole('button', { name: '确定' }).click() | ||
await expect(modal).not.toBeVisible() | ||
|
||
// 成功提示框 | ||
await page.getByRole('button', { name: '成功提示框' }).click() | ||
await expect(modal).toHaveClass(/tiny-modal-svg__success/) | ||
await page.getByRole('button', { name: '确定' }).click() | ||
|
||
// 消息提示 | ||
await page.getByRole('button', { name: '消息提示' }).click() | ||
await expect(content.nth(3)).toHaveText(/简单的消息/) | ||
|
||
// 打开弹窗1 | ||
await page.getByRole('button', { name: '打开弹窗1' }).click() | ||
await expect(content.nth(1)).toHaveText(/窗口内容1/) | ||
await page.getByRole('button', { name: '确定' }).click() | ||
|
||
// 打开弹窗2 | ||
await page.getByRole('button', { name: '打开弹窗2' }).click() | ||
await expect(content.nth(2)).toHaveText(/窗口内容2/) | ||
await page.getByRole('button', { name: '确定' }).click() | ||
Comment on lines
+11
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add proper wait conditions to prevent flaky tests. The current tests might be flaky due to timing issues. Consider adding explicit wait conditions: // 基本提示框
await page.getByRole('button', { name: '基本提示框' }).click()
+ await expect(modal).toBeVisible()
await page.getByRole('button', { name: '确定' }).click()
+ await expect(modal).toHaveCount(0)
- await expect(modal).not.toBeVisible()
// 成功提示框
await page.getByRole('button', { name: '成功提示框' }).click()
+ await expect(modal).toBeVisible()
await expect(modal).toHaveClass(/tiny-modal-svg__success/)
await page.getByRole('button', { name: '确定' }).click()
+ await expect(modal).toHaveCount(0)
|
||
}) | ||
Comment on lines
+10
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test coverage with additional scenarios. While the basic scenarios are covered, consider adding these essential test cases:
// Test modal doesn't close when clicking outside
await page.getByRole('button', { name: '基本提示框' }).click();
await page.click('body', { position: { x: 0, y: 0 } });
await expect(modal).toBeVisible();
// Test modal with invalid inputs
await page.getByRole('button', { name: '基本提示框' }).click();
await page.keyboard.press('Escape');
await expect(modal).not.toBeVisible();
// Test modal trap focus
await page.getByRole('button', { name: '基本提示框' }).click();
await expect(page.locator('dialog[role="dialog"]')).toHaveAttribute('aria-modal', 'true');
await page.keyboard.press('Tab');
await expect(page.getByRole('button', { name: '确定' })).toBeFocused();
// Test keyboard navigation
await page.getByRole('button', { name: '基本提示框' }).click();
await page.keyboard.press('Enter');
await expect(modal).not.toBeVisible(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,12 +24,12 @@ | |
</template> | ||
|
||
<script lang="jsx"> | ||
import { TinyButton, TinyModal, TinyNotify } from '@opentiny/vue' | ||
import { Button, Modal, Notify } from '@opentiny/vue' | ||
|
||
export default { | ||
components: { | ||
TinyButton, | ||
TinyModal | ||
TinyButton: Button, | ||
TinyModal: Modal | ||
}, | ||
data() { | ||
return { | ||
|
@@ -39,35 +39,34 @@ export default { | |
}, | ||
methods: { | ||
baseClick() { | ||
const modal = TinyModal.alert('基本提示框', '标题') | ||
const modal = Modal.alert('基本提示框', '标题') | ||
setTimeout(() => modal.vm.close(), 3000) | ||
Comment on lines
+42
to
43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add cleanup for setTimeout on component unmount The setTimeout could lead to memory leaks if the component unmounts before the timeout completes. Consider clearing the timeout when the component unmounts. export default {
+ data() {
+ return {
+ modalTimer: null
+ }
+ },
methods: {
baseClick() {
const modal = Modal.alert('基本提示框', '标题')
- setTimeout(() => modal.vm.close(), 3000)
+ this.modalTimer = setTimeout(() => modal.vm.close(), 3000)
}
},
+ beforeDestroy() {
+ if (this.modalTimer) {
+ clearTimeout(this.modalTimer)
+ }
+ }
}
|
||
}, | ||
successClick() { | ||
TinyModal.alert({ message: '成功提示框', status: 'success' }) | ||
Modal.alert({ message: '成功提示框', status: 'success' }) | ||
}, | ||
confirmClick() { | ||
TinyModal.confirm('您确定要删除吗?').then((res) => { | ||
TinyNotify({ | ||
Modal.confirm('您确定要删除吗?').then((res) => { | ||
Notify({ | ||
type: 'info', | ||
title: '触发回调事件', | ||
message: `点击${res}按钮` | ||
}) | ||
}) | ||
Comment on lines
+49
to
55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling for Modal.confirm promise The promise chain should include error handling to manage rejection cases. confirmClick() {
Modal.confirm('您确定要删除吗?').then((res) => {
Notify({
type: 'info',
title: '触发回调事件',
message: `点击${res}按钮`
})
+ }).catch(error => {
+ Notify({
+ type: 'error',
+ title: '操作取消',
+ message: '用户取消了操作'
+ })
})
}
|
||
}, | ||
jsxClick() { | ||
TinyModal.alert({ | ||
Modal.alert({ | ||
message: ( | ||
<div> | ||
<button>some button</button> | ||
<b>some text</b> | ||
</div> | ||
), | ||
|
||
status: 'success' | ||
}) | ||
}, | ||
messageClick() { | ||
TinyModal.message('简单的消息') | ||
Modal.message('简单的消息') | ||
} | ||
} | ||
} | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { test, expect } from '@playwright/test' | ||
|
||
test('消息的关闭和延时', async ({ page }) => { | ||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||
await page.goto('modal#message-close') | ||
const content = page.locator('.tiny-modal__text') | ||
await page.getByRole('button', { name: '消息可关闭' }).click() | ||
await expect(content).toHaveText(/5s 后得自动关闭/) | ||
}) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7,15 +7,15 @@ | |||||
</template> | ||||||
|
||||||
<script> | ||||||
import { TinyButton, TinyModal } from '@opentiny/vue' | ||||||
import { Button, Modal } from '@opentiny/vue' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Template and import mismatch needs resolution The template uses Apply this diff to fix the template: <template>
<div>
<div class="content">
- <tiny-button @click="btnClick">消息可关闭</tiny-button>
+ <Button @click="btnClick">消息可关闭</Button>
</div>
</div>
</template>
|
||||||
|
||||||
export default { | ||||||
components: { | ||||||
TinyButton | ||||||
TinyButton: Button | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify component registration The component registration uses an alias that contradicts the refactoring effort to standardize on Apply this diff: components: {
- TinyButton: Button
+ Button
}, 📝 Committable suggestion
Suggested change
|
||||||
}, | ||||||
methods: { | ||||||
btnClick() { | ||||||
TinyModal.message({ | ||||||
Modal.message({ | ||||||
status: 'info', | ||||||
message: '自定义消息的内容可关闭,5s 后得自动关闭', | ||||||
messageClosable: true, | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,10 @@ | ||||||||||||
import { test, expect } from '@playwright/test' | ||||||||||||
|
||||||||||||
test('消息的事件', async ({ page }) => { | ||||||||||||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance error handling with specific error information The current error handler silently expects all errors to be null. Consider logging the error details for better debugging. - page.on('pageerror', (exception) => expect(exception).toBeNull())
+ page.on('pageerror', (exception) => {
+ console.error('Page error:', exception);
+ expect(exception).toBeNull();
+ }) 📝 Committable suggestion
Suggested change
|
||||||||||||
await page.goto('modal#message-event') | ||||||||||||
const content = page.locator('.is-message') | ||||||||||||
await page.getByRole('button', { name: '打开带事件消息' }).first().click() | ||||||||||||
await expect(content.first()).toHaveText(/自定义消息的内容/) | ||||||||||||
await expect(content.nth(1)).toHaveText(/show 事件触发了/) | ||||||||||||
Comment on lines
+6
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve selector reliability and test maintainability The current implementation has several areas for improvement:
- const content = page.locator('.is-message')
- await page.getByRole('button', { name: '打开带事件消息' }).first().click()
- await expect(content.first()).toHaveText(/自定义消息的内容/)
- await expect(content.nth(1)).toHaveText(/show 事件触发了/)
+ // Define test constants
+ const MESSAGES = {
+ CUSTOM_CONTENT: '自定义消息的内容',
+ SHOW_EVENT: 'show 事件触发了'
+ };
+
+ // Use more specific selectors
+ const messageContainer = page.locator('[data-testid="modal-message-container"]');
+ const openButton = page.getByRole('button', { name: '打开带事件消息' });
+
+ // Perform actions
+ await openButton.click();
+
+ // Assert results with clear context
+ await expect(messageContainer.locator('[data-testid="custom-message"]'))
+ .toHaveText(new RegExp(MESSAGES.CUSTOM_CONTENT));
+ await expect(messageContainer.locator('[data-testid="event-message"]'))
+ .toHaveText(new RegExp(MESSAGES.SHOW_EVENT));
|
||||||||||||
}) | ||||||||||||
Comment on lines
+1
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Test coverage needs expansion for modal events The current test only verifies the 'show' event and message content, but the component supports multiple critical events (hide, confirm, cancel, close, zoom) in both function-call and tag-based usage. The test coverage should be expanded to include:
🔗 Analysis chainVerify test coverage against component changes Given that this is a new e2e test for the modal component that's being refactored from Let me gather more specific information about the event handling in the modal components to verify the test coverage. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for modal-related component changes and verify test coverage
# Find all modal-related component files
echo "Looking for modal component files..."
fd -e vue -e ts "modal" examples/sites/demos/pc/app/modal/
# Check for TinyModal to Modal migration patterns
echo "Checking for component migration patterns..."
rg -l "TinyModal" examples/sites/demos/pc/app/modal/
# Look for event-related code that might need testing
echo "Checking for event handlers that might need testing..."
ast-grep --pattern 'on$_($$$)' examples/sites/demos/pc/app/modal/
Length of output: 3291 Script: #!/bin/bash
# Let's examine the event-related code more closely
# Check the implementation of message-event component
echo "Checking message-event implementation..."
cat examples/sites/demos/pc/app/modal/modal-event.vue
# Check for all event-related tests
echo "Looking for event-related tests..."
rg -l "test.*event" examples/sites/demos/pc/app/modal/
# Look for event-related patterns in modal components
echo "Checking event patterns in components..."
rg -A 5 "@(show|hide|close|open)" examples/sites/demos/pc/app/modal/modal-event.vue
Length of output: 2974 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,18 +5,18 @@ | |
</template> | ||
|
||
<script> | ||
import { TinyButton, TinyModal } from '@opentiny/vue' | ||
import { Button, Modal } from '@opentiny/vue' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Maintain consistent component naming There's an inconsistency in the component naming:
This mixing of naming conventions could lead to confusion. Consider applying this change for consistency: import { Button, Modal } from '@opentiny/vue'
export default {
components: {
- TinyButton: Button
+ Button
}
} And update the template: - <tiny-button @click="btnClick">打开带事件消息</tiny-button>
+ <Button @click="btnClick">打开带事件消息</Button> Also applies to: 12-12 |
||
|
||
export default { | ||
components: { | ||
TinyButton | ||
TinyButton: Button | ||
}, | ||
methods: { | ||
btnClick() { | ||
// eslint-disable-next-line @typescript-eslint/no-this-alias | ||
const $this = this | ||
|
||
TinyModal.message({ | ||
Modal.message({ | ||
message: '自定义消息的内容', | ||
events: { | ||
show(ev) { | ||
|
@@ -32,7 +32,7 @@ export default { | |
}) | ||
}, | ||
handleEvent(eventName, eventArgs) { | ||
TinyModal.message({ status: 'info', message: `${eventName} 事件触发了` }) | ||
Modal.message({ status: 'info', message: `${eventName} 事件触发了` }) | ||
console.log({ eventName, eventArgs }) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,9 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { test, expect } from '@playwright/test' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
test('防止重复消息提示', async ({ page }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
page.on('pageerror', (exception) => expect(exception).toBeNull()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await page.goto('modal#message-id') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const content = page.locator('.tiny-modal__text') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await page.getByRole('button', { name: '重复点击不出现多个' }).click() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await expect(content).toHaveText(/唯一 id/) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+3
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test coverage for duplicate message prevention. While the basic test structure is good, consider these improvements to make the test more robust:
Here's a suggested implementation: -test('防止重复消息提示', async ({ page }) => {
+test('Modal - should prevent duplicate messages when button is clicked multiple times', async ({ page }) => {
page.on('pageerror', (exception) => expect(exception).toBeNull())
await page.goto('modal#message-id')
const content = page.locator('.tiny-modal__text')
- await page.getByRole('button', { name: '重复点击不出现多个' }).click()
+ const button = page.getByRole('button', { name: '重复点击不出现多个' })
+
+ // Simulate multiple rapid clicks
+ await Promise.all([
+ button.click(),
+ button.click(),
+ button.click()
+ ])
+
+ // Wait for any animations to complete
+ await page.waitForTimeout(500)
+
await expect(content).toHaveText(/唯一 id/)
+
+ // Verify only one modal is present
+ await expect(page.locator('.tiny-modal__text')).toHaveCount(1)
}) 📝 Committable suggestion
Suggested change
|
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
Add cleanup for setTimeout
The setTimeout should be cleaned up if the modal is closed before the timer expires to prevent potential memory leaks.
📝 Committable suggestion