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: update app-layout visual tests to use fixed height #8385

Closed
wants to merge 1 commit into from

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Dec 20, 2024

Description

Tweaked vaadin-app-layout visual tests to use fixed 600px height to avoid errors when bumping Chrome version:

 ❌ app-layout > ltr > basic
      Error: Screenshot is not the same width and height as the baseline. Baseline: { width: 1024, height: 651 } Screenshot: { width: 1024, height: 650 }
        at async n.<anonymous> (packages/app-layout/test/visual/material/app-layout.test.js:37:9)

Note: in some version the height was decreased to 627px so let's use lower value to be on the safe side.

Type of change

  • Test

@web-padawan web-padawan requested a review from vursen December 20, 2024 13:03
@web-padawan web-padawan force-pushed the test/app-layout-fixed-height branch from 4d8bf4e to 493004f Compare December 20, 2024 13:11
@@ -8,7 +9,7 @@ describe('app-layout', () => {

beforeEach(() => {
div = document.createElement('div');
div.style.height = '100%';
div.style.height = '600px';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we achieve the same by explicitly setting the viewport size?

await setViewport({ width: 1024, height: 600 });

Copy link
Contributor

@vursen vursen Dec 20, 2024

Choose a reason for hiding this comment

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

Or rather globally by specifying viewportSize here:

web-components/wtr-utils.js

Lines 270 to 275 in 3ee7650

{
name: `${theme[0].toUpperCase()}${theme.slice(1)} visual tests`,
build: `${process.env.GITHUB_REF || 'local'} build ${process.env.GITHUB_RUN_NUMBER || ''}`,
recordScreenshots: false,
recordVideo: false,
},

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, ideally we would use setViewport() but it needs to be implemented: modernweb-dev/web#1072
I'll look into that since it should be easy (especially when using concurrency: 1 and single session).

@web-padawan
Copy link
Member Author

Closing this for now. Will create a new PR once modernweb-dev/web#2850 is merged.

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