-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add ShouldJSONEqual assertion #676
Merged
yesnault
merged 8 commits into
ovh:master
from
philippgille:add-shouldjsonequal-assertion
Jul 19, 2023
Merged
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5615d62
Add ShouldJSONEqual assertion
philippgille 35060c7
Fix example in Godoc
philippgille 37c406a
Improve Godoc
philippgille dbb11f5
Add to test suite
philippgille ba3769c
Add ShouldJSONEqual to README
philippgille 5c05103
Support primitive JSON values
philippgille 4c68805
Merge branch 'master' into add-shouldjsonequal-assertion
philippgille 32871e2
Merge branch 'master' into add-shouldjsonequal-assertion
philippgille File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
name: test ShouldJSONEqual | ||
vars: | ||
json_expected: '{"a":1,"b":"foo"}' | ||
testcases: | ||
- name: test assertion | ||
steps: | ||
- type: exec | ||
script: "echo '{ \"b\" : \"foo\", \"a\" : 1 }'" | ||
assertions: | ||
- result.systemout ShouldJSONEqual "{{.json_expected}}" |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think that the parser to detect the json could be more json compliant. There is a check for
{
or[
, but a simple text or number is also a valid number, see all values on https://www.json.org/json-en.htmlThere 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.
You're right, an older spec restricted it to objects and arrays, but the latest spec doesn't: https://stackoverflow.com/questions/7487869/is-this-simple-string-considered-valid-json
It's a good point to be compliant to the latest spec, but I also wonder when someone writes an assertion for a string or bool, wouldn't it be preferable to use
ShouldEqual "foo"
,ShouldBeTrue
/ShouldBeFalse
etc to be more precise? This could be documented to make clear.What would you prefer:
ShouldJSONObjectEqual
+ShouldJSONArrayEqual
ShouldJSONArrayEqual foo bar baz
instead ofShouldJSONArrayEqual '["foo", "bar", "baz"]'
, although not sure if that brings much valueThere 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 prefer the first proposal: let the choice to the user and having the ShouldJSONEqual supporting the different types. Maybe not the easiest to implement, but definitely the easiest for users to understand if we say that ShouldJSONEqual supports the full JSON spec.
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.
Makes sense 👍
Done in 5c05103
null
are tricky, because Venom passes it as empty string and not Go'snil
. Haven't checked if this is a fundamental limitation in the upstream callers in Venom, or if it can be changed. Do you know maybe?And added to README in ba3769c
Looking at the assertion list in the README it's a bit messy, I think it would make sense to sort the assertions alphabetically. But then the
ShouldJSONEqual
feels misplaced. => WDYT about naming itShouldEqualJSON
like originally proposed in #633, and then for consistency createShouldEqualTime
as an alias forShouldTimeEqual
, which is required for backward compatibility. The latter could be marked as deprecated in the README.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.
About naming,
ShouldEqualJSON
is ok too, perhaps we can add some alias in another pr than here if needed. Thank you for adding different json values.LGTM