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

Fix snapshots within a nested crate #531

Closed
wants to merge 15 commits into from
Closed

Conversation

max-sixty
Copy link
Collaborator

Fixes #396

@max-sixty
Copy link
Collaborator Author

max-sixty commented Jul 21, 2024

I've now pushed a slightly broader change, because I was concerned that not passing any packages would fail to include interior packages.

  • In the existing code, we placed all packages in LocationInfo, and then sometimes passed selected packages to functions when required.
  • In the initial commit of the PR, we no longer necessarily pick up all code by running the root package (because we want to skip interior packages). Without better test coverage I'm not sure whether that can miss parts of the repo in some conditions.
  • With the subsequent commits, we now place all packages we want to run over in LocationInfo. Making this explicit in a single place means we're less likely to make a mistake. There's a bit more cloning, but of tiny objects outside any hot loops, and I think worth waiting for Upgrade structopt to clap #518 etc before optimizing those.

@max-sixty max-sixty requested a review from mitsuhiko July 21, 2024 22:50
max-sixty added a commit to max-sixty/insta that referenced this pull request Jul 22, 2024
On top of mitsuhiko#531; that needs to merge first. This simplifies that code
@regexident
Copy link

@max-sixty It looks like this introduces a regression, which I unfortunately haven't yet been able to reproduce with the MCVE:

The regression is that while running insta test with --accept --workspace now works as expected it fails to accept the snapshots when running with a specific package via --accept --package "package-name".

This is the console output when running with --accept --workspace:

cargo insta test --release --accept --workspace
...

running 3 tests
stored new snapshot <SNIP>/crates/<SNIP>/tests/snapshots/<SNIP>.snap.new
stored new snapshot <SNIP>/crates/<SNIP>/tests/snapshots/<SNIP>.snap.new
...
test snapshots ... ok

...

insta review finished
accepted:
  crates/<SNIP>/tests/snapshots.rs (<SNIP>.snap)
  crates/<SNIP>/tests/snapshots.rs (<SNIP>.snap)
  ...

And this is with --accept --package "<SNIP>":

cargo insta test --release --accept --package "<SNIP>"
...

running 3 tests
stored new snapshot <SNIP>/crates/<SNIP>/tests/snapshots/<SNIP>.snap.new
stored new snapshot <SNIP>/crates/<SNIP>/tests/snapshots/<SNIP>.snap.new
...
test snapshots ... ok

...

Running the latter command (i.e. --accept --package) with the currently released version of cargo-insta I get the correct behavior.

Looks like it now under-counts in certain circumstances, instead of double-counting. 😅

@max-sixty
Copy link
Collaborator Author

max-sixty commented Jul 24, 2024

Thanks! If there's any way of getting a repo that would be really helpful. I'll try to have a think about what might be going on but I suspect it'll be difficult to manage without a repro.

Do you know whether the failure happens when the selected package is the root vs the interior package?


One other oddity I noticed using this branch on my work is that snapshots are getting saved with empty headers. I'll try and make a repro myself (it doesn't seem to be all snapshots...) edit: unrelated & fixed!

@regexident
Copy link

regexident commented Jul 28, 2024

@max-sixty It looks like this introduces a regression, which I unfortunately haven't yet been able to reproduce with the MCVE:

I was wrong, it can be reproduced with the MCVE!

I had commented out the root package. Once I reset the shared repo to commit b8a2892 (7fd61fb would work, too), installed cargo-insta from branch 396 and ran cargo insta test --accept --package "member-crate" I got this:

cargo insta test --accept --package "member-crate"
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.03s
     Running unittests src/lib.rs (target/debug/deps/member_crate-7d5d5f9bfb1209aa)

running 1 test
stored new snapshot <SNIP>/insta-bug-repro/crates/member-crate/src/snapshots/member_crate__tests__member.snap.new
test tests::test_member ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s

   Doc-tests member_crate

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

done: no snapshots to review

@max-sixty
Copy link
Collaborator Author

Great! Can repro, looking now...

@max-sixty
Copy link
Collaborator Author

Fixed! Let me know if that works for you.

(separately I'm thinking about how we can have reasonable integration tests — atm I don't think we're compounding that well, given someone could break this and we wouldn't immediately find out, which then makes changes more difficult. I very much appreciate your patience and help fixing this one...)

if let Some(manifest_path) = manifest_path {
cmd.manifest_path(manifest_path);
}

// TODO: why do we have this? When do we ever want dependencies here?
Copy link
Collaborator Author

@max-sixty max-sixty Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone reviewing — all this code is replaced in #536

max-sixty added a commit to max-sixty/insta that referenced this pull request Jul 28, 2024
Very draft — but shows a reasonably scalable way to add integration tests. There are some TODOs at the top.

This would help with issues such as mitsuhiko#531
max-sixty added a commit to max-sixty/insta that referenced this pull request Jul 28, 2024
Very draft — but shows a reasonably scalable way to add integration tests. There are some TODOs at the top.

This would help with issues such as mitsuhiko#531
@max-sixty
Copy link
Collaborator Author

This now has a nice integration test with a new integration test approach, in #537

@regexident
Copy link

Fixed! Let me know if that works for you.

It does! 🥳

@max-sixty
Copy link
Collaborator Author

I would like to merge #537 first and add a test, as I'm not sure I've got the logic right here (even though it seems to pass our hand-tests).

So removing the review request for the moment

@max-sixty max-sixty removed the request for review from mitsuhiko August 2, 2024 06:52
@mitsuhiko
Copy link
Owner

I merged #537.

@mitsuhiko
Copy link
Owner

So looking through this I think we need real integration tests here. I find it hard to follow both existing code and the changes here with confidence that nothing regressed.

@max-sixty
Copy link
Collaborator Author

So looking through this I think we need real integration tests here. I find it hard to follow both existing code and the changes here with confidence that nothing regressed.

Yes agree (re my request to merge #537 first!)

I ended up just integrating everything into #532, because it made the code simpler and easier to debug. So going to close this in favor of that.

@max-sixty max-sixty closed this Aug 2, 2024
@max-sixty max-sixty deleted the 396 branch August 2, 2024 21:43
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.

Duplicated snapshots and os error 2 when running a workspace + root crate
3 participants