Skip to content
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

draft: Prevent lots of empty inline snapshots with --force-update-snapshots without --accept #573

Closed
wants to merge 1 commit into from

Conversation

max-sixty
Copy link
Collaborator

I realize what you mean in the prior comment. Currently running with --accept is kinda painful. I'm not sure how common that is, but would be nice to fix.

Had a go for 30 mins, need to run, but pushing an initial effort and a pointer to the thing that needs fixing

…apshots` without `--accept`

I realize what you mean in the prior comment. Currently running with `--accept` is kinda painful. I'm not sure how common that is, but would be nice to fix.

Had a go for 30 mins, need to run, but pushing an initial effort and a pointer to the thing that needs fixing
@max-sixty
Copy link
Collaborator Author

OK, I think I understand the underlying problem here. In a test like:

assert_snapshot!("test", @r#test#");
  • The macro invocation only sees the test value; it doesn't know about the r#...#
  • insta can't find out about the r#...#, only cargo-insta, because only cargo-insta has the rust parser
  • We need to compare the full r#...# to correctly decide whether a snapshot requires updating when --force-update-snapshots is passed, and we need to do that with insta only
  • Unless we can run matches_fully correctly, we default to creating pending snapshots for snapshots which don't need updating, which is what the existing master does

A couple of ideas, none are great:

  • Have --force-update-snapshots accept by default. This is reasonable UX — it's generally something folks are not running on every loop, but rather an occasional clean-up.
    • Currently, running --force-update-snapshots --accept is a reasonable experience — it prints a few too many names of snapshots it's updating, but that's the only cost.
    • It also sounds like that's what it's doing already, or we could pick a new name --force-accept if we wanted
  • Revert Enable inline snapshots to be force-updated #569, give up on force-updating inline snapshots.
  • Offer some additional option; --force-update-inline-snapshots. This would be a shame.

My current thinking is to revert #569, and then default to --force-update-snapshots accepting over a couple of versions.

@max-sixty
Copy link
Collaborator Author

I think this is broadly fixed in #581

@max-sixty max-sixty closed this Sep 2, 2024
@max-sixty max-sixty deleted the force-update-inline branch September 2, 2024 19:27
max-sixty added a commit that referenced this pull request Sep 19, 2024
#581)

This solves the issue in #573 for the moment:
- When `--force-update-snapshots` in passed, we only update inline
snapshots when there's some difference _within_ the string, such as an
additional linebreak at the start or the end
- We no longer update the surrounding hashes. In the existing code, this
happened because we were writing too many inline snapshots, not because
we were actually checking the number of hashes
- Checking the number of hashes isn't easy to do — reason outlined at
#573 (comment)
- The main change in the code is that we store the unnormalized snapshot
and then normalize when we need to. The existing code stored the
normalized version, which meant we couldn't evaluate whether the
unnormalized versions were different

It's a decent number of changes, but will integrate nicely with
#563.

~(FYI we currently don't look at the indentation, but we could adjust
this)~ Now indentation works too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant