test: update @playwright/test to 1.49.x and fix e2e case #2792
Conversation
WalkthroughThis pull request encompasses a comprehensive update to the Changes
Suggested labels
Poem
Finishing Touches
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 (
|
WalkthroughThis pull request updates the Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
package.json
Outdated
| @@ -210,7 +210,7 @@ | |||
| "@vue/runtime-core@3": "3.4.38", | |||
| "@vue/runtime-dom@3": "3.4.38", | |||
| "@vue/shared@3": "3.4.38", | |||
| "@playwright/test": "1.42.1", | |||
| "@playwright/test": "1.49.1", | |||
There was a problem hiding this comment.
The update of @playwright/test to version 1.49.1 should be tested to ensure compatibility with existing tests and configurations. Please verify that all tests pass successfully with this new version.
gimmyhehe
changed the title
gimmyhehe
changed the title
|
[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 |
package.json
Outdated
| @@ -210,7 +210,7 @@ | |||
| "@vue/runtime-core@3": "3.4.38", | |||
| "@vue/runtime-dom@3": "3.4.38", | |||
| "@vue/shared@3": "3.4.38", | |||
| "@playwright/test": "1.42.1", | |||
| "@playwright/test": "1.49.1", | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (14)
examples/sites/demos/pc/app/grid/validation/tipconfig.spec.js (1)
5-5: Consider using more specific selectors for better test reliability.While the simplified selector approach is cleaner, using
.tiny-grid-default-inputmight be too generic and could match multiple elements. Consider using a more specific selector combining the demo container with a test ID:- const demo = page.locator('#validation-tipconfig') + const demo = page.locator('[data-test="validation-tipconfig"]') - await demo.locator('.tiny-grid-default-input').fill('') + await demo.locator('[data-test="grid-input"]').fill('')Also applies to: 8-8
examples/sites/demos/pc/app/grid/validation/row-data-valid.spec.js (1)
Line range hint
1-1: Consider implementing a shared test utility for grid interactions.The changes across these files show a common pattern of grid interaction. To improve maintainability and reliability, consider:
- Creating a shared test utility for grid operations
- Implementing custom test fixtures for common setup
- Defining consistent selectors in a central configuration
This would help standardize the approach across all grid-related tests while making them more maintainable.
examples/sites/demos/pc/app/grid/validation/editing-validation.spec.js (1)
5-5: Consider using data-testid attributes for more reliable selectors.While the refactoring improves code maintainability, class-based selectors like
.tiny-grid-default-inputcould be fragile if CSS classes change. Consider adding data-testid attributes for more reliable element selection.Example implementation:
- const demo = page.locator('#validation-editing-validation') + const demo = page.locator('[data-testid="validation-editing-validation"]') - await demo.locator('.tiny-grid-default-input').fill('') + await demo.locator('[data-testid="grid-input"]').fill('')Also applies to: 8-8
examples/sites/demos/pc/app/grid/validation/editing-isvalidalways-validation.spec.js (2)
5-6: Remove unnecessary empty line.Remove the extra empty line to maintain consistent spacing.
8-8: Make element selection more specific.Using
first()without more context could be fragile if the page structure changes. Consider using a more specific selector or data-testid attribute.Example:
- await demo.locator('.tiny-grid-default-input').first().fill('GF') + await demo.locator('[data-testid="grid-input-name"]').fill('GF')examples/sites/demos/pc/app/grid/validation/select-validation.spec.js (1)
11-13: Add assertions between actions.Multiple sequential actions without intermediate assertions can make it harder to diagnose failures. Consider adding expectations after each significant action.
Example:
await demo.locator('.tiny-grid-body__row .tiny-grid-checkbox__icon').first().click() + await expect(demo.locator('.tiny-grid-checkbox__icon').first()).toHaveClass(/checked/) await page.getByRole('button', { name: '校验选中数据' }).click()examples/sites/demos/pc/app/grid/edit/trigger-mode-hm-editing.spec.js (1)
5-5: Consider using data-testid for more reliable element selection.While the simplified locator strategy is more maintainable, using
.first()without proper filtering could make tests flaky if the order of elements changes. Consider:
- Adding data-testid attributes for unique identification
- Using more specific filters when selecting elements
Example implementation:
- const demo = page.locator('#edit-trigger-mode-hm-editing') + const demo = page.locator('[data-testid="edit-trigger-mode-hm"]') - await demo.locator('.tiny-grid-body__row').first().getByRole('button', { name: '编辑' }).click() + await demo.locator('[data-testid="grid-row-1"] [data-testid="edit-button"]').click()Also applies to: 9-10
examples/sites/demos/pc/app/grid/validation/before-submit-validation.spec.js (1)
Line range hint
11-15: Consider using data-testid for button selection.Using role and name for button selection could be fragile if text changes. Consider using data-testid attributes.
- await page.getByRole('button', { name: '提交数据' }).click() + await demo.locator('[data-testid="submit-button"]').click() await expect(page.getByText('校验不通过', { exact: true })).toBeVisible() - await page.getByRole('button', { name: '确定' }).click() + await demo.locator('[data-testid="confirm-button"]').click() - await page.getByRole('button', { name: '保存 Promise' }).click() + await demo.locator('[data-testid="save-promise-button"]').click()examples/sites/demos/pc/app/grid/edit/editing.spec.js (1)
8-8: Consider maintaining role-based selectors for better accessibility testingWhile using
.tiny-grid-default-inputsimplifies the selector, role-based selectors (e.g.,getByRole('textbox')) are recommended by Playwright for better accessibility testing and maintenance. They ensure your application remains accessible while making tests more resilient to UI changes.- await page.locator('.tiny-grid-default-input').fill('GFD 科技 Y 水电费第三方 X 公司') + await page.getByRole('textbox').first().fill('GFD 科技 Y 水电费第三方 X 公司')examples/sites/demos/pc/app/grid/edit/status-of-editing.spec.js (1)
7-7: Maintain consistent use of role-based selectors across test filesSimilar to other test files, consider using role-based selectors instead of class selectors for better accessibility testing and maintenance.
- await page.locator('.tiny-grid-default-input').fill('sdfdf') + await page.getByRole('textbox').first().fill('sdfdf')examples/sites/package.json (1)
80-80: Consider documenting the purpose of rollup-plugin-visualizerWhile this addition is beneficial for bundle analysis, it would be helpful to document its intended use in the project.
examples/vue3/package.json (1)
33-33: Consider documenting browser installation requirementThe update to
@playwright/testis consistent. Given theinstall:browserscript in this package, consider adding documentation about browser installation requirements after upgrading to 1.49.x.examples/vue2/package.json (2)
30-30: Document catalog-based version managementThe consistent use of
"catalog:"across multiple dependencies suggests a centralized version management strategy. Consider documenting this approach in the project's contribution guidelines.
30-30: Consider implementing a centralized dependency management solutionWith the consistent use of
"catalog:"across multiple package.json files, consider implementing a more robust solution like npm workspaces or Lerna for better dependency management across the monorepo.Also applies to: 33-33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
examples/docs/package.json(1 hunks)examples/sites/demos/pc/app/grid/edit/custom-editing.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/editing.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/has-row-change.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/revert-data.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/status-of-editing.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/trigger-mode-for-editing.spec.js(1 hunks)examples/sites/demos/pc/app/grid/edit/trigger-mode-hm-editing.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/before-submit-validation.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/custcomp.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/editing-isvalidalways-validation.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/editing-validation.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/row-data-valid.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/select-validation.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/tipconfig.spec.js(1 hunks)examples/sites/demos/pc/app/grid/validation/validation-scroll-to-col.spec.js(1 hunks)examples/sites/package.json(3 hunks)examples/vue2.7/package.json(3 hunks)examples/vue2/package.json(1 hunks)examples/vue3/package.json(1 hunks)internals/playwright-config/package.json(1 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🔇 Additional comments (11)
examples/sites/demos/pc/app/grid/edit/has-row-change.spec.js (1)
5-12: Well-structured use of scoped selectors!Good practice using a scoped locator with
#edit-has-row-change. This approach:
- Improves test reliability by limiting the selection scope
- Reduces potential conflicts with similar elements outside the grid
- Makes the test more maintainable
pnpm-workspace.yaml (2)
12-12: Appropriate version constraint for Playwright updateThe addition of
@playwright/test: '~1.49.0'to the catalog:
- Addresses the Ubuntu 24.04 compatibility issue
- Uses appropriate tilde range for patch updates
- Maintains consistent versioning across the workspace
12-12: Verify Playwright compatibility and breaking changesBefore merging, please verify:
- The compatibility with Ubuntu 24.04 environments
- Any breaking changes in Playwright 1.49.x that might affect existing tests
Run this script to check Playwright's changelog and known issues:
✅ Verification successful
Version change to ~1.49.0 is safe and compatible
The change is verified to be safe because:
- It matches the current Playwright version (1.49.1) in use
- Restricts to patch updates only, providing better version control
- No OS-specific configurations that would affect Ubuntu 24.04 compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Playwright's changelog for breaking changes gh api repos/microsoft/playwright/releases | jq -r '.[].body' | grep -A 10 "1.49.0" # Check for any open issues related to Ubuntu 24.04 gh api search/issues -f q="repo:microsoft/playwright ubuntu 24.04 in:title,body" | jq '.items[] | {title, html_url}'Length of output: 293
Script:
#!/bin/bash # Find Playwright config files and test files echo "=== Playwright Files ===" fd -e ts -e js -e mjs "playwright.config" echo -e "\n=== Test Files ===" fd -e spec.ts -e test.ts -e spec.js -e test.js echo -e "\n=== Package Files with Playwright ===" fd package.json -x grep -l "playwright" {} echo -e "\n=== CI Configuration Files ===" fd -e yml -e yaml . .github/workflows/Length of output: 74155
Script:
#!/bin/bash # Check Playwright configuration files echo "=== Vue3 Playwright Config ===" cat examples/vue3/playwright.config.js echo -e "\n=== Vue2 Playwright Config ===" cat examples/vue2/playwright.config.js echo -e "\n=== Vue2.7 Playwright Config ===" cat examples/vue2.7/playwright.config.js # Check package.json files for current Playwright version echo -e "\n=== Current Playwright Version ===" find . -name "package.json" -exec grep -l "@playwright/test" {} \; -exec grep "@playwright/test" {} \;Length of output: 2051
internals/playwright-config/package.json (3)
Line range hint
23-23: Consistent dependency management approach across workspaceThe change to
"catalog:"for@playwright/testis consistently applied across all workspace packages. This centralized version management approach is good practice for maintaining version consistency.Let's verify the workspace dependency resolution:
Also applies to: 30-30, 33-33, 30-30
✅ Verification successful
Workspace dependency configuration verified successfully
- All packages using
@playwright/testconsistently use the "catalog:" version- Vue example projects (vue2, vue2.7, vue3) correctly use the internal playwright config
- Different configuration for docs and sites examples appears intentional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent @playwright/test versions across workspace echo "Checking @playwright/test version consistency across workspace..." find . -name 'package.json' -type f -exec grep -l '"@playwright/test"' {} \; | while read -r file; do echo "=== $file ===" jq -r '.devDependencies["@playwright/test"]' "$file" done # Verify if all packages using the internal playwright config echo "Checking internal playwright config usage..." find . -name 'package.json' -type f -exec grep -l '"@opentiny-internal/playwright-config"' {} \;Length of output: 1848
13-13: Verify catalog resolution to version 1.49.xThe change from explicit version
~1.42.0to"catalog:"needs verification to ensure it resolves to version 1.49.x as intended by the PR.Let's verify the catalog resolution:
✅ Verification successful
Version resolves correctly to 1.49.1
The "catalog:" version in playwright-config is resolved to version 1.49.1 through pnpm workspace overrides, which matches the PR's intention to upgrade to 1.49.x.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the resolved version of @playwright/test npm info @playwright/test versions --json | jq -r '.[] | select(startswith("1.49"))' | sort -V | tail -n 1 # Verify if package-lock.json or pnpm-lock.yaml exists and check the resolved version find . -type f -name 'package-lock.json' -o -name 'pnpm-lock.yaml' | while read -r file; do echo "Checking $file for @playwright/test version..." case "$file" in *.json) jq -r '.. | .["@playwright/test"]? | select(. != null)' "$file" ;; *.yaml) grep -A 1 '@playwright/test' "$file" ;; esac doneLength of output: 214
Script:
#!/bin/bash # Look for catalog configuration files fd -t f -e json -e yaml -e yml catalog fd -t f -e json -e yaml -e yml version # Check root package.json for any catalog or version management configuration cat package.json || echo "No root package.json found" # Search for any files containing "catalog:" version reference rg -l "catalog:" -g "*.json" -g "*.yaml" -g "*.yml"Length of output: 22124
13-13: Verify Ubuntu 24.04 compatibility after upgradeThe consistent update to use catalog-based versioning for
@playwright/testacross all packages is a good approach. However, since the PR's main objective is to resolve Ubuntu 24.04 dependency issues, we should verify this specific compatibility.Let's verify the Ubuntu compatibility:
Also applies to: 23-23, 30-30, 33-33, 30-30
examples/sites/package.json (1)
59-59: LGTM! Consistent with the migration strategy.The update to use
catalog:for@playwright/testaligns with the PR objective and maintains consistency across the project.package.json (1)
213-213: Consider removing the explicit version overrideSince the project is using
catalog:references for@playwright/test, this explicit version override inpnpm.overridesmight be redundant. However, if the override is intentionally kept to ensure version consistency across all packages, please document this decision.Note: The update to version 1.49.1 addresses the libasound2 dependency issues with Ubuntu 24.04 as intended.
examples/docs/package.json (1)
23-23: Verify catalog resolution for @playwright/testThe change to use
"catalog:"instead of a fixed version aligns with the PR objectives. However, we should verify that this resolves to version 1.49.x as intended.Let's verify the resolved version and its dependencies:
✅ Verification successful
@playwright/test resolves to version 1.49.1
The "catalog:" syntax is correctly resolved through pnpm's override configuration, which explicitly sets @playwright/test to version 1.49.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the resolved version of @playwright/test and check for libasound2 dependency # Check the resolved version in package-lock.json or yarn.lock echo "Checking resolved version:" find . -name "package-lock.json" -o -name "yarn.lock" | xargs grep -A 5 "@playwright/test" # List direct dependencies of @playwright/test echo -e "\nChecking dependencies:" npm info @playwright/test@1.49 dependenciesLength of output: 354
Script:
#!/bin/bash # Search for workspace configuration files and catalog definitions echo "Checking for workspace configuration:" fd -e json "package.json" --exec grep -l "workspaces\|catalog" {} \; echo -e "\nChecking catalog definitions:" fd -e json "catalog.json" --exec cat {} \; echo -e "\nChecking root package.json:" cat package.json || trueLength of output: 22791
examples/vue2.7/package.json (2)
30-30: Consistent with other package.json filesThe update to
@playwright/testusing"catalog:"maintains consistency across the project.
31-33: Verify the scope of additional dependency changesSeveral new dependencies have been added that seem unrelated to the PR's objective of updating @playwright/test. Please clarify if these changes should be part of a separate PR:
- @unocss/* packages
- js-sha256
- onigasm
- unocss
Also applies to: 39-39, 41-41, 46-46
| }) | ||
| .getByRole('textbox') | ||
| ).toBeVisible() | ||
| await expect(page.locator('.tiny-grid-default-input')).toBeVisible() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and state assertions for better test stability.
The current visibility check might be flaky if multiple inputs exist or if the input's visibility is delayed. Consider:
- Adding a timeout for slow rendering
- Ensuring correct input state before proceeding
- await expect(page.locator('.tiny-grid-default-input')).toBeVisible()
+ await expect(page.locator('.tiny-grid-default-input'))
+ .toBeVisible({ timeout: 5000 })
+ await expect(page.locator('.tiny-grid-default-input'))
+ .toBeEditable()📝 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.
| await expect(page.locator('.tiny-grid-default-input')).toBeVisible() | |
| await expect(page.locator('.tiny-grid-default-input')) | |
| .toBeVisible({ timeout: 5000 }) | |
| await expect(page.locator('.tiny-grid-default-input')) | |
| .toBeEditable() |
| @@ -2,14 +2,10 @@ import { test, expect } from '@playwright/test' | |||
|
|
|||
| test('自定义组件与插槽编辑器校验', async ({ page }) => { | |||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | |||
| const demo = page.locator('#validation-custcomp') | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify correct row selection before input interaction.
The current implementation doesn't verify we're interacting with the correct row. Add assertions to ensure test reliability:
const demo = page.locator('#validation-custcomp')
await page.goto('grid-validation#validation-custcomp')
await page.getByText('GFD 科技 YX 公司').first().click()
+ // Ensure correct row is selected
+ await expect(page.locator('tr.row-selected'))
+ .toContainText('GFD 科技 YX 公司')
await demo.locator('.tiny-grid-default-input').fill('')Also applies to: 8-8
| @@ -2,9 +2,10 @@ import { test, expect } from '@playwright/test' | |||
|
|
|||
| test('数据关联校验', async ({ page }) => { | |||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | |||
| const demo = page.locator('#validation-row-data-valid') | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add initial state verification and input value assertion.
The test should verify the initial state and confirm the input value was actually set:
const demo = page.locator('#validation-row-data-valid')
await page.goto('grid-validation#validation-row-data-valid')
await page.getByText('70').first().click()
+ // Verify initial state
+ await expect(demo.locator('.tiny-grid-default-input'))
+ .toHaveValue('70')
await demo.locator('.tiny-grid-default-input').fill('700')
+ // Verify input value was set
+ await expect(demo.locator('.tiny-grid-default-input'))
+ .toHaveValue('700')Also applies to: 8-8
| }) | ||
| .getByRole('textbox') | ||
| ).toBeVisible() | ||
| await expect(page.locator('.tiny-grid-default-input')).toBeVisible() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test reliability with more specific selectors.
Using the same generic selector .tiny-grid-default-input for different inputs could lead to flaky tests. The test can't distinguish which input becomes visible after each action.
Consider using more specific selectors:
- await expect(page.locator('.tiny-grid-default-input')).toBeVisible()
+ await expect(page.locator('[data-testid="grid-input-company1"]')).toBeVisible()
- await expect(page.locator('.tiny-grid-default-input')).toBeVisible()
+ await expect(page.locator('[data-testid="grid-input-company2"]')).toBeVisible()Also applies to: 11-11
| await page.waitForTimeout(200) | ||
| await page | ||
| .getByRole('row', { | ||
| name: '1 华东区 福州 公司技术和研发实力雄厚，是国家 863 项目的参与者，并被政府认定为“高新技术企业”。' | ||
| }) | ||
| .locator('span') | ||
| .click() | ||
| await demo.locator('.tiny-grid-body__row .tiny-grid-checkbox__icon').first().click() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded timeout with proper wait conditions.
Using waitForTimeout can make tests flaky. Instead, wait for specific conditions or state changes.
Example:
await demo.locator('.tiny-grid-default-input').fill('')
- await page.waitForTimeout(200)
+ await expect(demo.locator('.tiny-grid-default-input')).toHaveValue('')Committable suggestion skipped: line range outside the PR's diff.
| }) | ||
| .getByRole('textbox') | ||
| .fill('') | ||
| await demo.locator('.tiny-input__inner').fill('') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded timeout with proper wait conditions.
Using waitForTimeout is an anti-pattern in Playwright as it makes tests flaky. Instead, wait for specific conditions:
await page.getByRole('button', { name: '保存' }).click()
- await page.waitForTimeout(200)
+ await expect(page.getByRole('cell', { name: 'WWW 科技 YX 公司' })).toBeVisible()Also applies to: 15-15
| }) | ||
| .getByRole('textbox') | ||
| .fill('sss') | ||
| await page.locator('.tiny-grid-default-input').fill('sss') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add wait conditions and distinguish between input fields.
The current implementation might be flaky because:
- It's not clear which
.tiny-grid-default-inputis being filled - There are no wait conditions between fill operations
- await page.locator('.tiny-grid-default-input').fill('sss')
+ await page.locator('.tiny-grid-default-input').first().waitFor()
+ await page.locator('.tiny-grid-default-input').first().fill('sss')
await page.getByText('RFV 有限责任公司').first().click()
- await page.locator('.tiny-grid-default-input').fill('eee')
+ await page.locator('.tiny-grid-default-input').nth(1).waitFor()
+ await page.locator('.tiny-grid-default-input').nth(1).fill('eee')Also applies to: 9-9
| @@ -2,14 +2,11 @@ import { test, expect } from '@playwright/test' | |||
|
|
|||
| test('提交前校验', async ({ page }) => { | |||
| page.on('pageerror', (exception) => expect(exception).toBeNull()) | |||
| const demo = page.locator('#validation-before-submit-validation') | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency in locator usage and add wait conditions.
The test mixes page and demo locators inconsistently. Also, missing wait conditions could make the test flaky.
const demo = page.locator('#validation-before-submit-validation')
await page.goto('grid-validation#validation-before-submit-validation')
+ await demo.waitFor()
await demo.getByText('GFD 科技 YX 公司').first().click()
+ await demo.locator('.tiny-grid-default-input').waitFor()
await demo.locator('.tiny-grid-default-input').fill('')
- await page.getByRole('button', { name: '提交数据' }).click()
+ await demo.getByRole('button', { name: '提交数据' }).click()Also applies to: 7-9
| "@docsearch/css": "^3.8.0", | ||
| "@docsearch/js": "^3.8.0", | ||
| "@docsearch/react": "npm:@docsearch/css", |
There was a problem hiding this comment.
Review @docsearch dependencies configuration
The dependency configuration for @docsearch/react pointing to @docsearch/css appears incorrect and might cause issues.
Consider updating to:
"@docsearch/css": "^3.8.0",
"@docsearch/js": "^3.8.0",
- "@docsearch/react": "npm:@docsearch/css",
+ "@docsearch/react": "^3.8.0",📝 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.
| "@docsearch/css": "^3.8.0", | |
| "@docsearch/js": "^3.8.0", | |
| "@docsearch/react": "npm:@docsearch/css", | |
| "@docsearch/css": "^3.8.0", | |
| "@docsearch/js": "^3.8.0", | |
| "@docsearch/react": "^3.8.0", |
gimmyhehe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
examples/sites/demos/pc/app/grid/editor/inner-editor.spec.js (1)
8-8: Consider adding a test data attribute for more reliable selection.While using class selectors is better than complex role-based chains, consider adding a dedicated test attribute (e.g.,
data-test-id="grid-editor-input") to make the selector even more resilient to UI changes.Example implementation:
- const input = demo.locator('.tiny-grid-body__row input').first() + const input = demo.locator('[data-test-id="grid-editor-input"]').first()examples/sites/demos/pc/app/grid/pager/show-save-msg.spec.js (2)
7-7: LGTM! Consider making the selector more specific.The simplified element selection using
.tiny-grid-default-inputis a good improvement over complex role-based queries. However, consider making it more specific to avoid potential conflicts with other similar elements on the page.- await page.locator('.tiny-grid-default-input').fill('GFD 科技 YX 公司 ds') + await page.locator('#pager-show-save-msg .tiny-grid-default-input').fill('GFD 科技 YX 公司 ds')
Line range hint
3-11: Enhance test reliability.A few suggestions to make the test more robust:
- Consider using data-testid attributes instead of text content for element selection
- The use of
first()could make the test fragile if the page structure changesExample improvement:
- await page.getByText('GFD 科技 YX 公司').first().click() + // Add a data-testid attribute to the target element + await page.locator('[data-testid="company-name-cell"]').click()examples/sites/demos/pc/app/grid/event/valid-error-event.spec.js (1)
7-7: Consider adding a more specific locator strategy.While using the class selector
.tiny-grid-default-inputworks, it might be fragile if multiple grid inputs exist on the page. Consider combining it with a more specific parent context or data attribute for better test stability.- await page.locator('.tiny-grid-default-input').clear() + await page.locator('tr:has-text("GFD 科技 YX 公司") .tiny-grid-default-input').clear()examples/sites/demos/pc/app/grid/toolbar/save-data.spec.js (1)
7-7: Consider using data-testid for more reliable element selection.While using class selectors (
.tiny-grid-default-input) is simpler than the previous role-based approach, it couples the test to implementation details that may change. Consider adding a dedicateddata-testidattribute for more stable test selectors.Example implementation:
- await page.locator('.tiny-grid-default-input').fill('dsfds') + await page.locator('[data-testid="grid-cell-input"]').fill('dsfds')This would require adding the corresponding attribute to your component:
<input class="tiny-grid-default-input" data-testid="grid-cell-input" />examples/sites/demos/pc/app/time-picker/format.spec.ts (4)
15-15: LGTM! Consider extracting the selector for reusability.The change to use
.tiny-time-panel[visible=true]is more reliable. However, since this selector is used multiple times, consider extracting it into a constant.const VISIBLE_TIME_PANEL = '.tiny-time-panel[visible=true]';
43-44: Make selectors more specific to avoid.first().Using
.first()can be fragile if multiple time panels are present. Consider making the selectors more specific by:
- Using a more targeted parent selector
- Adding data-testid attributes for better test maintainability
// Example with data-testid: const timePickerHour = page.locator('[data-testid="time-picker-hour-18"]') const timePickerMinute = page.locator('[data-testid="time-picker-minute-40"]')
Line range hint
3-3: Consider splitting the test into smaller, focused test cases.The current test covers multiple format scenarios. Consider breaking it down into separate test cases for better maintainability and clarity:
- Format display in input
- Value format
- Picker options format
test.describe('Time Picker Format', () => { test('should display time in specified format in input field', async ({ page }) => { // First scenario }); test('should use specified value format', async ({ page }) => { // Second scenario }); test('should display time in specified format in picker options', async ({ page }) => { // Third scenario }); });
Line range hint
4-4: Improve error handling and messaging.The current error handler is too broad. Consider:
- Being more specific about expected errors
- Adding custom error messages to assertions
page.on('pageerror', (error) => { console.error('Page error:', error); // Only fail on unexpected errors if (!error.message.includes('expected_error_pattern')) { expect(error).toBeNull(); } });examples/sites/demos/pc/app/grid/faq/grid-in-dialog-box.spec.js (1)
8-8: LGTM! Selector simplified for Playwright 1.49.x compatibility.The change to use
.tiny-grid-default-inputaligns with Playwright's updated string matching behavior and simplifies the element location strategy.Consider adding a comment explaining why this selector was chosen over role-based selectors, to help future maintainers:
+ // Using class selector for better compatibility with Playwright 1.49.x string matching await page.locator('.tiny-grid-default-input').fill('ss')examples/sites/demos/pc/app/grid/tree-table/tree-grid-insert-delete-update.spec.js (1)
27-29: Consider adding a test comment explaining the selector change.Since this change is part of adapting to Playwright 1.49.x's different string matching behavior, it would be helpful to add a comment explaining why this simpler selector is preferred over the previous role-based approach.
+ // Using class selector instead of role-based selector due to Playwright 1.49.x string matching changes await page.locator('.tiny-grid-default-input').fill('华南区 ee') await page.getByText('WWWW 科技 YX 公司').first().click() await page.getByRole('button', { name: '获取修改' }).click()examples/sites/demos/pc/app/grid/custom/column-visible-hidden.spec.js (1)
5-5: LGTM! Good use of scoped locators.The introduction of the
customlocator improves test reliability by scoping element queries to the grid component. This follows Playwright's best practices for component testing.Consider adding a test helper function to create the custom locator, as this pattern is repeated across multiple test files:
const getGridLocator = (page) => page.locator('.tiny-grid-custom');Also applies to: 8-9
examples/sites/demos/pc/app/grid/custom/column-sort.spec.js (1)
5-5: Consider replacing waitForTimeout with more reliable wait strategies.While the locator changes look good, using
waitForTimeoutcan make tests flaky. Consider using Playwright's built-in wait mechanisms likewaitForSelectororwaitForResponse.Replace:
await page.waitForTimeout(200);with:
await expect(sortBtn).toBeVisible();Also applies to: 8-8
examples/sites/demos/pc/app/grid/custom/column-fixed.spec.js (1)
5-5: Consider adding more comprehensive assertions for frozen columns.The locator changes look good, but the test could be more thorough in verifying the frozen column behavior.
Consider adding assertions for:
- Left position of the frozen column
- Presence of frozen column styling classes
- Scroll behavior verification
Also applies to: 8-9
examples/sites/demos/pc/app/grid/custom/multiple-column-sort.spec.js (1)
5-5: Reduce code duplication in click operations.While the locator changes are good, there's repetition in the click operations that could be simplified.
Consider extracting repeated operations into a helper function:
async function toggleColumnSort(custom, columnName) { await custom.getByRole('row', { name: columnName }) .getByTitle('未排序') .getByRole('img') .click(); }This would simplify the test code and make it more maintainable.
Also applies to: 8-10, 14-16
examples/sites/demos/pc/app/grid/custom/ordercolumn-local.spec.js (1)
8-9: Consider using consistent locator strategies throughout the test.While the changes to use the
customlocator are good, there's inconsistent usage of locators in the test. Some interactions still use direct page calls (e.g., lines 13, 14, 17) while others use the scopedcustomlocator.Consider updating all grid-related interactions to use the
customlocator for consistency:- await page.getByRole('button', { name: '确定' }).click() - await page.locator('.tiny-grid-custom__setting-btn').click() + await custom.getByRole('button', { name: '确定' }).click() + await custom.locator('.tiny-grid-custom__setting-btn').click()Also applies to: 15-15, 18-18
examples/sites/demos/pc/app/grid/custom/reset-resizable.spec.js (1)
33-34: Consider applying the custom locator pattern to both test cases.While the second test case benefits from the scoped
customlocator, the first test case still uses direct page calls. For consistency and maintainability, consider applying the same pattern to both test cases.Example refactor for the first test case:
test('重置列宽', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) + const custom = page.locator('.tiny-grid-custom') await page.goto('grid-custom#custom-reset-resizable') - const draggerDom = page.getByRole('cell', { name: '员工数' }).locator('.tiny-grid-resizable') - const thDom = page.getByRole('cell', { name: '员工数' }) + const draggerDom = custom.getByRole('cell', { name: '员工数' }).locator('.tiny-grid-resizable') + const thDom = custom.getByRole('cell', { name: '员工数' })Also applies to: 37-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
examples/sites/demos/pc/app/grid/custom/column-fixed.spec.js(1 hunks)examples/sites/demos/pc/app/grid/custom/column-sort.spec.js(1 hunks)examples/sites/demos/pc/app/grid/custom/column-visible-hidden.spec.js(1 hunks)examples/sites/demos/pc/app/grid/custom/multiple-column-sort.spec.js(1 hunks)examples/sites/demos/pc/app/grid/custom/ordercolumn-local.spec.js(1 hunks)examples/sites/demos/pc/app/grid/custom/reset-resizable.spec.js(1 hunks)examples/sites/demos/pc/app/grid/editor/inner-editor.spec.js(1 hunks)examples/sites/demos/pc/app/grid/event/valid-error-event.spec.js(1 hunks)examples/sites/demos/pc/app/grid/faq/grid-in-dialog-box.spec.js(1 hunks)examples/sites/demos/pc/app/grid/pager/show-save-msg.spec.js(1 hunks)examples/sites/demos/pc/app/grid/toolbar/save-data.spec.js(1 hunks)examples/sites/demos/pc/app/grid/tree-table/tree-grid-insert-delete-update.spec.js(1 hunks)examples/sites/demos/pc/app/time-picker/format.spec.ts(1 hunks)package.json(0 hunks)
💤 Files with no reviewable changes (1)
- package.json
🔇 Additional comments (6)
examples/sites/demos/pc/app/grid/editor/inner-editor.spec.js (1)
5-5: LGTM! Good use of ID-based selector.Using
page.locator('#editor-inner-editor')is a reliable approach for element selection, making the test more maintainable.examples/sites/demos/pc/app/grid/toolbar/save-data.spec.js (1)
Line range hint
1-12: Verify test stability with Playwright 1.49.x.Since this PR upgrades Playwright, please ensure this test remains stable, particularly around input field interactions which were noted to behave differently in the new version.
Run this script to check for similar input field selectors that might need updates:
✅ Verification successful
Test uses stable selectors, no updates needed
The test uses a consistent
.tiny-grid-default-inputselector that's reliably used across the entire test suite for input field interactions. This class-based approach is more stable than role-based selectors and should continue working well with Playwright 1.49.x.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential input field selectors that might need updating # Look for role-based input selections that might need to be simplified ast-grep --pattern 'getByRole($_, { name: $_ }).fill($_)' # Also check for direct class-based input selections rg "tiny-grid-default-input"Length of output: 2954
examples/sites/demos/pc/app/grid/faq/grid-in-dialog-box.spec.js (1)
8-11: Verify selector uniqueness and stability.Let's ensure the
.tiny-grid-default-inputselector reliably targets the correct input field and isn't affected by multiple matches.✅ Verification successful
The
.tiny-grid-default-inputselector is properly scoped and stable.The selector shows consistent usage across test files and is properly isolated within each test scenario. The framework-level selector is reliably targeting the intended input field without ambiguity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for multiple occurrences of the class in the codebase # and verify its usage in other test files # Search for the class definition to ensure it's a stable framework class rg -l "tiny-grid-default-input" "examples/sites/demos/pc/" --type css # Check other test files using this selector to confirm consistent usage rg -l "tiny-grid-default-input" "examples/sites/demos/pc/" --type jsLength of output: 1398
Script:
#!/bin/bash # Check the actual usage context in a few test files # Looking for potential multiple instances in the same test # Check the file under review rg "tiny-grid-default-input" "examples/sites/demos/pc/app/grid/faq/grid-in-dialog-box.spec.js" -B 2 -A 2 # Check a few other files to compare usage patterns rg "tiny-grid-default-input" "examples/sites/demos/pc/app/grid/edit/editing.spec.js" -B 2 -A 2 rg "tiny-grid-default-input" "examples/sites/demos/pc/app/grid/validation/editing-validation.spec.js" -B 2 -A 2Length of output: 1081
examples/sites/demos/pc/app/grid/tree-table/tree-grid-insert-delete-update.spec.js (1)
27-27: Verify the uniqueness and reliability of the simplified selector.The change from role-based selection to
.tiny-grid-default-inputmakes the test less specific. While this aligns with the needed updates for Playwright 1.49.x, we should ensure this selector uniquely identifies the correct input field.Let's verify the selector's uniqueness in the test:
examples/sites/demos/pc/app/grid/custom/ordercolumn-local.spec.js (1)
5-5: LGTM! Good practice introducing a scoped locator.The introduction of a scoped locator for
.tiny-grid-customimproves test maintainability and readability.examples/sites/demos/pc/app/grid/custom/reset-resizable.spec.js (1)
Line range hint
1-45: Verify test stability with mouse interactions in Playwright 1.49.xSince this is a major version upgrade of Playwright and the test involves precise mouse interactions for drag operations, we should verify the stability of these operations.
Let's analyze the codebase for similar mouse interaction patterns that might need attention:
✅ Verification successful
Consider using
stepsparameter for smoother mouse movementsThe test follows the same stable pattern used across the codebase. For enhanced reliability, consider using the
stepsparameter in mouse movements as demonstrated ingrid-resizable-config.spec.ts:await page.mouse.move(startX + distance, startY, { steps: 10 })🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for mouse interaction patterns in test files echo "Searching for mouse interaction patterns..." rg -l "mouse\.(move|down|up)" "examples/sites/demos/pc/app/grid" echo "Searching for drag operation patterns..." rg -l "drag|resizable|拖拽" "examples/sites/demos/pc/app/grid"Length of output: 5171
Script:
#!/bin/bash # Search for mouse interaction patterns with timeouts echo "Searching for mouse interaction patterns with timeouts..." rg -A 3 "mouse\.(move|down|up)" "examples/sites/demos/pc/app/grid" --type js --type ts echo "Searching for stability-related comments..." rg -i "(flaky|unstable|stability|reliable)" "examples/sites/demos/pc/app/grid" --type js --type tsLength of output: 11573
| await page.waitForTimeout(400) | ||
| await expect(page.getByText('触发校验不通过事件').first()).toBeVisible() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed timeout with waitForSelector.
Using waitForTimeout is discouraged in Playwright as it makes tests flaky. Instead, wait for the validation message to appear using waitForSelector.
- await page.waitForTimeout(400)
- await expect(page.getByText('触发校验不通过事件').first()).toBeVisible()
+ await expect(page.getByText('触发校验不通过事件').first()).toBeVisible({ timeout: 5000 })📝 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.
| await page.waitForTimeout(400) | |
| await expect(page.getByText('触发校验不通过事件').first()).toBeVisible() | |
| await expect(page.getByText('触发校验不通过事件').first()).toBeVisible({ timeout: 5000 }) |
| await page.getByRole('button', { name: '确定' }).click() | ||
| await page.waitForTimeout(100) | ||
| await expect(page.getByRole('textbox', { name: '20:40:00 pm' })).toBeVisible() | ||
|
|
||
| await page.getByRole('textbox', { name: '8:40:0 PM' }).click() | ||
| await page.waitForTimeout(100) | ||
| await page.getByRole('listitem').filter({ hasText: '09 PM' }).locator('span').click() | ||
| await page.locator('.tiny-time-panel[visible=true]').getByText('09 PM').click() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed timeouts with proper wait conditions.
Using fixed timeouts (waitForTimeout) can make tests flaky. Consider using Playwright's built-in wait conditions instead.
// Instead of:
await page.waitForTimeout(100)
// Use:
await page.locator('.tiny-time-panel[visible=true]').waitFor({ state: 'visible' })Also applies to: 29-29, 37-37
* test: update @playwright/test to 1.49.x * test(grid): fix grid e2e test case * test(grid): fix grid e2e test case
* test: update @playwright/test to 1.49.x * test(grid): fix grid e2e test case * test(grid): fix grid e2e test case
PR
升级@playwright/test到1.49.x版本
旧版本的playwright依赖于ibasound2，这个包在大于ubantu24.04版本的环境下会报错。
cypress-io/cypress-documentation#5816
新版本playwright已升级相关依赖，因此需要升级playwright以修复此问题
修复升级后的e2e测试用例报错
测试用例报错原因：新版本playwright会匹配输入框中的字符串，而老版本的不会
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
Dependency Updates
@playwright/testto use catalog-based versioning across multiple projectsPackage Configuration
package.jsonfilesTesting Improvements