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

ci: fix flaky animation story #1804

Merged
merged 4 commits into from
Aug 31, 2022
Merged

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Aug 30, 2022

Summary

Adds maxDiffPixelRatio, maxDiffPixels and threshold options to screenshot options and passes to playwright expect(element).toMatchSnapshot() method.

Fixes flaky annotation story with minor diff.

Error: Screenshot comparison failed:

  5 pixels (ratio 0.01 of all image pixels) are different

Expected: /app/e2e/test_failures/annotations_stories-Annotations-stories-Hover--acf9e-her-annotations-when-rect-annotation-is-hovered-chrome/annotations-stories/hover-state/should-fade-all-other-annotations-when-rect-annotation-is-hovered-expected.png
Received: /app/e2e/test_failures/annotations_stories-Annotations-stories-Hover--acf9e-her-annotations-when-rect-annotation-is-hovered-chrome/annotations-stories/hover-state/should-fade-all-other-annotations-when-rect-annotation-is-hovered-actual.png
    Diff: /app/e2e/test_failures/annotations_stories-Annotations-stories-Hover--acf9e-her-annotations-when-rect-annotation-is-hovered-chrome/annotations-stories/hover-state/should-fade-all-other-annotations-when-rect-annotation-is-hovered-diff.png

  33 |     });
  34 |     test('should fade all other annotations when rect annotation is hovered', async ({ page }) => {
> 35 |       await common.expectChartWithMouseAtUrlToMatchScreenshot(page)(
     |       ^
  36 |         url,
  37 |         {
  38 |           bottom: 188,

    at /app/e2e/page_objects/common.ts:368:25
    at /app/e2e/page_objects/common.ts:379:5
    at /app/e2e/page_objects/common.ts:414:7
    at /app/e2e/tests/annotations_stories.test.ts:35:7

Minor diff screenshot below 🧐

image


Adds retry logic to exec helper function to retry processes when needed. This was the suggestion from ops team as they have similar network issues with buildkite agents. This does not fix network issues with commands not using exec.

Should prevent errors such as...

yarn install v1.22.15
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
info There appears to be trouble with your network connection. Retrying...
error An unexpected error occurred: "https://registry.yarnpkg.com/unquote/-/unquote-1.1.1.tgz: Request failed \"503 Service Unavailable\"".
info If you think this is a bug, please open a bug report with the information provided in "/app/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Failed to run command: [yarn install --frozen-lockfile --ignore-scripts]

@nickofthyme nickofthyme added visual testing issues related to visual testing :ci internal Internal changes with no external impact labels Aug 30, 2022
@nickofthyme
Copy link
Collaborator Author

buildkite test this

@nickofthyme
Copy link
Collaborator Author

buildkite test this

@nickofthyme
Copy link
Collaborator Author

buildkite test this

@markov00
Copy link
Member

buildkite test this

(testing this a bit more)

@markov00
Copy link
Member

buildkite test this

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, I've run the CI multiple and seems stable

@nickofthyme nickofthyme merged commit 80846c2 into elastic:master Aug 31, 2022
@nickofthyme nickofthyme deleted the fix-flaky-test branch August 31, 2022 14:28
@nickofthyme
Copy link
Collaborator Author

It's strange to me that just setting the maxDiffPixels does nothing because threshold and maxDiffPixelRatio are still controlling from being set in playwright.config.ts, which is why I have to clear them manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ci internal Internal changes with no external impact visual testing issues related to visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants