- Notifications
You must be signed in to change notification settings - Fork7.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
✅ Useinline-snapshot to support different Pydantic versions in the test suite#12534
base:master
Are you sure you want to change the base?
Conversation
ae68755 to8d1875bCompare8d1875b to9708ef1Compareca3c891 to0bdd732Compareinline-snapshot 0.15 is out. I changed the order of the v1,v2 arguments to match the order which is currently used in the tests. I changed not all the existing tests to use the new function. Let me know if you think this is needed. |
This is great! 🚀 I would love to have this in all the tests, but that's a giant task, you don't have to do it. 😅 Having this first one as a template is already great. 🚀 |
b430987 to2a7e93bCompare…fferent pydantic versions
2a7e93b tod8b34afCompareinline-snapshot to support different Pydantic versions in the test suiteinline-snapshot to support different Pydantic versions in the test suitesnapshot_pydantic to support different Pydantic versions in the test suitesnapshot_pydantic to support different Pydantic versions in the test suiteinline-snapshot to support different Pydantic versions in the test suiteThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks great, just left a few small suggestions & questions.
| @@ -117,14 +117,14 @@ def test_crud_app(client: TestClient): | |||
| ) | |||
| assert response.status_code == 200, response.text | |||
| assert response.json() == snapshot( | |||
| {"name": "Dog Pond", "age": None, "id": hero_id} | |||
| {"name": "Dog Pond", "age": None, "id":Is(hero_id)} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is the added advantage of usingIs() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
inline-snapshot tries to manage everything withinsnapshot() and wants toupdate the changes back to the code which it things is correct.
The user can take control back by using dirty-equals or Is().
inline-snapshot will also not fixIs(hero_id) in the future when the hero_id is not equal any more (tests fail for some reason). It preserves the changes made by the developer and does not revert them.
It makes things easier to implement, makes it easier to understand what happens in some cases and provides a distinction between code which is written by inline-snapshot and code which is written by the developer.
| @@ -71,9 +77,8 @@ def test_cookie_param_model_invalid(client: TestClient): | |||
| } | |||
| ] | |||
| } | |||
| ) | |||
| | IsDict( | |||
| # TODO: remove when deprecating Pydantic v1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should keep this TODO statement somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we add it to the function?
"Remove v1 argument when deprecating Pydantic v1"
Maybe we can usecodecrumbs in the future to clean things up.
There are a lot of refactorings missing which would be needed for this. But it would be a reason to continue this project :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we add it to the function?
Sure!
Co-authored-by: Sofie Van Landeghem <svlandeg@users.noreply.github.com>
@svlandeg, thank you for the review. I added the missing comment. |
This pull-request contains an example how inline-snapshot can be used with different snapshots for different pydantic versions.
The following code can be used to create different snapshots for each pydantic version (pytest has to be run with both pydantic version).
both snapshots will be created (see example in the pull-request).
@tiangolo
I would be interested if this approach looks useful to you, or if you prefer the current IsDict(...)|IsDict(...) pattern for some reason.
The next version of inline-snapshot will support things like
snapshot_pydantic()inside of snapshots. which will make the following code work:Thank you for using inline-snapshot 😃.