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

test(e2e): [dialog-box,bulletin-board,cascader-panel,company,config-provider,country,fullscreen] update e2e test #2329

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

zzcr
Copy link
Member

@zzcr zzcr commented Oct 21, 2024

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

  • Tests
    • Updated the test case for the dialog box demo to improve element targeting and interaction methods, ensuring more precise testing of buttons and labels.
    • Adjusted expected CSS properties for various components, enhancing visual feedback during tests.
    • Removed unnecessary checks for clearing input fields in dropdown selections, streamlining the test flow.
    • Modified fullscreen image display tests to reflect changes in visibility expectations after requesting fullscreen.
    • Removed interactions related to locale selection in the custom service tests, simplifying the test structure.

Copy link

coderabbitai bot commented Oct 21, 2024

Walkthrough

The changes in this pull request involve modifications to multiple test cases across various files. Key updates include refining element access in the destroy-on-close.spec.ts file using a demo locator for more specific interactions. Other test cases have adjustments to expected CSS properties, such as border and background colors, and some tests have had sections removed that previously verified the clearing of selections in dropdowns. The overall structure of the tests remains intact, focusing on ensuring accurate visual and functional validation.

Changes

File Path Change Summary
examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts Updated element access in tests to use demo locator for scoped interactions with dialog box elements.
examples/sites/demos/pc/app/base-select/automatic-dropdown.spec.ts Changed expected border color in one test case; no changes in the second test case.
examples/sites/demos/pc/app/bulletin-board/active-name.spec.ts Updated expected CSS color for the activated second tab title.
examples/sites/demos/pc/app/bulletin-board/base.spec.ts Updated expected CSS color values for tab titles in multiple locations.
examples/sites/demos/pc/app/cascader-panel/basic-usage.spec.ts Changed expected background color for a menu item.
examples/sites/demos/pc/app/company/custom-service.spec.ts Removed code that cleared input field after dropdown selection.
examples/sites/demos/pc/app/config-provider/tag.spec.ts Updated expected CSS padding value for an element.
examples/sites/demos/pc/app/country/custom-service.spec.js Removed code that cleared selection in a dropdown component.
examples/sites/demos/pc/app/fullscreen/example-api.spec.ts Adjusted expectations after fullscreen request; removed exit fullscreen test code.
examples/sites/demos/pc/app/locales/custom-service.spec.ts Removed click action for selecting 'enUS' locale.

Possibly related PRs

🐰 In the demo where the dialog does play,
Buttons and labels now find their way.
Scoped to the section, they dance and they twirl,
Testing with care, in a rabbit's soft whirl.
Hopping through code, with joy we embrace,
Each change a delight in this testing space! 🐇


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 000f2b8 and 54afc86.

📒 Files selected for processing (1)
  • examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts (2)

8-15: Great refactoring of element selections!

The consistent use of the demo locator across all interactions improves the test's reliability and maintainability. The new method for selecting radio buttons using locator('label').filter() is more precise and robust.

For consistency, consider refactoring the radio button selections into a reusable function:

const selectRadioButton = (text: string) => demo.locator('label').filter({ hasText: text }).click();

// Usage:
await selectRadioButton('关闭时不销毁');
await selectRadioButton('关闭时销毁');

This would further improve readability and maintainability of the test.


Line range hint 1-17: Consider enhancing test coverage

While the refactoring improves the test's robustness, consider the following suggestions to enhance test coverage:

  1. Add assertions to verify the dialog box's visibility after opening it.
  2. Test the "关闭时不销毁" (Don't destroy on close) functionality by checking if the dialog box still exists in the DOM after closing.
  3. Add negative test cases, such as trying to interact with the dialog box after it has been destroyed.

These additions would provide more comprehensive coverage of the dialog box's behavior.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 65d3aeb and c1a4847.

📒 Files selected for processing (1)
  • examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts (1)

7-7: Excellent use of a specific locator!

Introducing the demo locator improves the test's robustness by scoping interactions to the specific #destroy-on-close section. This change enhances maintainability and reduces the risk of unintended interactions with elements outside the target demo area.

@zzcr zzcr changed the title test(e2e): [dialog-box] update dialogbox e2e test test(e2e): [dialog-box,bulletin-board,cascader-panel,company,config-provider,country,fullscreen] update dialogbox e2e test Oct 21, 2024
@zzcr zzcr changed the title test(e2e): [dialog-box,bulletin-board,cascader-panel,company,config-provider,country,fullscreen] update dialogbox e2e test test(e2e): [dialog-box,bulletin-board,cascader-panel,company,config-provider,country,fullscreen] update e2e test Oct 21, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
examples/sites/demos/pc/app/bulletin-board/base.spec.ts (1)

Line range hint 1-31: Summary: Consistent color updates, consider UX implications.

The changes in this file consistently update the expected color for tab titles across different states (default, hover, and active) from blue (rgb(94, 124, 224)) to near-black (rgb(25, 25, 25)). While the implementation in the test is correct, these changes may impact the user experience by reducing visual distinction between different tab states.

Consider the following recommendations:

  1. Review the overall design of the bulletin board component to ensure it provides clear visual feedback for user interactions.
  2. If the intention is to have distinct visual states, update the test and implementation to reflect different colors or styles for default, hover, and active states.
  3. If the current design is intentional, consider adding comments in the test file explaining the rationale behind using the same color for all states.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c1a4847 and 000f2b8.

📒 Files selected for processing (10)
  • examples/sites/demos/pc/app/base-select/automatic-dropdown.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/bulletin-board/active-name.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/bulletin-board/base.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/cascader-panel/basic-usage.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/company/custom-service.spec.ts (0 hunks)
  • examples/sites/demos/pc/app/config-provider/tag.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/country/custom-service.spec.js (0 hunks)
  • examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/fullscreen/example-api.spec.ts (1 hunks)
  • examples/sites/demos/pc/app/locales/custom-service.spec.ts (0 hunks)
💤 Files with no reviewable changes (3)
  • examples/sites/demos/pc/app/company/custom-service.spec.ts
  • examples/sites/demos/pc/app/country/custom-service.spec.js
  • examples/sites/demos/pc/app/locales/custom-service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/sites/demos/pc/app/dialog-box/destroy-on-close.spec.ts
🧰 Additional context used
🔇 Additional comments (11)
examples/sites/demos/pc/app/config-provider/tag.spec.ts (1)

12-12: Verify the CSS padding change and its implications.

The expected CSS padding value for the second element has been changed from '16px' to '12px'. This change might be intentional due to a styling update in the component, but it's important to verify this change.

Please run the following script to check if this padding change is consistent with other related components or if it's an isolated change:

Additionally, could you confirm if this change is related to any recent updates in the component's styling or design specifications?

✅ Verification successful

CSS padding change in tag.spec.ts is consistent with existing tests.

The update from '16px' to '12px' aligns with component-specific padding configurations observed in other test files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of CSS padding expectations in test files

# Test: Search for CSS padding expectations in test files
rg -g '*.spec.ts' 'toHaveCSS\(.(padding|padding-\w+).' examples/sites/demos/pc/app/

Length of output: 926

examples/sites/demos/pc/app/cascader-panel/basic-usage.spec.ts (1)

11-11: Confirm the new background color for the active menu item.

The expected background color for the active menu item has been updated from a blue shade to a very light gray. This change aligns with similar updates in other components mentioned in the PR.

Please confirm that this new color (rgb(245, 245, 245)) is the correct and intended background color for an active menu item in the cascader panel. You may want to cross-check this with your design system or UI guidelines to ensure consistency across components.

examples/sites/demos/pc/app/bulletin-board/active-name.spec.ts (1)

13-13: Approve change, but clarification needed on PR scope.

The update to the expected color (from 'rgb(94, 124, 224)' to 'rgb(25, 25, 25)') aligns with similar changes mentioned in the AI-generated summary for other files. This consistency suggests an intentional UI update.

However, there's a discrepancy between the PR title, which mentions updating the dialog-box e2e test, and this file, which is for the bulletin-board component.

Could you please clarify the scope of this PR? Are changes to the bulletin-board component intended to be part of this PR?

If so, I recommend updating the PR description to accurately reflect all affected components. This will help reviewers understand the full extent of the changes and ensure nothing is overlooked during the review process.

To verify the extent of changes, let's run the following script:

This will help us understand if changes to both components are indeed part of this PR.

examples/sites/demos/pc/app/bulletin-board/base.spec.ts (3)

14-14: LGTM. Verify design specifications.

The color change for the active tab title from blue (rgb(94, 124, 224)) to near-black (rgb(25, 25, 25)) has been correctly implemented in the test. This aligns with the changes observed in other related components.

Please confirm that this color change matches the latest design specifications for the bulletin board component.


21-21: Consider hover state distinction.

The color change for tab titles on hover has been correctly implemented in the test, matching the active tab title color. However, using the same color (rgb(25, 25, 25)) for both default and hover states might reduce visual feedback for user interaction.

Please confirm if the design intentionally removes the hover effect. If not, consider implementing a distinct hover state color to improve user experience.


27-27: LGTM. Review overall component visual states.

The color change for clicked tab titles has been correctly implemented in the test, consistent with the changes for default and hover states. All states now use the same color (rgb(25, 25, 25)).

Given that all visual states (default, hover, and active) now use the same color, please conduct a comprehensive review of the bulletin board component's visual feedback mechanisms. Ensure that users can easily distinguish between different tab states for optimal usability.

examples/sites/demos/pc/app/base-select/automatic-dropdown.spec.ts (3)

12-12: Approved: Border color expectation updated.

The change in the expected border color from rgb(94, 124, 224) to rgb(25, 25, 25) has been noted and is consistent with similar updates in other files.

Please confirm with the design team that rgb(25, 25, 25) (a very dark gray) is the intended color for the focus state of the input element.

Consider updating the test description to include the expected visual feedback:

test('不可搜索时,获取焦点不下拉且边框变为深灰色', async ({ page }) => {
  // ... (rest of the test case)
})

Line range hint 18-32: Confirmed: No changes in the searchable dropdown test.

This test case for the searchable dropdown functionality remains unchanged. It continues to verify that the dropdown opens on focus and allows item selection.

Please confirm that this test case still aligns with the current expected behavior and styling of the searchable dropdown, especially considering the changes made to the non-searchable dropdown test case.


Line range hint 1-32: Summary: Minor update to non-searchable dropdown test, searchable dropdown test unchanged.

This file update primarily involves a change in the expected border color for the non-searchable dropdown's focus state. The searchable dropdown test remains unmodified. These changes appear to be part of a broader styling update across the component library.

To ensure consistency across the component library:

  1. Verify that all relevant dropdown components have been updated to use the new focus state color (rgb(25, 25, 25)).
  2. Confirm that the lack of changes in the searchable dropdown test doesn't indicate missed updates.

Run the following script to check for consistency:

This will help ensure that all necessary updates have been made consistently across the codebase.

✅ Verification successful

Verification Successful: Border-color updates are consistent across all dropdown tests.

All searchable dropdown tests correctly include the updated border-color expectation (rgb(25, 25, 25)). No discrepancies found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistency in border color updates across dropdown components

# Search for border-color expectations in test files
echo "Searching for border-color expectations in test files:"
rg -n 'toHaveCSS\(.border-color.' examples/sites/demos/pc/app

# Search for potential missed updates in searchable dropdown tests
echo "Searching for potential missed updates in searchable dropdown tests:"
rg -n 'test\(.可搜索时' examples/sites/demos/pc/app

Length of output: 2275


Script:

#!/bin/bash
# Description: Verify border-color expectations in searchable dropdown tests

echo "Checking border-color expectations in searchable dropdown tests:"

# Find all searchable dropdown test files
searchable_tests=$(rg -l 'test\(.可搜索时' examples/sites/demos/pc/app)

# Check border-color expectations in each searchable dropdown test
for file in $searchable_tests; do
  echo "Inspecting $file:"
  rg -n 'toHaveCSS\(.border-color' "$file" || echo "No border-color expectations found in $file"
done

Length of output: 1066

examples/sites/demos/pc/app/fullscreen/example-api.spec.ts (2)

22-23: Approved: Corrected fullscreen image visibility expectations.

The change in expectations for image visibility after entering fullscreen mode appears to be correct. It's more logical for the larger image (bigImg) to be visible and the smaller image (smallImg) to be hidden when in fullscreen mode.


22-23: Verify: Removed test for "Exit Fullscreen" functionality.

The test case for exiting fullscreen mode seems to have been removed. This is a significant change that leaves an important part of the functionality untested.

Could you please explain why the "Exit Fullscreen" test was removed? If there are technical limitations preventing this test, consider adding a comment explaining the situation or create a separate issue to track this gap in test coverage.

Comment on lines 28 to 29
})

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address TODO comments regarding fullscreen issues.

There are two TODO comments at the end of the file indicating known issues with exiting fullscreen mode and element retrieval in fullscreen. These comments suggest that there are unresolved problems with the fullscreen functionality.

Please consider addressing these TODO items:

  1. Investigate and fix the issue with exiting fullscreen mode using the Esc key.
  2. Resolve the problem of not being able to retrieve elements in fullscreen mode.

Would you like assistance in creating GitHub issues to track these problems?

@kagol kagol merged commit 5661ab5 into dev Oct 21, 2024
6 checks passed
@kagol kagol deleted the fix-dialog-box-e2e-error-1021 branch November 4, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants