-
Notifications
You must be signed in to change notification settings - Fork 102
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
Please add ignored fields #21
Comments
We also ran into this issue, but if this library matches what jest does, then the resulting data shouldn't have changing data. https://facebook.github.io/jest/docs/en/snapshot-testing.html#tests-should-be-deterministic I guess that means either mocking the bits above or manually removing the auto-generated data before snapshotting it |
Want to start off by saying love the lib 👍 am a huge fan of snapshot testing and this powers pretty much all of our python integration tests. I agree around tests being deterministic, but just realistically our api's return db id's and I'd prefer not to mock out the whole db interaction bec it's kind of nice that it's captured by the snapshot test. our tests our deterministic outside of id generation from the db (sometimes things get committed/flushed for a bunch of reasons), so I don't think it necessarily makes sense to be overly dogmatic that a snapshot can't have an id field that's dynamic. we have a bunch of code that cleans up our api responses to test that id's are returning correct values so that we can run them through the snapshot test. it would be really nice if there was someway to handle these situations smoothly and the tests would handle type checking values that are dynamic. so for example it would be great if in the snapshot we could do something like the following: self.assertMatchSnapshot(res, dynamic=[("id", number)]) where dynamic is a list of tuples specifying the dynamic ids, and the expected instance that should be asserted to match. I understand if this is a bit beyond the scope of the lib, bec of all of the different workflow that can happen around dynamic fields. That being said it could get pretty hairy to support the various workflows of dynamic json generation - alternatively you could say you only handle simple use cases. There's a go lib called abide that handles this in a cool manner. @syrusakbary if we can agree on an appropriate api, I'd be happy to tackle and give back to the lib since it's been super helpful for us. |
@sdalezman I'm wondering if ignoring ids would actually be a good idea or not myself. I totally understand the reasons to ignore them, but at the same time, why not creating the db entries with a predefined id? In Django for example you can create models with a given id. Ids can also be useful to make assertions on the returned data, and so maybe they should be part of what's tested in the snapshot? |
I think @fabiosussetto is right, it is not concern of the lib to deal with dynamic data. My team use factory boy to make test data and, when using snapshot testing, we use to reset the sequence counter and avoid fuzzy data or faker. When using snapshot testing with Jest, we use Rosie.js to generate test data and also reset sequences during setup. Dynamic data outside of our hands, like any datetime field with auto_now_add, we just mock:
|
@fabiosussetto with regards to dates yea i think that's the right approach. with regards to ids if you're using auto-incrementing ids (which I expect a lot of users of various pg systems are and I've seen used pretty heavily) - then you have to do an extra assertion on each id and then delete from the resulting json. One of the things that I love about snapshot testing is it reduces the burden of writing any test pretty significantly. What I've noticed on teams is making writing tests easier to write always results in tests being written whereas anytime it provides a bit of extra work it becomes something easy to cut. So that's more the basis of my suggestion. That being said, I've implemented some code that wraps around the assert lib and checks ids for us, which is def a route someone can go down if they're using auto incrementing ids |
For information, Jest has a feature like that in its snapshot section : https://jestjs.io/docs/en/snapshot-testing#property-matchers |
Regarding ID generation, note that not everyone uses incrementing integers. Some of us use UUIDs. Yes, there are workarounds, but they are tedious. Those of us advocating for ignoring specific fields would like to avoid the tedium. |
This is a much needed feature. My team currently needs to delete certain ID's / dates from the response before passing it to assert_match. |
I think this is a practicality vs purity concern, and I'm in the practicality corner. Mocking everything that could be a timestamp, uuid or random value is not always practical but I won't stop anyone from doing so. Adding the possibility to explicitly ignore keys in generated snapshot would add a lot of value I think and not hurt anyone who wants to mock things. Idea for the API: ...
snapshot.assert_match(
some_rendered_data,
'some_rendered_data',
ignore_keys=("created_at", "id")
) The response and frozen version could be traversed and have those keys removed, but still be asserted to exist. And in essence this can be seen as a sort of mocking, but on a different level |
Any updates on this? This would be great to have |
Hm, this is actually a handy feature and the fact that it hasn't been added in 7 years means it's never going in. The thing is you don't always fully control the environment the snapshot happens in. If you do, it's obvious the IDs will always be the same, etc. But sometimes you don't and then... crickets. |
Please consider adding ignored fields that not necessarily need to match. It would be useful to ignore auto-generated fields like database ids and last_modified timestamps
The text was updated successfully, but these errors were encountered: