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

Resolve VRT issues for local runs #5824

Closed
aaemnnosttv opened this issue Sep 12, 2022 · 20 comments
Closed

Resolve VRT issues for local runs #5824

aaemnnosttv opened this issue Sep 12, 2022 · 20 comments
Labels
Type: Infrastructure Engineering infrastructure & tooling

Comments

@aaemnnosttv
Copy link
Collaborator

Bug Description

We recently updated our VRT infrastructure to support running on ARM/M1 machines which are becoming more common rather quickly.

While the build runs on these machines where it didn't before, it seems to have introduced some degree of instability in local runs on Intel machines (and M1s?).

At current, I see ~10 scenario failures when running locally, most of which seem to be very small differences in font rendering although some appear to have timeouts as well, albeit inconsistently.

I do see some amount of failures consistently though. See #4619 (comment)

Steps to reproduce

  1. Run npm run test:visualtest locally
  2. See failures

Additional Context

  • OS: MacOS 12.5.1

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Visual regression tests should consistently run successfully, both locally (on Intel + ARM) and on GitHub actions

Implementation Brief

Test Coverage

QA Brief

Changelog entry

@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Infrastructure Engineering infrastructure & tooling labels Sep 12, 2022
@asvinb asvinb self-assigned this Sep 20, 2022
@asvinb
Copy link
Collaborator

asvinb commented Sep 21, 2022

Update after spending some time on the ticket:

  • Have yet to determine the cause of the issue
  • Tried loading fonts within Storybook differently, using webfontloader and trigger backstop when the fonts are loaded via the custom wf-active class name.
  • Might not be related to whether fonts are already loaded.
  • Taking a lot of time to run the tests to check.

@eclarke1
Copy link
Collaborator

@FlicHollis I think we can remove this from Sprint 85 as it looks like it needs more work on the IB :-)

@FlicHollis
Copy link
Collaborator

This has been removed, thanks @eclarke1

@asvinb
Copy link
Collaborator

asvinb commented Sep 28, 2022

Latest updates:

  • Fonts are not rendering even though technically they are loaded.
    Reference:
    image

Test:
image

  • No improvements noted when increasing the delay in tests/backstop/config.js
  • The same tests consistently fail (for e.g Global admin bar) which means that potentially we are missing something in those tests.

@asvinb asvinb removed their assignment Oct 12, 2022
@asvinb asvinb self-assigned this Oct 21, 2022
@techanvil
Copy link
Collaborator

techanvil commented Oct 26, 2022

I am also seeing failures on my Linux workstation(Ubuntu 22.04, AMD64), similar to the description but more, ~30 failures here.

@techanvil techanvil added P0 High priority and removed P1 Medium priority labels Oct 26, 2022
@asvinb asvinb removed their assignment Oct 27, 2022
@asvinb asvinb self-assigned this Nov 3, 2022
@asvinb
Copy link
Collaborator

asvinb commented Nov 3, 2022

@aaemnnosttv Not sure if am missing something here and how the GH actions are all passing, but if you look at one of the reference image on develop and you compare it with what we currently have in Storybook for that image, they are different, with the GA4 CTA present in Storybook and not in the reference image.
Screenshots in case things change:

Storybook:
image

BackstopJS Reference:
image

Weird how it VRT fails locally on develop whereas in GH actions, everything works fine 🤔
Do you have any idea @aaemnnosttv @tofumatt @eugene-manuilov ?

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Nov 3, 2022
@aaemnnosttv
Copy link
Collaborator Author

Wow, that is indeed strange. Perhaps @techanvil or @nfmohit could investigate this further?

@aaemnnosttv aaemnnosttv removed their assignment Nov 9, 2022
@techanvil techanvil self-assigned this Nov 15, 2022
@techanvil
Copy link
Collaborator

Having discovered that the VRT suite is not actually running as expected in CI, I am parking this issue until that has been resolved: #6151

@techanvil
Copy link
Collaborator

Based on the work on #6151, it looks like this issue will still be relevant, with a reduction in the number of test failures but still a couple of cases that need fixing and also a more general issue of stability with unpredictable timeouts occurring locally.

@derweili
Copy link
Collaborator

@asvinb @aaemnnosttv
We fixed the VRT font loading issue with #6324
So I think we can close this issue.

@asvinb asvinb self-assigned this Jan 16, 2023
@techanvil
Copy link
Collaborator

I think it could be worth keeping to see about fixing the timeouts that occur when running locally...

@aaemnnosttv
Copy link
Collaborator Author

I tried running on develop today and most of them failed for me with what seemed to be impercievable font-related differences.

       report | Test completed...
       report | 16 Passed
       report | 152 Failed

E.g.

image

When scrubbing the diff, I can't really notice a difference 🤔

@derweili
Copy link
Collaborator

@aaemnnosttv what machine do you use? For me on mac M1 the VRT tests do not fail.

@asvinb
Copy link
Collaborator

asvinb commented Jan 17, 2023

@aaemnnosttv @derweili I just checked locally (Mac with Intel chip) and there are no failures:
image

I see images have been updated last night, can you try again @aaemnnosttv ?

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Jan 17, 2023
@derweili
Copy link
Collaborator

@asvinb the updates were only about removing unused reference images. This should not change anything for this issue.

@aaemnnosttv
Copy link
Collaborator Author

That's odd @asvinb – I'm also on a Intel based Mac. I ran it again on develop just now and got the same results 😞

@derweili
Copy link
Collaborator

derweili commented Jan 19, 2023

@aaemnnosttv is your VRT docker image up to date? Maybe you could try rebuilding it.

@aaemnnosttv
Copy link
Collaborator Author

@derweili I deleted the images I had and ran it again and everything's passing now 🤷‍♂️ 🎉

I'll close this out then but do you know what changed in the image? It's built based on the specific version of backstop but something must have changed 🤔

@aaemnnosttv aaemnnosttv removed their assignment Jan 23, 2023
@aaemnnosttv aaemnnosttv removed the P0 High priority label Jan 23, 2023
@derweili
Copy link
Collaborator

@aaemnnosttv actually I don't really know. With #6324 only images were updated that had visual changes, mostly due to font rendering errors.

The VRT image from your example was updated 1 month ago but also without visual changes. https://github.com/google/site-kit-wp/pull/6303/files

However, I experienced the error you had (errors without visual change), when I ran backstopjs locally without docker. I did this to debug font loading. So backstop used a local instance of chromium. When running the VRT tests this way, almost all the tests failed even though there were no visual changes.
So I thought, this could be related to a different font rendering/antialising setting or so.

So I though it might be related to an outdated VRT docker image with different settings.

But if deleting the test images helped for you, should be delete the test images before the test? Why do we need to keep the test images for all test runs?

@aaemnnosttv
Copy link
Collaborator Author

But if deleting the test images helped for you, should be delete the test images before the test?

I didn't delete the test images, I deleted the local Docker image! When running again, it rebuilt the image which seemed to do the trick. It must have been something in the alpine:latest base image that changed 🤔

Why do we need to keep the test images for all test runs?

I think this is just how Backstop works, but I could be wrong. It references the generated test files in the report it creates but I'm really not sure why it accumulates them the way it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

6 participants