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

Feature - assert_match() with ignored keys #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HeyHugo
Copy link

@HeyHugo HeyHugo commented Oct 26, 2019

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:

We want to make it as frictionless as possible to write good tests that are useful. We observed that when engineers are provided with ready-to-use tools, they end up writing more tests, which in turn results in stable and healthy code bases.

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 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 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.

def test_snapshot_can_ignore_keys(snapshot):
snapshot.assert_match(
{
"id": uuid.uuid4(),
Copy link

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.

Copy link
Author

@HeyHugo HeyHugo Dec 4, 2019

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.

Copy link
Author

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 :)

snapshottest/module.py Outdated Show resolved Hide resolved
@povilasb
Copy link

povilasb commented Dec 3, 2019

I like it 👍
That's defo smth I could use.
Now I'm filtering the data using pydash btw:

    snapshot.assert_match(pydash.omit(data, ['id']))

snapshottest/module.py Outdated Show resolved Hide resolved
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.
@gurunars
Copy link

gurunars commented Feb 3, 2020

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: {key: value for key, value in my_dict.items() if key not in my_ignored_keys}

I strongly suggest not to introduce features that can be easily replicated via existing means.

Thus I wouldn't approve it.

@gurunars
Copy link

gurunars commented Feb 3, 2020

As for ignoring nested things, to align with a single responsibility principle, it is better to have a helper pure function such as pydash.omit mentioned above.

@HeyHugo
Copy link
Author

HeyHugo commented Feb 4, 2020

@gurunars

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.

  1. What about nested keys?
  2. You would not know if the C key was missing or not, leading to a passing but incorrect test.

In python it is done via a one liner: {key: value for key, value in my_dict.items() if key not in my_ignored_keys}

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.

@x3ro
Copy link

x3ro commented Jul 2, 2021

I've recently started using snapshottest because it's being recommended in the Grapehene documentation, and I really like it so far. I've also run into the problem that I would like to assert_match while ignoring certain sub-fields.

I'll admit that I haven't read all issues and PRs on the topic here, but I would like to address the following:

I strongly suggest not to introduce features that can be easily replicated via existing means.

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 snapshot

Omitting data before passing it to assert_match causes it to not be stored in the snapshot file, meaning that I'm now generating a potentially invalid response to be stored. I can no longer look at the snapshot file and say "ah yes, this is what a valid response looks like". It's also no longer possible to "un-ignore" a field at a later point in time without re-generating the snapshot.

It leads to less readable tests (in my opinion)

So let's assume I have a response variable which contains the API response as a dict.

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 assert_match is the preferable way, as this stores the full response and, at the same time, communicates the intent of the person writing the test.

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 🚀

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.

Please add ignored fields
6 participants