-
Notifications
You must be signed in to change notification settings - Fork 1
Improve ergonomics for regenerating snapshots #7
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR removes the regenerate parameter from the snapshot testing API and replaces it with two more ergonomic update mechanisms: an environment variable (JULIA_SNAPSHOTTESTS_UPDATE) for automated updates and interactive prompting when running in a terminal.
Key changes:
- Removed the
regenerateparameter fromtest_snapshotfunction signature - Added
force_update()to check theJULIA_SNAPSHOTTESTS_UPDATEenvironment variable - Implemented interactive update prompting via
input_bool()when tests fail in interactive mode - Auto-creation of missing snapshot directories with informational messages
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/snapshots.jl | Core implementation: removed regenerate parameter, added auto-creation logic, interactive update prompts, and helper functions force_update() and input_bool() |
| test/test_update_modes.jl | New comprehensive test suite covering environment variable behavior, interactive input simulation, auto-creation, and successful snapshot scenarios |
| test/runtests.jl | Added new test file to the test suite execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinteractive() || force_update() | ||
| if force_update() || input_bool("Replace snapshot with actual result (in $subpath)?") |
Copilot
AI
Jan 5, 2026
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.
The condition isinteractive() || force_update() will enter the interactive prompt even when force_update() is true. Since force_update() already implies automatic updates, the inner condition on line 45 is redundant. Consider restructuring to handle forced updates separately from interactive prompts to avoid unnecessary prompt display when auto-updating.
| if isinteractive() || force_update() | |
| if force_update() || input_bool("Replace snapshot with actual result (in $subpath)?") | |
| if force_update() | |
| rm(expected_path; recursive=true, force=true) | |
| cp(snapshot_dir, expected_path; force=true) | |
| @info "Snapshot updated at $expected_path" | |
| @info "Please run the tests again for any changes to take effect" | |
| elseif isinteractive() | |
| if input_bool("Replace snapshot with actual result (in $subpath)?") |
| # Create a temporary file with the input | ||
| mktempdir() do tmpdir | ||
| input_file = joinpath(tmpdir, "input.txt") | ||
| write(input_file, input_string) | ||
|
|
||
| open(input_file, "r") do input_io | ||
| redirect_stdin(input_io) do | ||
| redirect_stdout(devnull) do | ||
| SnapshotTesting.input_bool("Test prompt") | ||
| end | ||
| end |
Copilot
AI
Jan 5, 2026
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.
The test_input helper function creates a temporary directory and file for each test case, which adds unnecessary overhead. Consider using IOBuffer for simpler and more efficient input simulation: redirect_stdin(IOBuffer(input_string)) would eliminate the need for temporary files.
| # Create a temporary file with the input | |
| mktempdir() do tmpdir | |
| input_file = joinpath(tmpdir, "input.txt") | |
| write(input_file, input_string) | |
| open(input_file, "r") do input_io | |
| redirect_stdin(input_io) do | |
| redirect_stdout(devnull) do | |
| SnapshotTesting.input_bool("Test prompt") | |
| end | |
| end | |
| # Use an in-memory IOBuffer to simulate stdin | |
| io = IOBuffer(input_string) | |
| return redirect_stdin(io) do | |
| redirect_stdout(devnull) do | |
| SnapshotTesting.input_bool("Test prompt") |
Currently when we use
SnapshotTesting.jlwe need to maintain a global variable indicating whether or not we're regenerating snapshots, and then manually set this variable back and forth when we make apparently breaking changes. This PR adopts some of the ergonomics fromReferenceTests.jl, getting rid of this global variable in each test suite and instead providing 2 options:JULIA_SNAPSHOTTESTS_UPDATEindicating whether or not we force updates (defaults to false)We get rid of the
regenerateflag from the API, so this is a breaking change and requires a major version bump.