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

feat: insta for snapshot testing #688

Closed
wants to merge 2 commits into from

Conversation

vasucp1207
Copy link
Contributor

@vasucp1207 vasucp1207 commented Mar 8, 2024

Related to #634

Warning

This is just for checking how to update snapshots with insta. Not a migration from the current test system to insta snapshots.

Trying out the insta for snapshot testing and find out that moving to insta is not a big deal we just have to adjust the current testing logic.
Run cargo-test and cargo-insta-accept, snapshots will created in src/snapshots with ron format. No need for manually paste the diff.

cc @obi1kenobi

@@ -48,6 +48,7 @@ rayon = "1.8.0"

[dev-dependencies]
assert_cmd = "2.0"
insta = { version = "1.36.1", features = ["yaml"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the following at the bottom of this file might be benefitial according to their docs

# See https://insta.rs/docs/quickstart/#optional-faster-runs
[profile.dev.package]
insta.opt-level = 3
similar.opt-level = 3

Copy link
Owner

Choose a reason for hiding this comment

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

This PR is a bit outdated at this point, and the Cargo.toml already has a broader version of those directives already: profile.dev.package."*" is opt-level = 3

Comment on lines +410 to +412
let output_snap =
ron::ser::to_string_pretty(&expected_results, ron::ser::PrettyConfig::default()).unwrap();
insta::assert_snapshot!(query_name, output_snap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: Why did you use this vs insta::assert_ron_snapshot!(..)?
This seems a bit simpler, or am I missing something?

@obi1kenobi
Copy link
Owner

Hey @CommanderStorm, thanks for taking a look at this!

This PR has been dormant for a while, and I haven't heard from @vasucp1207 about any specific plans to resume working on it. (@vasucp1207 lmk if you're still interested, and no worries if not!)

Assuming @vasucp1207 isn't able to continue working on it (and we should give them a few days to reply), I'd be happy to have you take over the work and drive it home!

@vasucp1207
Copy link
Contributor Author

Since this is more than 4 months old I don't how the project has changed since then and what are the implications of this PR overall. So yes @CommanderStorm you can take onto this PR I have no problem.

@vasucp1207
Copy link
Contributor Author

Close this in favour of PR #951.

@vasucp1207 vasucp1207 closed this Sep 29, 2024
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.

3 participants