-
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
Feature - assert_match() with ignored keys #102
base: master
Are you sure you want to change the base?
Conversation
c6ecc69
to
4eb8d44
Compare
def test_snapshot_can_ignore_keys(snapshot): | ||
snapshot.assert_match( | ||
{ | ||
"id": uuid.uuid4(), |
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.
Given that single quotes are used for strings in other places, I'd stick to that.
See https://pep8.org/#string-quotes
In Python, single-quoted strings and double-quoted strings are the same. This PEP does not make a recommendation for this. Pick a rule and stick to it.
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.
Ah I've gotten so used to default black formatting so didn't notice. Agree this should be consistent, will change.
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.
Hm looking closer at the code I can see both double quotes and single quotes are used at this point in the project.
This is a problem in itself and should be addressed in a separate PR imo. Preferably with black formatting if you ask me since I think formatting beats linting. And at the same time linting this should be added to enforce it. (black --check
if black i chosen)
I don't really care if it's single or double quote just consistent :)
I like it 👍
|
60eb300
to
1b057db
Compare
Enable setting ignore_keys argument on snapshot.assert_match() ignore_keys is a list or tuple of keys whose values should be ignored when running assert_match. The values of the ignore_keys are set to None, this way we still assert that the keys are present but ignore its value. Dicts, lists and tuples are traversed recursively to ignore any occurrence of the key for all dicts on any level.
1b057db
to
4701f8e
Compare
As alluring as it sounds, I have to say that this feature doesn't look reasonable. If you happen to have an API that provides a payload with keys A, B and C and value C is the value you want to drop because it is e.g. a randomly generated stuff, just drop it from the payload that you pass to the matcher. In python it is done via a one liner: I strongly suggest not to introduce features that can be easily replicated via existing means. Thus I wouldn't approve it. |
As for ignoring nested things, to align with a single responsibility principle, it is better to have a helper pure function such as |
Sure it's perfectly possible to either remove those keys by hand or use mocks. But like my motivation for this feature says. Doing any of that is more work. This feature is meant to reduce friction from using snapshottest. If this library is not sufficient on it's own to create tests without much hassle, but expects users to add extra dependencies (pydash) or require more boilerplate I think it makes it less useful. In fact I think the main purpose of this package is to reduce the time it takes to create tests that freeze apis. From looking at the other PRs and issues regarding this (#21 #105 #68 #32) it seems I'm not alone in thinking this feature would be valuable. But each to their own, if you don't use it, it won't affect you in any noticeable way. |
I've recently started using I'll admit that I haven't read all issues and PRs on the topic here, but I would like to address the following:
It's true that it's easily replicated, and that's what I ended up doing. However, going down this road, in my opinion, has two major downsides: It modifies the state that is stored in the snapshotOmitting data before passing it to It leads to less readable tests (in my opinion)So let's assume I have a This works, but it generates an odd-looking snapshot and is not obvious unless I'm already familiar with the problem. response['data']['createFoobar']['foobar']['id'] = ""
snapshot.assert_match(response) Less odd-looking snapshot, but still raises the question what ID this is and where it's coming from. response['data']['createFoobar']['foobar']['id'] = "d8599b59-52ae-4e23-b37d-39e6083ff211"
snapshot.assert_match(response) This works, of course, but it's pretty verbose if you're doing it often. # Forcing the returned UUID from the API to be stable so that the snapshot doesn't fail
response['data']['createFoobar']['foobar']['id'] = "d8599b59-52ae-4e23-b37d-39e6083ff211"
snapshot.assert_match(response) This is okay from the readability perspective, but stores a malformed response: snapshot.assert_match(pydash.omit(response, ['data.createFoobar.foobar.id'])) So, from my perspective, ignoring during the snapshot.assert_match_with_ignore(response, ignore_keys=["data.create.foobar.id"]) That being said, thanks to the maintainers and contributors for creating this very useful library 🚀 |
Motivation
The motivation behind this feature is to enable a more simple and practical workflow. Mocking any random values is of course viable, but usually more work. Also this feature aligns well with the philosophy already stated in the readme:
If you still prefer to mock your random or time dependent data no change in the PR will affect you. Fixes #21
How it works
Enable setting
ignore_keys
keyword argument onsnapshot.assert_match()
ignore_keys
is a list or tuple of keys whose values should be ignored when running assert_match.The values of the ignored keys are set to
None
. This way we still assert that the keys are present but ignore its value. Dicts, lists and tuples are traversed recursively to ignore any occurrence of the key for all dicts on any level.I know there is another PR #32 with a much similar feature, but it has slipped behind master and it doesn't ensure the keys are still present when comparing with the snapshot.