site-kit-wp
site-kit-wp copied to clipboard
VRT tests cannot be run on ARM Macs/M1 Macs
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: https://github.com/garris/BackstopJS/issues/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: https://github.com/google/site-kit-wp/issues/4619#issuecomment-1201808910
- 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 ofbackstopjs/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
there are many binaries in the wild that aren't generated for ARM architecture. my lazy solution was to change computer. sorry if this doesn't help
Update: looks like https://github.com/garris/BackstopJS/issues/1300#issuecomment-1069146987 might fix it for us, I'll try that later today as I'm working on an issue that'll require VRT updates π
Another update: looks like Puppeteer will soon have an ARM build, there's a PR here: https://github.com/puppeteer/puppeteer/pull/7546 and the issue is here: https://github.com/puppeteer/puppeteer/issues/6622.
Waiting a bit longer on this one as the workarounds aren't working for meβ¦
Hi @tofumatt just checking you are still planning on working on this IB? If not feel free to unassign yourself.
Hi @FlicHollis indeed, it's mostly waiting on the upstream fixes mentioned in https://github.com/google/site-kit-wp/issues/4619#issuecomment-1104094196. But I'm keeping myself assigned to write up the IB once that's ready.
Looks like Puppeteer 14 includes a fix. I'll test that and write up an IB soon if that works π
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 π€

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

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:

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.
IB :white_check_mark:
@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.
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?
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
@tofumatt can you also test this with Intel?
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:
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 π€
Closing this as done as a first step. Will open a new issue to see about addressing the failures we're seeing locally.
@FlicHollis I've opened https://github.com/google/site-kit-wp/issues/5824 as the follow up for this sprint which is already assigned π