Make running Playwright to update snapshots easier

[madewithkode]

Fixes

Fixes #4535 by @sarayourfriend

Description

This PR aims to make debugging Playwright test failures caused by snapshot changes easier by modifying the current Playwright CI workflow to run tests with the -u option which basically updates modified snapshots automatically. We then upload a gzipped git patch blob of these changes so the initiating user can download and apply the patch to their branch without having to run Playwright updates(which happen to be a resource intensive process) locally.

Testing Instructions

This can be tested by triggering a failing Playwright workflow due to UI changes.

Checklist

• My pull request has a descriptive title (not a vague title likeUpdate index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.
• I ran the DAG documentation generator (./ov just catalog/generate-docs for catalog
  PRs) or the media properties generator (./ov just catalog/generate-docs media-props
  for the catalog or ./ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[github-actions]

Full-stack documentation: https://docs.openverse.org/_preview/4585

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

Changed files 🔄:

• https://docs.openverse.org/_preview/4585/frontend/reference/testing_guidelines.html

[sarayourfriend]

Looks good to me, just two small things to change:

1. Remove the need to disable SC2086 or add a comment explaining why it needs to be disabled.
2. Retain the previous behaviour of uploading the test-results trace file, as it serves a purpose beyond snapshot test debugging.

The rest are clarifications/questions/nit picks. Looks good otherwise though 🙂

[sarayourfriend]

Just two small things on conditions for the steps I am pushing changes for, but otherwise this looks good to me. If we find we need to adjust, this is a perfect base to iterate on (but for now I think this should work just fine and will be a good improvement).

[sarayourfriend]

It occurred to me when thinking about the condition on the step to upload the snapshot diff artefact, that we need this step to run even if there testing step fails. By default, it wouldn't, so if there are failures in addition to changed snapshots, we'd miss the changed snapshots.

Suggested change

	id: check_snapshots_changed
	id: check_snapshots_changed
	if: success() || failure()

[sarayourfriend]

@madewithkode it looks like this change got lost in the mix, but I think we still need it in the case I mentioned where there are multiple types of failures.

[krysal]

This is looking very cool! Generating snapshots is quite a consuming task. I tried the flow in #4600 and (with some little changes) it was a breeze to get the updated snapshots.

My only concern is that I did not get failing tests from the changes. They're supposed to fail so you are aware that you need to update them. Are we on the same page here that this is still the expected behavior?

[krysal]

I downloaded it as playwright_vr_snapshot_diff.zip so I had to decompress it twice (first from a .zip and then from a .gz). It seems that the GitHub actions always do the compression for downloads, so we can skip that part.

Suggested change

	`cat snapshot_diff.patch.gz | ov gunzip -kc - | git apply`
	`cat playwright_vr_snapshot_diff.zip | ov unzip -kc - | git apply`

[sarayourfriend]

Nice!

[madewithkode]

Thanks for pointing this out @krysal and you're indeed correct as regards the expected behavior. A slight change to this part of your suggestion would be:

cat *_snapshot_diff.zip | ov unzip -kc - | git apply

This is because the prefix * represents the current playwirght test. It is dynamic(could vary between playwright_vr or storybook) and auto generated in the workflow using a matrix. I'd go ahead and note this on the doc too.

[krysal]

Let's skip this compression.

Suggested change

	git add . && git diff --staged --binary | gzip -f > /tmp/snapshot_diff.patch.gz
	git add . && git diff --staged --binary > /tmp/snapshot_diff.patch

[krysal]

Not exactly accurate and mostly redundant. I'd omit the comment altogether.

Suggested change

	# This step only runs if there was a failure and snapshots indeed changed.
	# This step only runs if snapshots are changed.

[sarayourfriend]

You're right, @krysal, it should still consider it a failure in the check. I guess we can fix that by adding exit 1 to the script that creates the patch diff when there are updated snapshots. Maybe it should also log the list of updated snapshots in that step too.

[madewithkode]

@sarayourfriend I'd think that git diff --binary --name-only in the patch creation script itself already logs the list of updated snapshots. If this is not the case, perhaps something like git diff --binary --name-only | echo should suffice? Also, I agree with exiting with 1 to trigger a failure.

This would be my modification of the patch creation script(let me know if it makes sense):

      if [ -n "$(git diff --binary --name-only)" ]; then

        git add . && git diff --staged --binary > /tmp/snapshot_diff.patch

        echo "snapshots_changed=true" | tee "$GITHUB_OUTPUT"

        echo "The following snapshots were updated:"

        git diff --binary --name-only | echo

        exit 1

      else

        echo "snapshots_changed=false" | tee "$GITHUB_OUTPUT"

      fi

[sarayourfriend]

Because git diff --binary --name-only is captured in the test expression, it's stdout doesn't go to the actual logs. Or at least, that's my understanding of how output capture in scripts work, and it's how it behaves when I try it locally.

You should force a playwright failure in this PR so we can see how it goes in practice 🙂

The change to the script looks fine, but you don't need to pipe to echo. You'd also need to add --staged in the second invocation if you used your version, git diff only shows unstaged changes by default, and the git add . will have staged everything. Something like this would be a bit more idiomatic bash script-y (I think):

diff_names="$(git diff --binary --name-only)"
if [ -n "$diff_names" ]; then
  # ...
  printf "The following snapshots were updated:\n%s\n" "$diff_names"
  exit 1
else
  # ...
fi

By the way, just to clarify the reason to stage the changes, we want to make sure that new snapshots are included. git diff won't show unstaged changes for new paths. Maybe worth including a comment to explain that in the script, it's probably not obvious why it's needed.

[madewithkode]

@sarayourfriend @krysal I have just triggered a playwright failure with the latest workflow changes on this build on this PR. Everything on the workflow works as expected. An update to snapshots triggered a failure and both test results and the snapshot patch/diff were uploaded.

However, I have noted a few things that might require slight changes with the realization that we no longer need to gzip/gunzip as Github CI zips artifacts by default.

After trying to make this work for myself locally while trying as much as I can to assume the role of a new contributor, the following command set seems more intuitive and works out of the box.

unzip -q *_snapshot_diff.zip && git apply snapshot_diff.patch

• unzip -q *_snapshot_diff.zip basically uses the unzip tool to unpack the contents of *_snapshot_diff.zip.
  The -q option tells unzip to operate quietly, suppressing informational messages during the extraction process.

Note: The ov toolkit does not seem to have the unzip utility installed yet. FWIW, unzip is widely available on most Unix based OS. However if we decide to use ov instead and add unzip to the ov toolkit, the command can easily be modified to ov unzip -q *_snapshot_diff.zip && git apply snapshot_diff.patch

Running the above allowed me to seamlessly apply the downloaded patch without any errors. If this is okayed across board i'd proceed to update the PR with the changes.

[sarayourfriend]

Awesome, glad the workflow is working!

You can definitely add unzip to ov, that'd be great! Piping the unzipped output would be better, I think, because unzipping two archives that have the same name of file inside the archive will overwrite each other, won't they? If both the storybook and playwright snapshots have diffs, wouldn't they output to the same snapshot_diffs.patch file, and overwrite whichever one the glob chose as second?

To add unzip to ov, add it to the dnf-installed packages in docker/dev_env/Dockerfile. Fedora calls it unzip (you can search packages with ov dnf search):

unzip.x86_64 : A utility for unpacking zip files

After that, this works well for me locally and is nice and succinct. We could probably add a just recipe (or ov alias) for the unzip part too, to make it less fiddly, but not required for this PR if it's a hassle or behaves strangely for whatever reason.

ov unzip -p *_snapshot_diff.zip | git apply

[openverse-bot]

Based on the contributor urgency of this PR, the following reviewers are being gently reminded to review this PR:

@zackkrida
@AetherUnbound
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 7 day(s) ago. PRs labelled with contributor urgency are expected to be reviewed within 3 weekday(s)².

@madewithkode, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

1. Specifically, Saturday and Sunday. ↩

2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

[madewithkode]

@sarayourfriend you're indeed correct about the patch overriding possibilities as the actual patches for both tests would aways have same name, thanks for spotting that. This, alongside other suggestions have been addressed and I can confirm that the latest version works as it should locally and can also be confirmed from this test run.

[sarayourfriend]

LGTM. I have a few bits of feedback on the docs, but none are blocking. The only blocking part is the condition of the snapshot check job, which needs to run if the previous step fails. Otherwise this LGTM!

[sarayourfriend]

@madewithkode it looks like this change got lost in the mix, but I think we still need it in the case I mentioned where there are multiple types of failures.

[sarayourfriend]

Unfortunately #1986 means we need to manually wrap the text inside code blocks, to at least around 80 characters. It's so tedious I really don't try too hard other than to get it around 80. Still needs to be done, unfortunately.

There's also a space missing between file and the parenthetical after it.

[sarayourfriend]

Also: for the files to be in context for ov, they must be downloaded to the repository. The repository is the only host filesystem location that ov explicitly mounts into the container. Probably worth just adding that to the instructions: download the zipped snapshot patches to the repository root, etc.

[sarayourfriend]

Just a nit, and something we need to address more widely in our documentation (it's unfortunately pervasive, including in my own writing), but this would read a lot more clearly in active voice, rather than passive, and even more so as imperative instructions. "Once downloaded, apply them to your working branch by running", etc.

[sarayourfriend]

You can leave this out, in favour of the general advice we've written in #4567. We've decided not to have to write this caveat everywhere and just have it in a single place.

[madewithkode]

LGTM. I have a few bits of feedback on the docs, but none are blocking. The only blocking part is the condition of the snapshot check job, which needs to run if the previous step fails. Otherwise this LGTM!

@sarayourfriend If I get you correctly, you basically want a failure() check added to check_snapshots_changed step? If this is correct, this would mean that it'd only run when playwright test step above exits with 1, however, I don't think snapshot changes/updates are typically considered a failure by playwright, so this might mean that the snapshot check step would be skipped if there were no actual test failures but only snapshot changes, which I assume isn't the behavior we want. I stand to be corrected.

[sarayourfriend]

It should be failure() || success(). If the previous step fails on non-snapshot tests, but also includes changes to the snapshots, then we still need the next step to run and generate the diff. It needs to run:

• When the test run step "succeeds", so we can check for changed snapshots, generate the diff, and fail if necessary
• When the test run step fails due to non-snapshot related errors, so that we can also check for changed snapshots, generate the diff, etc.

It needs to run in both cases. Right now it will only run in the first one, so that if there are both types of test failures, the CI run would only show non-snapshot related failures.

[sarayourfriend]

LGTM!

[sarayourfriend]

Docs build failure was in the storybook build part of the job, not the docs changed in this PR. I've just kicked off a re-run of the job. Looks like we got rate limited in the request to GlotPress.

@dhruvkb Maybe we should consider caching translation results for 24 hours and only pulling new ones once a day instead of on every single CI/CD run. I'll write an issue for it. Issue here: #4614

[obulat]

Thank you for all the work to get this working, @madewithkode ! This will make the process of updating tests much simpler