Lift the two fixtures that had nothing left to decide - #376
Merged
Conversation
sync_session (23 files) and db_factory (3) had one body, one scope and no autouse between them, so they move to conftest and the 26 local definitions go. Everything else stays where it is, and the reason for each is recorded next to them: what was considered, and why lifting it would trade visible duplication for invisible inheritance.
This file contains hidden or 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
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.
sync_session(23 files) anddb_factory(3) had one body, one scope and noautouse between them, so there was nothing left to decide about them. They move
to
conftest.pyand the 26 local definitions go.Everything else stays, and the conftest records why, because the next person
counting fixtures will otherwise repeat the investigation:
_workspace- 12 files, one body, one scope, but autouse and spread acrossfour directories. The only conftest covering all four is the top one, so
lifting it hands an autouse fixture to the whole backend suite: 12 recipients
becomes thousands. Its cost is one
monkeypatch.setenv, but the real cost isestablishing that none of those thousands depends on
WORKSPACE_HOST_PATHbeing unset, which exceeds the value of removing 12 duplicates.
client(89 files, 14 bodies),app(79, 5),db_session(66, 11)silently unless every one of them overrides, and pytest gives no sign of which
file diverges or why. Visible duplication beats invisible inheritance: these
have already diverged, and the divergence would stop showing.
session(15 files, 9 bodies),_clean_env(12, 10) - as many shapesas files. There is no common version to lift.
temp_backups/temp_backups_root,anon_client/client,_patch_async_client/patch_async_client.How the comparison was done is written down too, since it decides the answer:
bodies compared with string literals and decorators kept. Erasing strings
merged eight unrelated fixtures, the strings being the payload; erasing
decorators would have merged different lifetimes, and that is what would have
made
_workspacelook liftable.An unrelated failure this surfaced
Running the 26 affected files together,
test_reset_demo_scope_db.pyfails twotests on a database trigger: "last active super_admin cannot be removed". It does
the same on
mainwith the same selection and a fresh database, and the samewhen run alone. What it does not do is fail against a database that earlier
runs have already populated - there it passes.
So the file depends on state it does not create, and the dependency is invisible
whenever the suite happens to supply it. Not caused by this change and not fixed
here; recorded because "passes on a used database, fails on a fresh one" is the
kind of thing that gets attributed to whatever change is in flight when it
finally shows.