-
Notifications
You must be signed in to change notification settings - Fork 56
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
Changed snapshot assert to field check #1911
Conversation
# Remove auditlog_level field | ||
row.pop("auditlog_level", "OFF") |
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.
How about creating a minimal set of required fields for this snapshot to work and discard all others.
Another option is to create a tuple of fields to discard.
With first case we won't be notice if any new field is added - only removed ones will trigger test to fail
With second we will need to update each time a new field is added.
Finally, do we really need to test for payload in audit log? Do we control the content?
Maybe we could only check if response exists and/or response status?
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.
Changed
3441036
to
e1be2d2
Compare
row.pop("metric_level", None) | ||
assert actual.json == snapshot | ||
assert len(actual.json) == 2 | ||
_assert_list_of_dicts_contains_key_with_value(actual.json, "patch", 0) |
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.
This is significantly weakening the assertion. Since the JSON output is part of the contract, I'd prefer checking that the actual dict is a superset of the expected one instead. Do you want to make the change or would you prefer that we do 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.
I didn't know which fields should I include, treat this as a proposition. Would be great if you could do this 🙏 .
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.
ok feel free to close this, I'll submit a PR soon.
Replaced with #1918 |
Pre-review checklist
Changes description
Changed snapshot assert to field check.