-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
Fix unused snapshots not triggering failure in CI #128162
Conversation
unfortunately, I think reverting #127736 is the more reasonable option in this case, since not detecting unused snapshots while running with pytest-xdist is a known limitation of syrupy (syrupy-project/syrupy#668) |
tests/conftest.py
Outdated
# Override default finish to detect unused snapshots despite xdist | ||
SnapshotSession.finish = override_syrupy_finish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a workaround for an upstream issue?
I honestly really don't think this belongs in our codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a workaround for an upstream issue?
Yes it is a workaround for an upstream issue.
There is a PR open there, but it fills a gap right now, see #128477
I honestly really don't think this belongs in our codebase.
I have added a comment to indicate that it can be removed when upstream is implemented
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
a4429d3
to
7b12941
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @epenet 👍
../Frenck
This fix seems to cause a performance regression as a side effect. E.g.: This takes 25.75 sec without this fix on my dev env. and 72.24 sec with. When coverage is tested too, it gets worse: |
Than we need to revert 👍 |
Happy to revert - but I had checked overall run time and didn't see such discrepancy. |
Proposed change
syrupy
is currently unable to detect unused snapshots when it is run in conjuction withxdist
It is a know existing limitation of
syrupy
https://github.com/syrupy-project/syrupy?tab=readme-ov-file#known-limitationsSee also:
syrupy-project/syrupy#535
I have started work on implementing a clean version upstream: syrupy-project/syrupy#901, but I recommend that we add this workaround temporarily, until it is finalised there.
Sample failed run:
https://github.com/home-assistant/core/actions/runs/11380993290/job/31661830329
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: