2.1: delete the legacy namespace trees - #240
Closed
pftg wants to merge 2 commits into
Closed
Conversation
`rake test:canonical` is defined as "exactly what must still pass once test/legacy/ and the v1 trees are gone". Three times in one day a test asserting LEGACY behaviour was written into test/unit/, i.e. into that suite: a canonical surface table demanding the shim-only SnapDiff.start (#236), three legacy-constant probes in a canonical file (#237), and a pre-existing umbrella guard #236 had to relocate. Each would have failed the day the deletion landed, long after its author moved on. Reviews caught all three; the fourth would ship. The test-tree twin of core_tree_has_no_legacy_deps_test.rb: no file under test/unit/ or test/integration/ may require a doomed path, name a v1 namespace constant, or use a shim-only name (SnapDiff.start, .silence_deprecations, SnapDiff::Deprecation, suppress_migration_notice!). test/legacy/ is deliberately not policed -- exercising the legacy surface is its job. Same conventions as the twin: file:line: reason -- `code`, whole-line comments ignored, a vacuity guard, and a sub-test that fails on stale allowlist entries. The allowlist holds two entries, both gates rather than tests of behaviour (deletion_3_0_test.rb, which names the deletion set by construction, and the twin gate's own pattern literal). A third entry means canonical tests are still entangled and needs a decision, not a green build. This file cannot scan itself: a line-level allowlist has to quote the lines it blesses, and every quote is itself an offence -- no fixed point exists.
Removes the v1 compatibility surface: lib/capybara/, lib/capybara_screenshot_diff/,
the two gem-name entry points, snap_diff/legacy_shims.rb, snap_diff/deprecation.rb
and test/legacy/. 48 files, 2483 lines.
2.0 ships the compat layer intact -- working legacy names, per-constant
deprecation warnings, one migration notice per process. 2.1 completes the
move; docs/UPGRADING.md is the path.
The two edits the deletion needs, exactly as deletion_3_0_test.rb had been
asserting them for weeks:
- lib/snap_diff.rb drops `require "snap_diff/legacy_shims"`
- lib/snap_diff-capybara.rb repoints at "snap_diff/integrations/minitest"
Fallout beyond those two:
- test/test_helper.rb: SnapDiff::Deprecation.suppress_migration_notice! and
the Warning guard that raised on "[snap_diff deprecation]" both go -- the
channel that emitted those warnings no longer exists.
- Rakefile: `test:canonical` was "everything except test/legacy", which is
now `test`. Deleted rather than kept as a second name for one thing.
`test:benchmark` deleted too: it required
scripts/benchmark/find_region_benchmark, which is not in the repo, so the
task has been raising LoadError.
- scripts/generate_sample_report.rb (rake report:sample) and bin/console
loaded v1 entry points; repointed at canonical names.
Gates, per gate:
- test/legacy/legacy_tree_is_alias_only_test.rb -- proved the v1 trees held
no logic. Subject deleted; the test goes with it.
- test/unit/core_tree_has_no_legacy_deps_test.rb -- KEPT, repurposed. A
legacy require now fails loudly on its own, but a message or docstring
naming Capybara::Screenshot.* does not: it survives the deletion and
starts lying. The DELETED_IN_3_0 exclusion list is gone (both files it
named are deleted).
- test/unit/canonical_suite_has_no_legacy_refs_test.rb -- KEPT unchanged in
mechanism; the allowlist swaps deletion_3_0_test.rb for its replacement
and still holds two entries.
- test/unit/deletion_3_0_test.rb -- SIMULATED the deletion (copy lib/,
delete, probe). It is now real, and support_load_probe_test.rb runs the
same entry-point and advertised-constant tables against the real lib/ on
every run. Replaced by test/unit/legacy_surface_removed_test.rb, which
keeps the one claim nothing else makes: the removed paths are not back
under lib/ (everything there is packaged) and a fresh process defines
none of the removed names.
rake test: 468 runs, 0 failures, 1 skip. rake test:unit: 440/0. standardrb clean.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideThis PR removes the entire v1/legacy Capybara namespace surface and its deprecation machinery, repoints the remaining entry points and scripts to the canonical SnapDiff APIs, and replaces the legacy test split/deletion simulation with targeted gates that assert the legacy surface stays removed and never creeps back into the shipped code. Flow diagram for legacy surface removal verificationflowchart TD
Start["Load the real lib/ tree"] --> Paths["Assert removed paths are absent"]
Paths --> Process["Load the gem in a fresh process"]
Process --> Names["Assert removed names are undefined"]
Names --> Refs["Scan canonical code for legacy references"]
Refs --> Gate["rake test passes"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
|
Superseded by #249. #240 was opened before #242/#245/#246/#247/#248 landed, and the 2.1 scope has since grown to include the chunky_png driver, |
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
1 participant
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.
Draft. Nothing ships until the 2.1 plan is approved. No version bump, no
CHANGELOG entry, no release.
Stacked on #239 (the canonical/legacy test-split gate) — review that first;
this branch contains it.
2.0 ships the v1 compatibility layer intact: the old names keep working, each
one warns with the caller's own
file:line, and one migration notice perprocess points at the guide. 2.1 removes it. This PR is that removal, and
below is what a project on 2.0 changes to follow.
Migrating from 2.0 to 2.1
Measured, not imagined: a real consumer (a Jekyll/Rails site with committed
visual baselines, running its suite in Docker) was upgraded end to end against
this deletion. 17 lines, two files, zero blockers — 38 runs, 0 failures,
55 screenshots compared, byte-identical before and after. Every v1 API had a
canonical equivalent.
Requires
require "capybara_screenshot_diff/minitest"require "snap_diff/integrations/minitest"require "capybara_screenshot_diff/rspec"require "snap_diff/integrations/rspec"require "capybara_screenshot_diff/cucumber"require "snap_diff/integrations/cucumber"require "capybara_screenshot_diff/reporters/html"require "snap_diff/reporters/html"require "capybara/screenshot/diff"require "snap_diff"gem "capybara-screenshot-diff"(Bundler auto-require)gem "snap_diff-capybara"Configuration
Both v1 holders collapse into one object.
SnapDiff.configureis the singleconfig entry point (ADR-008).
Capybara::Screenshot.<setting> = …SnapDiff.config.<setting> = …Capybara::Screenshot::Diff.<setting> = …SnapDiff.config.<setting> = …Capybara::Screenshot::Diff.configure { |c| … }SnapDiff.configure { |config| … }SnapDiff.start { |screenshot, diff| … }SnapDiff.configure { |config| … }SnapDiff.startyielded the two v1 holders, so it could not outlive them —removed, not renamed. Same for
SnapDiff.silence_deprecationsandSNAP_DIFF_SILENCE_DEPRECATIONS: with no deprecations left to emit there isnothing to silence.
Constants and includes
Capybara::Screenshot::Os.nameSnapDiff::Os.nameCapybara::Screenshot::Diff::ImageCompareSnapDiff::ComparisonCapybaraScreenshotDiff::Reporters::HTMLSnapDiff::Reporters::HTMLCapybaraScreenshotDiff::ScreenshotAssertionSnapDiff::ScreenshotAssertioninclude CapybaraScreenshotDiff::DSLinclude CapybaraScreenshotDiff::Minitest::Assertionsinclude SnapDiff::Minitest::Assertions— the two collapse into oneCapybara::Screenshot::Osis the one to grep for. In the real upgrade itwas the only hard crash: on 2.0 it raises
NameErrorfrom the shim internalsonce the require line has been migrated but the constant has not — a
partially-migrated setup looks fine (config setters keep working) until
Osaborts the whole suite before a single test runs. On 2.1 it is simply gone.
Stubbing point
If your suite stubs the detected driver list, the constant moved:
SnapDiff::Drivers::AVAILABLE_DRIVERSis the published stubbing point;SnapDiff::Drivers.availableis the read API.What this deletes
lib/capybara/,lib/capybara_screenshot_diff/,lib/capybara-screenshot-diff.rb,lib/capybara_screenshot_diff.rb,lib/snap_diff/legacy_shims.rb,lib/snap_diff/deprecation.rb,test/legacy/.48 files, 2483 lines removed — 858 lines in 38
lib/files, 1429 in the 9test/legacy/files, 196 in the deletion simulation. Whole diff: 57 files,+140 / −2639.
Two edits carry the deletion, exactly as
deletion_3_0_test.rbhad beenasserting them for weeks:
lib/snap_diff.rbdropsrequire "snap_diff/legacy_shims", andlib/snap_diff-capybara.rbrepointsat
snap_diff/integrations/minitest.Fallout beyond those two
test/test_helper.rb—SnapDiff::Deprecation.suppress_migration_notice!and the
Warningguard that raised on any[snap_diff deprecation]messageboth go. The channel that emitted them no longer exists, so the guard could
never fire again.
Rakefile—test:canonicalwas defined as "everything excepttest/legacy/", which is now exactlytest. Deleted rather than kept as asecond name for one thing;
rake testis the gate. Also deletedtest:benchmark: it requiresscripts/benchmark/find_region_benchmark,which is not in the repo, so the task has been raising
LoadError(and itsbody named a v1 constant).
scripts/generate_sample_report.rb(rake report:sample) andbin/consoleloaded v1 entry points; repointed at canonical names. Bothverified running.
Gate dispositions
test/legacy/legacy_tree_is_alias_only_test.rbtest/unit/core_tree_has_no_legacy_deps_test.rbrequirenow fails loudly by itself, but a user-facing message or docstring namingCapybara::Screenshot.*does not — it survives the deletion and starts lying. ItsDELETED_IN_3_0exclusion list is gone (both files it named are deleted) and its framing moved from "sogit rmdoesn't break the gem" to "so the removed names don't creep back"test/unit/canonical_suite_has_no_legacy_refs_test.rb(#239)deletion_3_0_test.rbfor the replacement below and still holds two entriestest/unit/deletion_3_0_test.rblib/, delete, probe entry points in a subprocess) because the trees were still there. They are not, andsupport_load_probe_test.rbalready runs the same entry-point and advertised-constant tables against the reallib/on every run — so the ~200 lines of simulation harness, including the gate line that existed to prove the simulation was looking at the deleted tree, went with the treesIts replacement,
test/unit/legacy_surface_removed_test.rb(~50 lines), keepsthe one claim nothing else makes — absence: no removed path is back under
lib/(everything there is packaged, so a bad rebase ships it), and a freshprocess loading the gem defines none of the removed names. Both halves were
mutation-checked:
touch lib/snap_diff/deprecation.rbreds the first,def self.startinsnap_diff.rbreds the second.Suites
Baseline on master was 598/0/1 for
rake testand 570/0 fortest:unit; thedrop is
test/legacy/(9 files) and the deletion simulation leaving, minus thenew gates arriving.
Two things a reviewer will ask
1. Removing a public API in a MINOR departs from semver. It does. The
mitigation is that it is announced rather than surprising: 2.0's release
notes,
docs/UPGRADING.md, and the runtime migration notice must all say"removed in 2.1" explicitly, so the contract is published before it is
enforced. On 2.0 a v1 user gets working legacy names, a per-constant warning
carrying their own
file:line, and one migration notice per process pointingat the guide — the deprecation machinery built in #237 serves real users, not
just the beta line. Prerequisite on master, not on this branch (this branch
deletes the notice): the notice text currently says "REMOVED in 3.0" and must
be changed to 2.1 before 2.0 ships.
2. chunky_png and
shift_distance_limit. ADR-008 also schedules both forremoval. Not implemented here — that is a separate decision, still pending,
and this PR neither removes nor blesses them.
Not done here, deliberately: the docs sweep
Docs still describe the v1 surface as present and still say "3.0" where they
now mean "2.1". Left out to keep this diff reviewable; listing what I saw, by
match count:
docs/UPGRADING.md(62) ·docs/configuration.md(31) ·docs/framework-setup.md(19)·
docs/architecture.md(17, and it documentslegacy_shims.rbas an existingfile) ·
README.md(13) ·docs/snapdiff.md(10) ·docs/drivers.md(4) ·docs/ci-integration.md(3) ·docs/migration-guide.md(3) ·CONTRIBUTING.md(3)·
docs/organization.md(2) ·docs/thread_safety.md(2) ·docs/reporters.md(2)Not all are wrong — a migration guide must name the old API — but every one
needs a read. Same for the remaining
3.0vocabulary outside the files this PRtouches.
Summary by Sourcery
Delete the v1 compatibility surface and enforce the canonical SnapDiff API for 2.1.
Enhancements:
Tests: