Skip to content

The new-screenshot error prints a path that will never exist in CI #260

Description

@pftg

Under ADR-010 (2.0 warns, 2.1 deletes) this is a 2.0 blocker — it is a live bug regardless of the release plan.

The bug

lib/snap_diff/screenshot_matcher.rb raises, on a screenshot with no baseline:

No existing screenshot found for <path>.png!
To record it: run the test, then `git add <path>.png` and commit -- baselines are read from git.

Under CI that file is never written. build_screenshot_assertion calls check_base_screenshot before capture_screenshot (screenshot_matcher.rb:24-31), and fail_if_new defaults to !ENV["CI"].nil? (config.rb:106) — so in CI the raise happens first and nothing lands on disk.

$ CI=true bin/rails test test/system/about_page_test.rb
Failure: No existing screenshot found for .../doc/screenshots/about_page.png!
$ ls doc/screenshots/about_page.png
ls: no such file
$ CI= bin/rails test test/system/about_page_test.rb
$ ls doc/screenshots/about_page.png
doc/screenshots/about_page.png     # written

The instruction is unfollowable on the exact run that prints it. Two independent judges on the #259 panel found this ordering defect without being told about it.

This is the rule from #259 broken in a message we wrote ourselves: never print a command in an error message that is not generated from live state. Same shape as RECORD_SCREENSHOTS=1, which sat in this message for years pointing at a feature that never existed.

Checklist

  • Reproduce first — failing test before any fix
  • Capture before the raise, so the file exists whenever we name it. Careful: check_base_screenshot also performs the git checkout that drives need_to_compare? — only the raise moves, not the checkout
  • Message becomes a function of live state: name the path only when the file was actually written
  • Guard test written as the USER's code, not an internal unit test
  • Mutation-check: revert the fix, confirm the test reds, restore (targeted Edit, never git checkout <file>), confirm green
  • rake test:unit + rake test:canonical + standardrb lib test green
  • Blocks: the accept workflow in Accept-workflow and honest reporting: adopt what Playwright, VCR and SimpleCov already proved #259 — flipping any default without this gives failure without the file

Related, same file, not yet fixed

  • lib/snap_diff/errors.rb:12-14 asserts "EVERY error this gem raises inherits SnapDiff::Error". True at the raise sites, misleading about what a user sees: integrations/minitest.rb:38-39 converts ExpectationNotMet to Minitest::Assertion, integrations/rspec.rb:60-61 to RSpec::Expectations::ExpectationNotMetError. The docs were corrected in docs: stop teaching two commands that do not work #257; the code comment now contradicts them
  • SnapDiff::ErrorWithFilteredBacktrace does not inherit SnapDiff::Error — it is its superclass

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions