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

Update e2e screenshots #4808

Merged
merged 16 commits into from
Sep 19, 2024
Merged

Update e2e screenshots #4808

merged 16 commits into from
Sep 19, 2024

Conversation

sashankaryal
Copy link
Contributor

@sashankaryal sashankaryal commented Sep 17, 2024

Summary by CodeRabbit

  • New Features
    • Updated the Docker image for generating screenshots, potentially enhancing functionality and performance with the latest Playwright version.
  • Bug Fixes
    • Temporarily disabled screenshot assertions in various test cases due to a known issue with pixel discrepancies, ensuring test suite stability while addressing visual validation concerns.

@sashankaryal sashankaryal self-assigned this Sep 17, 2024
Copy link
Contributor

coderabbitai bot commented Sep 17, 2024

Walkthrough

The pull request updates the Dockerfile for generating screenshots with Playwright by changing the base image from version v1.46.0-jammy to v1.47.1. Additionally, it modifies several test files by commenting out screenshot assertions due to a known issue where the comparisons are "off by one pixel." This adjustment aims to prevent false negatives in tests while maintaining the overall test structure.

Changes

Files Change Summary
e2e-pw/scripts/generate-screenshots-docker-image/Dockerfile Updated base image from v1.46.0-jammy to v1.47.1.
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts Commented out assertions for modal screenshot comparisons due to a one-pixel discrepancy.
e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts Commented out assertions related to modal screenshot comparisons due to a one-pixel discrepancy.
e2e-pw/src/oss/specs/groups/ima-vid.spec.ts Commented out assertions for modal screenshot comparisons due to a one-pixel discrepancy.
e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts Commented out assertions for modal screenshot comparisons due to a one-pixel discrepancy.
e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts Added a skipped test case for modal screenshot comparison due to a one-pixel discrepancy.
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts Commented out assertions for modal screenshot comparisons due to a one-pixel discrepancy.

Possibly related PRs

  • fix tagger e2e tests #4783: Modifications to the tagger.spec.ts test suite, which is directly related to the screenshot assertions commented out in the main PR, indicating a connection in the testing of modal components.
  • skip e2e tests that use zoo datasets #4804: This PR modifies the tagger.spec.ts file by skipping tests that rely on a dataset, aligning with the main PR's focus on addressing issues with screenshot comparisons in modal tests.

Poem

In a world of code, where rabbits play,
A Docker image hops, brightening the day.
From v1.46 to v1.47, it leaps,
Enhancing our scripts, as the framework keeps.
With every change, we cheer and rejoice,
For in the land of code, we find our voice! 🐇✨


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.

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.

@sashankaryal sashankaryal requested a review from a team September 17, 2024 16:21
Copy link
Contributor

@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

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0b5ea33 and c3d6ea0.

Files selected for processing (2)
  • .github/workflows/e2e.yml (1 hunks)
  • e2e-pw/scripts/generate-screenshots-docker-image/Dockerfile (1 hunks)
Files skipped from review due to trivial changes (1)
  • e2e-pw/scripts/generate-screenshots-docker-image/Dockerfile
Additional context used
actionlint
.github/workflows/e2e.yml

8-8: label "ubuntu-22.04-m" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

Copy link
Contributor

@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: 5

Outside diff range and nitpick comments (3)
e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts (1)

78-81: Skipping the test case is a good temporary solution, but ensure the issue is tracked and resolved.

Skipping the test case using test.skip allows the test suite to pass without failing due to the known issue with modal screenshot comparison. This is a proactive approach to manage test coverage while acknowledging the existing bug.

However, to ensure the issue is not forgotten and is resolved in a timely manner, consider the following suggestions:

  • Create a GitHub issue to track the resolution of the known issue. Reference the issue number in the TODO comment.
  • Add more details to the TODO comment to provide better context for future reference. Include information such as the specific scenario causing the one-pixel difference, steps to reproduce the issue, and any other relevant details.
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (2)

111-113: Temporarily disabling the assertion is a good approach to avoid false negatives.

The comment indicates that the screenshot comparison is currently inaccurate due to a one-pixel discrepancy. It's a good practice to temporarily disable the assertion to avoid false negatives during test execution.

Consider creating a GitHub issue to track the resolution of the one-pixel discrepancy. This will ensure that the issue is not forgotten and can be addressed in a future update. Do you want me to open a new GitHub issue for this?


130-131: Investigate the root cause of the visual discrepancies.

Similar to the previous comment, this assertion has been temporarily disabled due to a one-pixel discrepancy in the screenshot comparison. While disabling the assertion is a good temporary solution, it's important to investigate the root cause of these visual discrepancies to prevent similar issues from occurring in the future.

Consider the following suggestions to improve the visual tests:

  1. Review the screenshot generation process to ensure consistency across different environments and platforms.
  2. Verify that the visual components are properly styled and positioned to minimize the chances of pixel-level differences.
  3. Consider using a more tolerant comparison method that allows for slight variations in pixel values.

By addressing the underlying causes of the visual discrepancies, you can improve the reliability and maintainability of the visual tests in the long run.

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 28681a8 and 6c9cb14.

Files ignored due to path filters (34)
  • e2e-pw/package.json is excluded by !**/*.json
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/max-line-width-scene-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/max-line-width-scene-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/min-line-width-scene-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/min-line-width-scene-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-2-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-2-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-1-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-1-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-1-left-pan-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-1-left-pan-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-2-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-2-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-2-right-pan-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts-snapshots/orthographic-projection-modal-cuboid-2-right-pan-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/groups/ima-vid.spec.ts-snapshots/ima-vid-1-3-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/groups/ima-vid.spec.ts-snapshots/ima-vid-1-3-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/groups/ima-vid.spec.ts-snapshots/ima-vid-1-5-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/groups/ima-vid.spec.ts-snapshots/ima-vid-1-5-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v1-after-play-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v1-after-play-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v1-before-play-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v1-before-play-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v2-after-play-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v2-after-play-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v2-before-play-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts-snapshots/v2-before-play-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts-snapshots/modal-media-field-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts-snapshots/modal-media-field-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-darwin.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-linux.png is excluded by !**/*.png, !**/*.png
  • e2e-pw/yarn.lock is excluded by !**/yarn.lock, !**/*.lock, !**/*.lock
Files selected for processing (7)
  • e2e-pw/scripts/generate-screenshots-docker-image/Dockerfile (1 hunks)
  • e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (1 hunks)
  • e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts (1 hunks)
  • e2e-pw/src/oss/specs/groups/ima-vid.spec.ts (2 hunks)
  • e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts (1 hunks)
  • e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts (1 hunks)
  • e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • e2e-pw/scripts/generate-screenshots-docker-image/Dockerfile
Additional context used
Path-based instructions (6)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

e2e-pw/src/oss/specs/groups/ima-vid.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

e2e-pw/src/oss/specs/regression-tests/group-video/group-video-label.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

e2e-pw/src/oss/specs/regression-tests/media-field.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (1)

Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.

Additional comments not posted (4)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (4)

111-115: Assertion temporarily disabled with TODO comment.

The assertion for comparing the modal container screenshot is temporarily commented out. The accompanying TODO comment indicates that the screenshot comparison is currently off by one pixel, which needs to be addressed in the future.


123-130: Assertion temporarily disabled with TODO comment.

The assertion for comparing the modal container screenshot with minimum line width settings is temporarily commented out. The accompanying TODO comment indicates that the screenshot comparison is currently off by one pixel, which needs to be addressed in the future.


134-141: Assertion temporarily disabled with TODO comment.

The assertion for comparing the modal container screenshot with maximum line width settings is temporarily commented out. The accompanying TODO comment indicates that the screenshot comparison is currently off by one pixel, which needs to be addressed in the future.


146-150: Assertion temporarily disabled with TODO comment.

The assertion for comparing the modal container screenshot after navigating to the next sample is temporarily commented out. The accompanying TODO comment indicates that the screenshot comparison is currently off by one pixel, which needs to be addressed in the future.

@sashankaryal sashankaryal merged commit 64136c5 into develop Sep 19, 2024
12 checks passed
@sashankaryal sashankaryal deleted the fix/e2e-screenshots branch September 19, 2024 22:36
@coderabbitai coderabbitai bot mentioned this pull request Nov 15, 2024
7 tasks
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