test: migrate the remaining snapshot-test to insta

[CommanderStorm]

I know that during #688 you said that waiting is appropriate.
Thing is that this is a really low ammount of work which is still left (given that insta is already in use in other parts)
Am I missing something?

Hope that @vasucp1207 does not mind too much ^^

For reviewing of this PR:

• I have split up the commits to be easy to review. There are a few formatting differences between insta and the previous technique.
  => Please review this commit by commit to make this reviewable
• Most of the changes are whitespace
  => please press the ignore whitespace button in the review window

Note

(unless that is not something you care about for these files 😉): If you decide to merge, please don't squash it to prevent the git history of the files getting lost. (moving and editing in 2 commits is history preseeving, I don't know about squashing)

Resolves #634
Closes #688

[obi1kenobi]

Hey! Thanks for doing all the work to make this happen.

An unfortunate part about reviewing individual commits is that the GitHub UI doesn't allow suggesting changes at the individual commit level. So when I make comments, you'll have to do a bunch of rebasing to apply the change to the appropriate commit. Are you sure you're okay with that?

We've generally preferred squash-merging so that there's less ceremony around making trivial updates to PRs, and so that every commit on main is valid, buildable, and passes tests. Also, so that we can revert PRs in one command, instead of having to revert all the commits separately. But if you don't mind the overhead of all those rebases (and we'll need a couple of iterations here), I don't mind doing per-commit reviews here.

Just one thing before we get started: could you separate out the "unrelated snapshot fixes" commit into its own PR, since it doesn't seem related to the rest of the work here?

[obi1kenobi]

Overall this is quite close to merging — thank you for putting it together!

I would just like to remove the changes from the integration test snapshots from this PR and make them into their own PR if they are necessary. With that + the minor nitpicks above, I think this can merge 🎉

[obi1kenobi]

A few remaining TODOs before we can merge:

• Please remove the changes in the unrelated snapshot files, since we'll want to merge them in a separate PR if they are needed.
• Please read through the Adding a new lint section and update the process description as needed to fit the new workflow. For example, it currently still references .output.ron files.
• Please update the make_new_lint.sh script as needed. I'm not sure if it's better to generate an empty snapshot file, or to not generate a snapshot at all — what do you think?

[obi1kenobi]

(continuing the discussion from #957)

About the different file suffixes, that is is as far as I read into it not configurable. This makes sense as the files contain the metadata and the snapshot => would not be valid .ron anyway => no syntax highlighting..

Here are the two current usages using .snap

• https://github.com/obi1kenobi/cargo-semver-checks/blob/main/test_outputs/witnesses/function_missing.snap
• https://github.com/obi1kenobi/cargo-semver-checks/blob/main/test_outputs/integration_snapshots__bugreport.snap

=> Not using .snap as a suffix would also be a bit confusing 😅

All this seems reasonable. In #957 I brought up the .snap suffix since IIRC we were just renaming .ron to .snap without using insta's format which is where the metadata gets added. When we're using insta, I think it's reasonable to do things insta's way :)

[CommanderStorm]

I'm not sure if it's better to generate an empty snapshot file, or to not generate a snapshot at all — what do you think?

I don't think this terribly matters.
Added a file as this way users know where to look, but 🤷🏻

[obi1kenobi]

I'm not sure if it's better to generate an empty snapshot file, or to not generate a snapshot at all — what do you think?

I don't think this terribly matters. Added a file as this way users know where to look, but 🤷🏻

Once we merge this, it would be a great idea if you were to run through the lint-writing process end-to-end and experience it as a new user would. That way, any issues that come up can be fixed while all this is still fresh in our minds and before any new contributors run into problems.

When this PR is ready for another review, let me know using the "re-request review" button (circular arrows) in the reviewers section of the right sidebar area!

[CommanderStorm]

Yes, having both a .snap.new and a .snap file was what I would love to make sure we prevent

When the testcase sees such a case, it will pass without failiour and remove .snap.new if the content is identical.
If not, it will fail and show the diff.

I added a simple testcase to check for the existance of .snap.new and show an appropriate errormessage.

[CommanderStorm]

it would be a great idea if you were to run through the lint-writing process end-to-end and experience it as a new user would. That way, any issues that come up can be fixed while all this is still fresh in our minds and before any new contributors run into problems.

Shure can try myself on the weekend.
Do you think one of these is a good fit as a simple, straigtforeward lint?

• a public API enum's variant used to have a well-defined discriminant, but no longer does New lints: Changes to enum variant discriminants #898
• New lint: public API struct became enum or union #954

[obi1kenobi]

The changes to this file are deeply problematic.

This is not the right place for this test case, since everything in this file is doing snapshot testing of the CLI specifically.

The code is also not up to par quality-wise:

• it does not match the surrounding code or comment style
• it has a random println!() which will produce confusing cargo test output
• it is not properly formatted
• it has excessive line lengths well above 100 chars, etc.

Honestly, it looks like something an AI bot spat out, instead of a high-quality PR contribution.

These are all issues you could have caught in a self-review. Instead, I just spent 15 minutes writing down things you could have spotted and fixed yourself in 5 minutes. And now we're both wasting our time.

Please do self-reviews before requesting a review from someone else. It is not kind to make other people catch bugs in your code when you could have caught them yourself.

[CommanderStorm]

I have moved the testcase to src/snapshot_tests
The file seemed like the best place to put this testcase as I only searched in the tests directory for tests. Creating another module for this seemed not quite something that I guessed you'd approve 😓

The tight formatting requirements around matching other code are really hard to match, given that there are a lot of unwritten rules for this.
This is something which seems obvious to you (given that you have seen all of the codebase) but does not look wrong when I review my code, sorry 😅

And no, this is just me (maybe a bit tired, but that is not a good excuse), no AI.
Maybe I should let PRs rest and look at them with a fresh head on the next day to prevent this, but I cannot guarantee that I would have caught on to the differenece in for example .unwrap vs .expect("failed to.. in the file..
Likely that is just experience of working with rust. Sorry

[obi1kenobi]

Shure can try myself on the weekend. Do you think one of these is a good fit as a simple, straigtforeward lint?

• a public API enum's variant used to have a well-defined discriminant, but no longer does New lints: Changes to enum variant discriminants #898
• New lint: public API struct became enum or union #954

Either of those lints is fine, but only if you are willing and able to thoroughly self-review your code in GitHub's UI before marking the PR as open for reviews. Please don't make me point out obvious issues with the code that you could have found & fixed in less time than it'll take me to write a review comment.

[obi1kenobi]

Thanks, this is much better. There's still room for growth, and as long as you are working hard on improving your skills, I'm happy to keep pointing you in the right direction.

[obi1kenobi]

This is excellent: it succinctly describes the test at the top, it explains why the thing it looks for is a problem, the capitalization and style matches throughout, and the lines aren't too long so it's reasonable to have multiple editors side-by-side without needing to scroll or wrap lines.

Well done! Now this code is super easy to maintain, and I'm happy to merge it.

[obi1kenobi]

This doc comment is totally fine and there's no need to change it.

I want to offer a couple of other alternatives which would have also been fine in this context, along with some discussion on which to pick when.

A) This is a private, test-only function whose name already describes what it does. The API works in the "one obvious way" given the function signature. No doc comment would have been okay in this case, since the doc comment here doesn't say anything that would be surprising to a programmer with zero cargo-semver-checks familiarity.

B) A one-liner doc comment would also have been okay: /// Recursively lists all files in the directory. Hard and soft links are ignored.. This variant of the doc comment actually communicates more information than the current one, even though it's shorter: we explicitly call out a design decision that "could have gone either way" and callers might not known which way we picked. We've avoided not-particularly-useful info (e.g. helper function), so we're helping the reader focus on what's most important or what might be surprising.

Option A) is fine for test code like here. Option B) is preferred inside the tool itself, especially if the decision on not following hard and soft links was less obvious and not directly just 5-10 lines below the function signature.