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

Add ShouldJSONEqual assertion #676

Merged
merged 8 commits into from
Jul 19, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 115 additions & 1 deletion assertions/assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ var assertMap = map[string]AssertFunc{
"ShouldHappenOnOrAfter": ShouldHappenOnOrAfter,
"ShouldHappenBetween": ShouldHappenBetween,
"ShouldTimeEqual": ShouldTimeEqual,
"ShouldJSONEqual": ShouldJSONEqual,
"ShouldBeArray": ShouldBeArray,
"ShouldBeMap": ShouldBeMap,
"ShouldMatchRegex": ShouldMatchRegex,
Expand Down Expand Up @@ -770,7 +771,6 @@ func ShouldHaveLength(actual interface{}, expected ...interface{}) error {
}

return fmt.Errorf("expected '%v' have length of %d but it wasn't (%d)", actual, length, actualLength)

}

// ShouldStartWith receives exactly 2 string parameters and ensures that the first starts with the second.
Expand Down Expand Up @@ -1174,6 +1174,120 @@ func ShouldTimeEqual(actual interface{}, expected ...interface{}) error {
return fmt.Errorf("expected '%v' to be time equals to '%v' ", actualTime, expectedTime)
}

// ShouldJSONEqual receives exactly 2 JSON arguments and does a JSON equality check.
// This means the keys can be in different order, and whitespace (except in keys or values) is ignored.
// JSON can be an object or array (i.e. start with `{` or `[`).
//
// Example of testsuite file:
//
// 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}}"
func ShouldJSONEqual(actual interface{}, expected ...interface{}) error {
if err := need(1, expected); err != nil {
return err
}

expectedString, err := cast.ToStringE(expected[0])
if err != nil {
return err
}

// `actual` can be a map, slice or the string representation of a JSON object or array.
var actualMap map[string]interface{}
var actualSlice []interface{}
var obj bool
switch actual.(type) {
case map[string]interface{}:
Copy link
Member

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

Copy link
Contributor Author

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:

  1. Allow any of the JSON types here, leave users the choice between ShouldEqual/ShouldBeTrue etc and ShouldJSONEqual
  2. Allow only objects and arrays and clearly document this in the README (the assertion list)
    • ⚠️ Which makes me realize I didn't add the new keyword to that list yet, I'll do that when you decided on one of the options
  3. Change the name to even split between object/array to be precise, which also makes the implementation simpler / more concise: ShouldJSONObjectEqual + ShouldJSONArrayEqual
    • One benefit could be to even allow an actual list as param to check against, like ShouldJSONArrayEqual foo bar baz instead of ShouldJSONArrayEqual '["foo", "bar", "baz"]', although not sure if that brings much value

Copy link
Member

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.

Copy link
Contributor Author

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

⚠️ JSON null are tricky, because Venom passes it as empty string and not Go's nil. 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 it ShouldEqualJSON like originally proposed in #633, and then for consistency create ShouldEqualTime as an alias for ShouldTimeEqual, which is required for backward compatibility. The latter could be marked as deprecated in the README.

Copy link
Member

@yesnault yesnault Jun 23, 2023

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

obj = true
var err error
actualMap, err = cast.ToStringMapE(actual)
if err != nil {
return err
}
// Marshal and unmarshal for later deepequal to work
actualBytes, err := json.Marshal(actualMap)
if err != nil {
return err
}
err = json.Unmarshal(actualBytes, &actualMap)
if err != nil {
return err
}
case []interface{}:
obj = false
var err error
actualSlice, err = cast.ToSliceE(actual)
if err != nil {
return err
}
// Marshal and unmarshal for later deepequal to work
actualBytes, err := json.Marshal(actualSlice)
if err != nil {
return err
}
err = json.Unmarshal(actualBytes, &actualSlice)
if err != nil {
return err
}
case string:
actualString, err := cast.ToStringE(actual)
if err != nil {
return err
}

// JSON can start with { or [
if strings.TrimSpace(actualString)[0] == '{' {
obj = true
actualMap = map[string]interface{}{}
err = json.Unmarshal([]byte(actualString), &actualMap)
if err != nil {
return err
}
} else if strings.TrimSpace(actualString)[0] == '[' {
obj = false
actualSlice = []interface{}{}
err = json.Unmarshal([]byte(actualString), &actualSlice)
if err != nil {
return err
}
} else {
return fmt.Errorf("actual string must represent a JSON object or array, starting with '{' or '['")
}
default:
return fmt.Errorf("unexpected type for actual: %T", actual)
}

if obj {
expectedMap := map[string]interface{}{}
err = json.Unmarshal([]byte(expectedString), &expectedMap)
if err != nil {
return err
}
if reflect.DeepEqual(actualMap, expectedMap) {
return nil
}
return fmt.Errorf("expected '%#v' to be JSON equals to '%#v' ", actualMap, expectedMap)
} else {
expectedSlice := []interface{}{}
err = json.Unmarshal([]byte(expectedString), &expectedSlice)
if err != nil {
return err
}
if reflect.DeepEqual(actualSlice, expectedSlice) {
return nil
}
return fmt.Errorf("expected '%#v' to be JSON equals to '%#v' ", actualSlice, expectedSlice)
}
}

func getTimeFromString(in interface{}) (time.Time, error) {
if t, isTime := in.(time.Time); isTime {
return t, nil
Expand Down
140 changes: 140 additions & 0 deletions assertions/assertions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1458,3 +1458,143 @@ func TestShouldMatchRegex(t *testing.T) {
})
}
}

func TestShouldJSONEqual(t *testing.T) {
type args struct {
actual interface{}
expected []interface{}
}

tests := []struct {
name string
args args
wantErr bool
}{
{
name: "ok",
args: args{
actual: `{"a":1,"b":"foo"}`,
expected: []interface{}{`{"a":1,"b":"foo"}`},
},
},
{
// Same with map
name: "ok",
args: args{
actual: map[string]interface{}{"a": 1, "b": "foo"},
expected: []interface{}{`{"a":1,"b":"foo"}`},
},
},
{
// Spaces, newlines, tabs and key order don't matter
name: "ok",
args: args{
actual: `{"a":1,"b":"foo"}`,
expected: []interface{}{` { "b" : "foo" ,` + "\n\t" + ` "a" : 1 } `},
},
},
{
// Order in nested objects doesn't matter
name: "ok",
args: args{
actual: `{"a":{"nested1":1,"nested2":2}}`,
expected: []interface{}{`{"a":{"nested2":2,"nested1":1}}`},
},
},
{
// An array instead of object is valid JSON too
name: "ok",
args: args{
actual: `[1,2]`,
expected: []interface{}{`[1,2]`},
},
},
{
// Same with slice
name: "ok",
args: args{
actual: []interface{}{1, 2},
expected: []interface{}{`[1,2]`},
},
},
{
// Bad value
name: "ko",
args: args{
actual: `{"a":1}`,
expected: []interface{}{`{"a":2}`},
},
wantErr: true,
},
{
// Bad type
name: "ko",
args: args{
actual: `{"a":1}`,
expected: []interface{}{`{"a":"1"}`},
},
wantErr: true,
},
{
// Missing key
name: "ko",
args: args{
actual: `{"a":1,"b":"foo"}`,
expected: []interface{}{`{"a":1}`},
},
wantErr: true,
},
{
// Wrong array order
name: "ko",
args: args{
actual: `{"a":[1,2]}`,
expected: []interface{}{`{"a":[2,1]}`},
},
wantErr: true,
},
{
// Object instead of array
name: "ko",
args: args{
actual: `{"a":1}`,
expected: []interface{}{`[1]`},
},
wantErr: true,
},
{
// Same with map
name: "ko",
args: args{
actual: map[string]interface{}{"a": 1},
expected: []interface{}{`[1]`},
},
wantErr: true,
},
{
// Array instead of object
name: "ko",
args: args{
actual: `[1]`,
expected: []interface{}{`{"a":1}}`},
},
wantErr: true,
},
{
// Same with slice
name: "ko",
args: args{
actual: []interface{}{1},
expected: []interface{}{`{"a":1}}`},
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := ShouldJSONEqual(tt.args.actual, tt.args.expected...); (err != nil) != tt.wantErr {
t.Errorf("ShouldJSONEqual() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
10 changes: 10 additions & 0 deletions tests/assertions/ShouldJSONEqual.yml
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}}"