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

VRT tests cannot be run on ARM Macs/M1 Macs #4619

Closed
tofumatt opened this issue Jan 5, 2022 · 16 comments
Closed

VRT tests cannot be run on ARM Macs/M1 Macs #4619

tofumatt opened this issue Jan 5, 2022 · 16 comments
Labels
P0 High priority Type: Infrastructure Engineering infrastructure & tooling

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Jan 5, 2022

Bug Description

Using an ARM/M1 Mac, running npm run test:visualtest will fail with an error because the Docker image for Backstop is not configured to run an ARM64 image. It will segfault and fail. If you are on a machine that you migrated from an Intel Mac, it will then open whatever previous results you ran in your project folder.

Until BackstopJS provides a fully-integrated solution (see: garris/BackstopJS#1300), we will need to update our codebase to allow the VRTs to run on ARM Macs, as several core team members are using these machines and can't run (and thus can't update) the VRTs locally.


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

Acceptance criteria

  • It should be possible to run the VRT (Visual Regression Tests) using npm run test:visualtest locally on both Intel and ARM machines. It would be preferable if these could be run on ARM without needing to create any custom images/etc. locally.

Implementation Brief

  • Create a custom Docker image based on the one in this comment: VRT tests cannot be run on ARM Macs/M1 Macs #4619 (comment)
    • It should be noted this image let me run the VRTs on macOS after updating Docker to use at least 4 CPU cores. This might need to be added to the docs or a script to detect CPUs available to Docker. 🤔
  • Publish the image to GitHub Packages so CI and team members can use the image
  • Ensure the BackstopJS code uses our new googlesitekit/vrt:VERSION image instead of backstopjs/backstopjs:VERSION
  • Update reference images in case of any minor/font differences
  • Ensure tests run on ARM and Intel macs do not differ; ensure CI tests still pass after being updated by an ARM-based Mac.

Test Coverage

  • VRT reference images will likely need updating after this change.

QA Brief

  • No QA needed.

Changelog entry

  • Update Visual Regression test code to run on ARM-based Macs.
@tofumatt tofumatt self-assigned this Jan 5, 2022
@tofumatt tofumatt added P0 High priority Type: Infrastructure Engineering infrastructure & tooling labels Jan 5, 2022
@henrywright

This comment has been minimized.

@tofumatt
Copy link
Collaborator Author

Update: looks like garris/BackstopJS#1300 (comment) might fix it for us, I'll try that later today as I'm working on an issue that'll require VRT updates 😅

@tofumatt
Copy link
Collaborator Author

Another update: looks like Puppeteer will soon have an ARM build, there's a PR here: puppeteer/puppeteer#7546 and the issue is here: puppeteer/puppeteer#6622.

Waiting a bit longer on this one as the workarounds aren't working for me…

@FlicHollis
Copy link
Collaborator

Hi @tofumatt just checking you are still planning on working on this IB? If not feel free to unassign yourself.

@tofumatt
Copy link
Collaborator Author

tofumatt commented Jun 6, 2022

Hi @FlicHollis indeed, it's mostly waiting on the upstream fixes mentioned in #4619 (comment). But I'm keeping myself assigned to write up the IB once that's ready.

@tofumatt
Copy link
Collaborator Author

tofumatt commented Jun 6, 2022

Looks like Puppeteer 14 includes a fix. I'll test that and write up an IB soon if that works 🙂

@tofumatt
Copy link
Collaborator Author

tofumatt commented Jun 7, 2022

Updating BackstopJS and Puppeteer to their latest versions thus far hasn't fixed the issue…

Oddly it still tries to run BackstopJS 6.0.4 when using the npm run test:visualtest command 🤔

CleanShot 2022-06-07 at 23 20 17

@tofumatt
Copy link
Collaborator Author

Just another update here: even using Backstop 6.1.0 this still isn't fixed, I think we're waiting on more from garris/BackstopJS#1300. I might try making our own Dockerfile for this as other users in that issue have reported success with that approach 🤔

CleanShot 2022-07-13 at 23 43 46

@tofumatt
Copy link
Collaborator Author

tofumatt commented Aug 1, 2022

Creating a custom Docker image using this Dockerfile:

FROM node:16.3.0

ARG BACKSTOPJS_VERSION

ENV \
	BACKSTOPJS_VERSION=$BACKSTOPJS_VERSION

# Base packages
RUN apt-get update && \
	apt-get install -y git sudo software-properties-common

RUN sudo PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true npm install -g --unsafe-perm=true --allow-root backstopjs@${BACKSTOPJS_VERSION}

RUN wget https://dl-ssl.google.com/linux/linux_signing_key.pub && sudo apt-key add linux_signing_key.pub
RUN sudo add-apt-repository "deb http://dl.google.com/linux/chrome/deb/ stable main"

# RUN	apt-get -y update && apt-get -y install google-chrome-stable

# RUN apt-get install -y firefox-esr

# gconf-service libxext6.... added for https://github.com/garris/BackstopJS/issues/1225

RUN apt-get -qqy update \
  && apt-get -qqy --no-install-recommends install \
    chromium \
    libfontconfig \
    libfreetype6 \
    xfonts-cyrillic \
    xfonts-scalable \
    fonts-liberation \
    fonts-ipafont-gothic \
    fonts-wqy-zenhei \
    gconf-service libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgcc1 libgconf-2-4 libgdk-pixbuf2.0-0 libglib2.0-0 libgtk-3-0 libnspr4 libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 libxcursor1 libxdamage1 libxss1 libxtst6 libappindicator1 libnss3 libasound2 libatk1.0-0 libc6 ca-certificates fonts-liberation lsb-release xdg-utils wget \
  && rm -rf /var/lib/apt/lists/* \
  && apt-get -qyy clean


WORKDIR /src

ENTRYPOINT ["backstop"]

And then building it and using that image, along with ensuring Docker for Mac could use at least 4 CPUs let me run the VRTs. Without upping the CPU limit to at least 4, it would hang:

CleanShot 2022-08-01 at 23 47 57

@tofumatt
Copy link
Collaborator Author

tofumatt commented Aug 3, 2022

This is mostly ready, but any issues from CI images and the involvement of multiple developers to test on both Intel and ARM Macs means I'm putting this one at a 7.

@tofumatt tofumatt removed their assignment Aug 3, 2022
@techanvil techanvil self-assigned this Aug 4, 2022
@techanvil
Copy link
Collaborator

IB ✅

@techanvil techanvil removed their assignment Aug 4, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 8, 2022
@eugene-manuilov
Copy link
Collaborator

@tofumatt could you please review my PR #5692 and try it on your end? I have added a custom image for backtopjs uses chromium browser to render stories. Unfortunately, sometimes it works unstable on my end and fails due to some small fonts changes. Can you try to troubleshoot it on your end? Also as to the registry, I think we won't be able to use GitHub's container registry because it saves images on the organisation level, not on the repository one. So it means that if we want to publish an image, it will be available for the whole Google organisation which is undesirable.

@tofumatt tofumatt removed their assignment Sep 2, 2022
@aaemnnosttv aaemnnosttv self-assigned this Sep 8, 2022
@aaemnnosttv
Copy link
Collaborator

Approval ⚠️

I can't test this on an M1 but @tofumatt already confirmed in the initial review that it worked for him. However, I'm getting failures on my Intel machine both times after running twice.

The first run produced 3 total failures, 2 of which were timeouts

Puppeteer encountered an error while running scenario "Modules/Analytics/Settings/SettingsEdit/WithExistingGTMPropertyMatching"
TimeoutError: Navigation timeout of 60000 ms exceeded
Puppeteer encountered an error while running scenario "SearchConsole/SearchFunnelWidget/ReadyWithActivateAnalyticsCTA"
TimeoutError: Navigation timeout of 60000 ms exceeded

The third was this one which seems to be a 0% diff but different size output?
image

The second run produced 8 failures, only 1 of which was a timeout (also different than the first run)

Puppeteer encountered an error while running scenario "SearchConsole/SearchFunnelWidget/ReadyWithCreateGoalCTA"
TimeoutError: Navigation timeout of 60000 ms exceeded

The remaining failures had actual visible diffs

Failed diff images

failed_diff_google-site-kit_Global_Admin_Bar_0_document_2_large
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormGA4_WithBothTags_0_document_0_small
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormGA4Transitional_WithExistingGTMPropertyMatching_0_document_0_small
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormGA4Transitional_WithExistingGTMPropertyMatching_0_document_1_medium
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormGA4Transitional_WithGA4AndUAExistingTag_0_document_1_medium
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormUA_WithBothTags_0_document_0_small
failed_diff_google-site-kit_Modules_Analytics_Setup_SetupFormUA_WithoutExistingTag_0_document_2_large

@tofumatt can you also test this with Intel?

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Sep 8, 2022
@tofumatt
Copy link
Collaborator Author

tofumatt commented Sep 9, 2022

Hmm, I'm not currently able to run the tests on Intel on a Mac, just on a Windows PC I have. And even then, I usually run them in a Linux VM. Even when I'd run the VRTs on my M1 in "Intel" it'd be x86 emulation running a Linux VM, and it was so slow that I'd often get weird failures.

I think we'll need someone on Intel to test, but I think @eugene-manuilov mentioned the tests were a bit flakey for him as well after this change? I wonder if there's anything we should be doing to improve that? Or just have longer timeouts? 🤷🏻‍♂️


That said, I see failures locally too:

CleanShot 2022-09-09 at 01 24 37

I saw similar issues when I ran the tests locally from the original PR, but then updated the VRTs and saw they still passed on CI (which isn't ARM) so I figured it was all good. But it seems there are super-subtle rendering differences and timeout issues with the new image 🤔

@aaemnnosttv
Copy link
Collaborator

Closing this as done as a first step. Will open a new issue to see about addressing the failures we're seeing locally.

@aaemnnosttv
Copy link
Collaborator

@FlicHollis I've opened #5824 as the follow up for this sprint which is already assigned 👍

@aaemnnosttv aaemnnosttv removed their assignment May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

7 participants