Bandaid: skip e2e modal screenshots until fix #4825
Conversation
WalkthroughThe changes in the pull request involve commenting out various screenshot validation assertions across multiple test files related to a 3D rendering application. This action addresses a known issue where the screenshot comparisons are "off by one pixel," which has caused test failures. The modifications serve as a temporary workaround to allow the test suites to run without interruption while highlighting the need for future fixes to the screenshot comparison logic. Changes
Possibly related PRs
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
Documentation and Community
|
d75b6ed to
99c79ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- 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 (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e-pw/src/oss/specs/3d/pcd-only-dataset.spec.ts
Additional context used
Path-based instructions (5)
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/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 (5)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts (4)
111-115: Temporary workaround: Address the TODO comment in the future.Commenting out the screenshot assertion is a temporary workaround to allow the test suite to run without interruption. However, it means that the visual correctness of the rendered scene is not being validated.
Please ensure that the TODO comment is addressed in the future to fix the screenshot comparison logic and re-enable the assertion.
123-130: Temporary workaround: Address the TODO comment in the future.Commenting out the screenshot assertion is a temporary workaround to allow the test suite to run without interruption. However, it means that the visual correctness of the rendered scene with minimum line widths is not being validated.
Please ensure that the TODO comment is addressed in the future to fix the screenshot comparison logic and re-enable the assertion.
134-141: Temporary workaround: Address the TODO comment in the future.Commenting out the screenshot assertion is a temporary workaround to allow the test suite to run without interruption. However, it means that the visual correctness of the rendered scene with maximum line widths is not being validated.
Please ensure that the TODO comment is addressed in the future to fix the screenshot comparison logic and re-enable the assertion.
146-150: Temporary workaround: Address the TODO comment in the future.Commenting out the screenshot assertion is a temporary workaround to allow the test suite to run without interruption. However, it means that the visual correctness of the rendered scene for the next sample is not being validated.
Please ensure that the TODO comment is addressed in the future to fix the screenshot comparison logic and re-enable the assertion.
e2e-pw/src/oss/specs/groups/ima-vid.spec.ts (1)
152-156: Temporary workaround: Commented out another screenshot verification assertion.Similar to the previous code segment, commenting out this screenshot verification assertion is a temporary workaround for the known issue where the modal screenshot comparison is off by one pixel. This allows the test to pass for now.
Please refer to the previous review comment about creating a GitHub issue to track the fix for the underlying screenshot comparison issue. Once the issue is fixed, both of these assertions should be re-enabled.
| test.skip( | ||
| null, | ||
| "TODO: FIX ME. MODAL SCREENSHOT COMPARISON IS OFF BY ONE-PIXEL" | ||
| ); |
There was a problem hiding this comment.
Skipped test case needs to be addressed in the future.
Skipping the test case temporarily unblocks the test suite, but it's important to fix the underlying issue with the modal screenshot comparison. The skipped test case should be re-enabled once the root cause is resolved to ensure the correctness of the functionality.
Consider creating a GitHub issue to track the resolution of the skipped test case. This will help ensure that the issue is not forgotten and is addressed in a timely manner.
Do you want me to open a GitHub issue to track this task?
| // TODO: FIX ME. MODAL SCREENSHOT COMPARISON IS OFF BY ONE-PIXEL | ||
| // await expect(modal.looker).toHaveScreenshot("labels.png"); |
There was a problem hiding this comment.
Temporary workaround: Disabled screenshot comparison
Commenting out the screenshot comparison assertion is a temporary measure to allow the test suite to run without interruption. However, it's crucial to investigate and resolve the underlying pixel mismatch issue to restore the visual validation of the modal component.
Please create a separate issue to track the resolution of the off-by-one-pixel discrepancy in the screenshot comparison logic. This will ensure that the problem is properly addressed and the visual fidelity of the modal is maintained in the long run.
| // TODO: FIX ME. MODAL SCREENSHOT COMPARISON IS OFF BY ONE-PIXEL | ||
| // check screenshot before video is played | ||
| await expect(modal.looker).toHaveScreenshot(`${slice}-before-play.png`, { | ||
| animations: "allow", | ||
| }); | ||
| // await expect(modal.looker).toHaveScreenshot(`${slice}-before-play.png`, { | ||
| // animations: "allow", | ||
| // }); |
There was a problem hiding this comment.
Temporary workaround: Disabled modal screenshot comparison
The code segment is commented out as a temporary workaround for the known issue where the modal screenshot comparison is off by one pixel. While this allows the test suite to run without interruption, it's important to prioritize fixing the underlying issue to enable proper visual validation.
In the meantime, consider focusing the test on verifying that the correct label is displayed rather than the exact visual fidelity of the screenshots.
| // TODO: FIX ME. MODAL SCREENSHOT COMPARISON IS OFF BY ONE-PIXEL | ||
| // check screenshot after video is played | ||
| await expect(modal.looker).toHaveScreenshot(`${slice}-after-play.png`, { | ||
| // masking time / frame because it might be off by a couple of seconds and we want to avoid flakiness | ||
| // the real test is that the correct label is shown | ||
| mask: [modal.video.time], | ||
| animations: "allow", | ||
| }); | ||
| // await expect(modal.looker).toHaveScreenshot(`${slice}-after-play.png`, { | ||
| // // masking time / frame because it might be off by a couple of seconds and we want to avoid flakiness | ||
| // // the real test is that the correct label is shown | ||
| // mask: [modal.video.time], | ||
| // animations: "allow", | ||
| // }); |
There was a problem hiding this comment.
Temporary workaround: Disabled modal screenshot comparison
Similar to the previous code segment, this assertion is commented out as a temporary workaround for the known issue where the modal screenshot comparison is off by one pixel. While this allows the test suite to run without interruption, it's important to prioritize fixing the underlying issue to enable proper visual validation.
In the meantime, consider focusing the test on verifying that the correct label is displayed rather than the exact visual fidelity of the screenshots.
Good practice: Masking time/frame to avoid flakiness
The practice of masking the time/frame to avoid flakiness in the test is a good one. This ensures that the test focuses on the essential aspects of the functionality rather than being sensitive to minor timing variations.
|
Subsumed by #4808 |
There's a known issue of screenshot comparison being off by 1 pixel: See microsoft/playwright#20015
Summary by CodeRabbit
These changes allow the testing suite to continue functioning without interruptions while the visual fidelity issues are being resolved.