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

Add Jest Snapshot Testing #208

Closed
Eric-Arellano opened this issue Mar 13, 2023 · 1 comment · Fixed by #345
Closed

Add Jest Snapshot Testing #208

Eric-Arellano opened this issue Mar 13, 2023 · 1 comment · Fixed by #345
Assignees
Milestone

Comments

@Eric-Arellano
Copy link
Collaborator

Problem

The current sphinx theme is really easy to accidentally break styling without realizing it, such as #206. This is partially because the CSS file is so huge, and also because we have many overriding rules that it's unclear what actually gets used and doesn't.

It's too hard to manually catch issues. There's too much to visually inspect in the rendered docs and the human eye is bad at recognizing differences like 1.0rem vs 1.5rem padding.

Proposal

With each PR, diff how the final CSS rules are impacted for every element in our sample docs/ project. That allows code authors and reviewers to see if there are unintentional changes.

We might be able to use https://github.com/evolvingweb/sitediff.

Otherwise, we can write a script that gets all the elements from the rendered docs/ site, then calls JavaScript's getComputedStyle() to get the final style rules for each element. Then, save those to something like a text file. Git will then diff for us.

Comments/questions:

  1. Which elements do we show? Do we somehow want to deduplicate?
  2. If we ever make content changes to docs/, it may mess up the diff because there are new or removed elements from before. Thus, content changes to docs/ should be kept separate from styling changes to our CSS.
@Eric-Arellano Eric-Arellano self-assigned this Mar 13, 2023
@Eric-Arellano
Copy link
Collaborator Author

A friend who specializes in frontend recommended that we can probably hook up Jest Snapshot Testing, which will be much more useful and robust: https://jestjs.io/docs/snapshot-testing

@Eric-Arellano Eric-Arellano changed the title Add diff workflow to see what a PR impacts Add Jest Snapshot Testing Mar 14, 2023
Eric-Arellano added a commit that referenced this issue Apr 27, 2023
Prework for #208.

Abby and I decided that we want Jest tests for two purposes:

1. Checking that web components are in sync between the Pytorch and Furo
themes, e.g. we didn't forget to update one of the folders.
1. We could do this either by using Snapshot testing or by simply
checking that the file contents are equal.
2. We have some non-trivial logic in
#267 and want to write
automated unit tests.

This PR adds the basic infrastructure, like setting up CI to install
Node.js. That will make it easier to review follow-up PRs that add
actual tests.

> We could do this either by using snapshot testing

I think we'd benefit from adding snapshot testing for the top nav bar
rendering correctly. I want to do this in a follow up PR so we can
properly decide if we want to add snapshots or not; the decision to use
snapshots is separate from whether to use Jest generally.
@Eric-Arellano Eric-Arellano modified the milestones: Theme v1.12.0, Furo May 3, 2023
Eric-Arellano added a commit that referenced this issue Jun 9, 2023
Closes #208.

## How snapshots work

Playwright snapshot tests take screenshots of certain portions of our
site, such as the footer. They save that screenshot to Git, so that we
all have the exact copy. Then, for every PR, our tests will retake a new
screenshot and make sure it's the exact same as before. That way, we
always know when we make a change.

When there was a change, Playwright will output this:

![Screenshot 2023-05-26 at 12 09 19
PM](https://github.com/Qiskit/qiskit_sphinx_theme/assets/14852634/a5157d1d-f9de-4e0a-a93b-93c413dba848)

As stated there, Playwright will generate a before, after, and diff
photo. In CI, we upload those as GitHub artifacts.

If the change was intentional, then the author copies the updated
screenshot to overwrite the old file.

## Uses Docker for consistency

Websites render slightly differently between operating systems, e.g.
Linux and macOS. Naively, this would cause visual regression tests to
fail when running the tests locally on a mac.

So, we use Docker to make sure we always use the same environment. 

If users don't want to run Docker locally, they can rely on CI to get
the updated snapshots via GitHub Actions Artifacts.

More advanced users can also run the tests locally. They need to install
Docker and have the Docker daemon running. But otherwise, our scripts
automate everything. They don't need to know how to use Docker. They
only need to run `npm test`.

## How to run locally

(Reminder that contributors can rely on CI for visual regression
testing.)

The user has to first build the docs with `THEME=_qiskit_furo tox -e
docs`. Then, they only run `npm test` (after originally running `npm
install` & starting Docker).

Playwright will start up a server for us.

If the user changed the theme, they must rebuild the docs with Tox.

## Switches from Jest to Playwright

I originally tried doing this change via Jest, a common JavaScript test
runner. But I had major issues getting the browser automation library
Puppeteer #341 to work
properly on my M1.

So, this changes our test runner to
[Playwright](https://playwright.dev), a test runner from Microsoft
optimized for visual regression testing like we're doing. It can do
things like click buttons, e.g. our Translations menu.

## Only Furo

To keep things simple, this only adds infrastructure to snapshot test
Furo. Given that our goal is to migrate ASAP, this seemed okay to me.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment