-
Notifications
You must be signed in to change notification settings - Fork 113
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
jsonschema: Test cleanup and bugfix #64
Conversation
I should mention, this change makes all the tests go1.7+, as I see the CI no longer runs Go 1.6 |
FYI: #51 will aim to move encoder logic into {typeName}.go as well, which will match nicely with 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.
Please don't merge yet...
// | "required": [ | ||
// | | "foo" | ||
// | ] | ||
// } |
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 output does not always seem to match! Please don't merge until fixed.
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 looks to be a side effect of us doing the JSON serialization by ourselves! The built-in JSON encoder sort map keys lexicographically (or so I read here).
We however loops through the schema Fields
with no sorting of keys.
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.
Fixed
24c72fc
to
61be337
Compare
Ready from my point of view now |
for k := range v { | ||
keys[i] = k | ||
i++ | ||
} |
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 it is more idiomatic (and equivalent perf-wise) to do:
keys := make([]string, 0, len(v))
for k := range v {
keys = append(keys, k)
}
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, I will change it shortly.
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.
Done
Testing: - Replaces encoder_test.go (encoding package) with example_test.go (jsonschema package). - Replaces remaining tests in jsonschema_test.go with tests that test the pulbic interface. The new tests are placed in {typeName}_test.go or all_test.go. - Remove go 1.6 testing support (no longer run by CI). Code: - Solves Issue rs#63 (found by test change) where errors from validatorToJSONSchema and serializeField where never propagated. - Encodes schema.Fields in alphabetical order to allow example test. - Order type methods to appear right below type definition.
61be337
to
848b5a3
Compare
testing the pulbic interface. The new tests are placed in
{typeName}_test.go or all_test.go.
validatorToJSONSchema and serializeField where never propagated.