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

utopia-gen: Use insta for snapshot testing #1247

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Dec 19, 2024

I saw in #145 that snapshot testing had been considered in the past, but then assert-json-diff was used instead. This PR migrates a couple of the utoipa-gen tests to use insta instead of performing a basic string comparison.

insta produces diffs like below, which makes it much easier to see at first glance what changed. The cargo-insta extension also makes it very fast to update snapshots in case of any desired changes.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: utoipa-gen/tests/snapshots/schema_generics__generic_schema_full_api.snap
Snapshot: generic_schema_full_api
Source: utoipa-gen/tests/schema_generics.rs:221
────────────────────────────────────────────────────────────────────────────────
Expression: doc
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
   64    64 │                     "items"
   65    65 │                   ],
   66    66 │                   "properties": {
   67    67 │                     "items": {
   68       │-                      "type": "object",
         68 │+                      "type": "array",
   69    69 │                       "items": {
   70    70 │                         "type": "object",
   71    71 │                         "required": [
   72    72 │                           "id",
────────────┴───────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`

If this is considered useful we could potentially also migrate the other tests to use snapshots instead of assert-json-diff.

@juhaku
Copy link
Owner

juhaku commented Dec 19, 2024

Try running tests with scripts/test.sh you can also specify crate by scripts/test.sh utoipa for example. I should setup a proper build and testing with Makefile or justfile or cargo make and there is an issue for it #1040 but it hasn't been started yet.

@juhaku
Copy link
Owner

juhaku commented Dec 19, 2024

The tests seems to fail because there are snapshots that are not generated for tests run with rc_schema feature flag on seemingly.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Dec 19, 2024

yeah, I must've missed running them for some reason. should be fixed now though :)

@Turbo87 Turbo87 changed the title utopia-gen/tests/schema_generics: Use insta for snapshot testing utopia-gen: Use insta for snapshot testing Dec 19, 2024
Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Pretty neat, 🥇 Perhaps we could push this insta bit further and migrate to it all the way. Though not necessarily in this PR as it is quite a lot of tests but could be done in future. 🤔

@juhaku
Copy link
Owner

juhaku commented Dec 19, 2024

And like before, the CHANGELOG.md entry would be useful 🙂 It is bit annoying extra step but allows better change tracking for other developers who are interested of changed details.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Dec 19, 2024

And like before, the CHANGELOG.md entry would be useful 🙂 It is bit annoying extra step but allows better change tracking for other developers who are interested of changed details.

hmm, which changelog would this go into (utoipa-gen?)? and under which category (Changes?)?

Perhaps we could push this insta bit further and migrate to it all the way. Though not necessarily in this PR as it is quite a lot of tests but could be done in future. 🤔

yep, should be relatively straight-forward to incrementally migrate the other tests too.

open question would be whether to use inline snapshots or regular ones like in this PR. the inline snapshots are closer to the current usage with json!() directly in the test code, but they are slightly more verbose and require recompiling the tests whenever a snapshot is changed.

I personally tend to use inline snapshots when the snapshot is relatively short, but with the amount of data that we're snapshotting here, I think I would use regular ones.

`insta` produces diffs like the following, which makes it much easier to see at first glance what changed. The `cargo-insta` extension also makes it very fast to update snapshots in case of any desired changes.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: utoipa-gen/tests/snapshots/schema_generics__generic_schema_full_api.snap
Snapshot: generic_schema_full_api
Source: utoipa-gen/tests/schema_generics.rs:221
────────────────────────────────────────────────────────────────────────────────
Expression: doc
────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬───────────────────────────────────────────────────────────────────
   64    64 │                     "items"
   65    65 │                   ],
   66    66 │                   "properties": {
   67    67 │                     "items": {
   68       │-                      "type": "object",
         68 │+                      "type": "array",
   69    69 │                       "items": {
   70    70 │                         "type": "object",
   71    71 │                         "required": [
   72    72 │                           "id",
────────────┴───────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
@juhaku
Copy link
Owner

juhaku commented Dec 19, 2024

hmm, which changelog would this go into (utoipa-gen?)? and under which category (Changes?)?

Changed is good enough category, and yes to the CHANGELOG of a project the changes are made to. Should write the code of conduct files etc. for this purpose 😄

@Turbo87
Copy link
Contributor Author

Turbo87 commented Dec 19, 2024

okay, should be added correctly now :)

@juhaku
Copy link
Owner

juhaku commented Dec 19, 2024

yep, should be relatively straight-forward to incrementally migrate the other tests too.

open question would be whether to use inline snapshots or regular ones like in this PR. the inline snapshots are closer to the current usage with json!() directly in the test code, but they are slightly more verbose and require recompiling the tests whenever a snapshot is changed.

I personally tend to use inline snapshots when the snapshot is relatively short, but with the amount of data that we're snapshotting here, I think I would use regular ones.

Yeah, probably which ever comes easier, since there will be quite a few tests. I guess convention to keep the test file name same as the test function name makes it easier to find.

@juhaku juhaku merged commit 66b80a4 into juhaku:master Dec 19, 2024
12 checks passed
@Turbo87 Turbo87 deleted the insta branch December 19, 2024 13:02
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.

2 participants