Skip to content
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

Merged
merged 3 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,44 +24,44 @@
</template>

<script setup lang="jsx">
import { TinyButton, TinyModal, TinyNotify } from '@opentiny/vue'
import { Button as TinyButton, Modal, Notify } from '@opentiny/vue'
import { ref } from 'vue'

const TinyModal = Modal
const show1 = ref(false)
const show2 = ref(false)

function baseClick() {
const modal = TinyModal.alert('基本提示框', '标题')
const modal = Modal.alert('基本提示框', '标题')
setTimeout(() => modal.vm.close(), 3000)
}
Comment on lines +35 to 37
Copy link

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.

 function baseClick() {
-  const modal = Modal.alert('基本提示框', '标题')
-  setTimeout(() => modal.vm.close(), 3000)
+  const modal = Modal.alert('基本提示框', '标题')
+  const timer = setTimeout(() => modal.vm.close(), 3000)
+  modal.vm.$once('close', () => clearTimeout(timer))
 }
📝 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.

Suggested change
const modal = Modal.alert('基本提示框', '标题')
setTimeout(() => modal.vm.close(), 3000)
}
function baseClick() {
const modal = Modal.alert('基本提示框', '标题')
const timer = setTimeout(() => modal.vm.close(), 3000)
modal.vm.$once('close', () => clearTimeout(timer))
}


function successClick() {
TinyModal.alert({ message: '成功提示框', status: 'success' })
Modal.alert({ message: '成功提示框', status: 'success' })
}

function confirmClick() {
TinyModal.confirm('您确定要删除吗?').then((res) => {
TinyNotify({
Modal.confirm('您确定要删除吗?').then((res) => {
Notify({
type: 'info',
title: '触发回调事件',
message: `点击${res}按钮`
})
})
}
function jsxClick() {
TinyModal.alert({
Modal.alert({
message: (
<div>
<button> some button </button>
<b> some text </b>
</div>
),

status: 'success'
})
}
function messageClick() {
TinyModal.message('简单的消息')
Modal.message('简单的消息')
}
</script>

Expand Down
33 changes: 33 additions & 0 deletions examples/sites/demos/pc/app/modal/basic-usage.spec.ts
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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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)

Committable suggestion skipped: line range outside the PR's diff.

})
Comment on lines +10 to +33
Copy link

Choose a reason for hiding this comment

The 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:

  1. Add negative 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();
  1. Add accessibility checks:
// 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();
  1. Add keyboard interaction tests:
// Test keyboard navigation
await page.getByRole('button', { name: '基本提示框' }).click();
await page.keyboard.press('Enter');
await expect(modal).not.toBeVisible();

19 changes: 9 additions & 10 deletions examples/sites/demos/pc/app/modal/basic-usage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)
+    }
+  }
 }

Committable suggestion skipped: line range outside the PR's diff.

},
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
Copy link

Choose a reason for hiding this comment

The 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: '用户取消了操作'
+    })
   })
 }

Committable suggestion skipped: line range outside the PR's diff.

},
jsxClick() {
TinyModal.alert({
Modal.alert({
message: (
<div>
<button>some button</button>
<b>some text</b>
</div>
),

status: 'success'
})
},
messageClick() {
TinyModal.message('简单的消息')
Modal.message('简单的消息')
}
}
}
Expand Down
17 changes: 0 additions & 17 deletions examples/sites/demos/pc/app/modal/footer-slot-composition-api.vue

This file was deleted.

26 changes: 0 additions & 26 deletions examples/sites/demos/pc/app/modal/footer-slot.vue

This file was deleted.

This file was deleted.

40 changes: 0 additions & 40 deletions examples/sites/demos/pc/app/modal/message-closable.vue

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
</template>

<script setup>
import { TinyButton, TinyModal } from '@opentiny/vue'
import { Button as TinyButton, Modal } from '@opentiny/vue'

function btnClick() {
TinyModal.confirm({
Modal.message({
status: 'info',
message: '自定义消息的内容可关闭,5s 后得自动关闭',
messageClosable: true,
Expand Down
9 changes: 9 additions & 0 deletions examples/sites/demos/pc/app/modal/message-close.spec.ts
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 后得自动关闭/)
})
6 changes: 3 additions & 3 deletions examples/sites/demos/pc/app/modal/message-close.vue
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
</template>

<script>
import { TinyButton, TinyModal } from '@opentiny/vue'
import { Button, Modal } from '@opentiny/vue'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Template and import mismatch needs resolution

The template uses tiny-button but the component is imported as Button. While it's registered with the TinyButton alias, the template should be updated to maintain consistency with the broader refactoring effort mentioned in the PR.

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>

Committable suggestion skipped: line range outside the PR's diff.


export default {
components: {
TinyButton
TinyButton: Button
Copy link

Choose a reason for hiding this comment

The 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 Button. Consider using the component directly without an alias.

Apply this diff:

  components: {
-    TinyButton: Button
+    Button
  },
📝 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.

Suggested change
TinyButton: Button
Button

},
methods: {
btnClick() {
TinyModal.message({
Modal.message({
status: 'info',
message: '自定义消息的内容可关闭,5s 后得自动关闭',
messageClosable: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
</template>

<script setup>
import { TinyButton, TinyModal } from '@opentiny/vue'
import { Button as TinyButton, Modal } from '@opentiny/vue'

function btnClick() {
TinyModal.message({
Modal.message({
message: '自定义消息的内容',
events: {
show(ev) {
Expand All @@ -25,7 +25,7 @@ function btnClick() {
}

function handleEvent(eventName, eventArgs) {
TinyModal.message({ status: 'info', message: `${eventName} 事件触发了` })
Modal.message({ status: 'info', message: `${eventName} 事件触发了` })
console.log({ eventName, eventArgs })
}
</script>
Expand Down
10 changes: 10 additions & 0 deletions examples/sites/demos/pc/app/modal/message-event.spec.ts
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())
Copy link

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
page.on('pageerror', (exception) => expect(exception).toBeNull())
page.on('pageerror', (exception) => {
console.error('Page error:', exception);
expect(exception).toBeNull();
})

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
Copy link

Choose a reason for hiding this comment

The 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:

  1. Using class selectors (.is-message) is fragile and could break with styling changes
  2. Hardcoded Chinese text in assertions makes the test less maintainable
  3. Using nth selectors without clear context is prone to errors
-  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));

Committable suggestion skipped: line range outside the PR's diff.

})
Comment on lines +1 to +10
Copy link

Choose a reason for hiding this comment

The 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:

  • Function-call modal events via Modal.confirm()
  • Tag-based modal events
  • Zoom functionality for resizable modals
  • Cancel and close interactions
  • Hide event verification
🔗 Analysis chain

Verify test coverage against component changes

Given that this is a new e2e test for the modal component that's being refactored from TinyModal to Modal, we should verify that all critical paths are covered.


Let me gather more specific information about the event handling in the modal components to verify the test coverage.

🏁 Scripts executed

The 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

8 changes: 4 additions & 4 deletions examples/sites/demos/pc/app/modal/message-event.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
</template>

<script>
import { TinyButton, TinyModal } from '@opentiny/vue'
import { Button, Modal } from '@opentiny/vue'
Copy link

Choose a reason for hiding this comment

The 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:

  • The import uses Button
  • The component is registered as TinyButton
  • The template uses <tiny-button>

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) {
Expand All @@ -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 })
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
</template>

<script setup>
import { TinyButton, TinyModal } from '@opentiny/vue'
import { Button as TinyButton, Modal } from '@opentiny/vue'

function btnClick() {
TinyModal.confirm({
Modal.message({
status: 'info',
message: '自定义消息具有唯一 id,所以不会重复出现',
id: 'unique'
Expand Down
9 changes: 9 additions & 0 deletions examples/sites/demos/pc/app/modal/message-id.spec.ts
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
Copy link

Choose a reason for hiding this comment

The 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:

  1. The test should simulate multiple clicks to properly verify duplicate prevention
  2. Add a wait for modal animations to complete
  3. Add more descriptive test name and comments in English for better maintainability

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

‼️ 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.

Suggested change
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/)
})
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')
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)
})

Loading