generated from cfpb/open-source-project-template
-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Snapshot testing] Improve test consistency #1103
Open
meissadia
wants to merge
14
commits into
main
Choose a base branch
from
1099-e2e-mask-footer
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meissadia
force-pushed
the
1099-e2e-mask-footer
branch
from
January 9, 2025 19:09
e871363
to
ecb8779
Compare
meissadia
requested review from
billhimmelsbach and
tanner-ricks
as code owners
January 9, 2025 19:10
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1099
I explored a few options to reduce the flakiness of our snapshot tests and the approach implemented in this PR seems to provide the most reliable result with sensible tradeoffs.
Local test runs in
headless
mode have passed 100%, 6x in a row, with 4 workers. Snapshot testing via the UI can still be rather flaky but, since tests pass consistently inheadless
mode, my interpretation is that UI failures are due to excessive system resource consumption by the Playwright UI causing inconsistent page rendering results.Changes
maxDiffPixels
from25
to0
, allowing us to catch even the smallest unexpected UI changesnapshotModifications.css
to apply snapshot-testing-specific style modifications. This approach allows us to replacedotted
borders withsolid
borders on all links to ensure consistent rendering. Since thedotted
style comes from the CFPB Design System, it does not seem necessary to verify it's implementation as part of our app (though it would certainly be preferred that these tests validate exactly what the user is expected to see).mask (ignore)
list. This element tends to trigger false-positives on tests with very tall screenshots and, since it's implementation comes from the Design System, does not seem to be something we need to validate as part of our app. As a next step, we could break these taller pages down into smaller tests of subsections of the page, but for now this workaround seems acceptable.How to test this PR
yarn start
yarn test:e2e:snapshot --headless
Notes - Alternate approaches not chosen
.u-usa-flag
and modifying the link border style, we were able to avoid the effort needed to significantly refactor how these tests are implemented.Screenshots