ast: preserve jsonOptions in NewAnnotationsRef
#5753
Conversation
|
Thanks for this @zregvart! This is makes sense to me. I'm interested to know how you spotted this and if it caused any issues for your use case? |
|
Seconded! Also, could a test be added to assert desired functionality here? |
|
We render our documentation using Antora, for which we built an extension that relies on opa-inspect-js Nodejs package to provide the Location in the I thought about creating a test for this change, the challenge with that is that it would be of little value as it would test only this specific scenario. What I think would help in terms of testing is to create a property based test |
|
Thanks for the notes @zregvart, really interesting to see how you're using the tooling to create rich documentation - it's great! Regarding the testing, I think you're right in that ideally we'd cover more than this specific case. However, since OPA is often included in applications as a dependency, we generally try to keep the dependencies we have in OPA to a minimum. I wonder if there's a compromise that tests more than this single case, but doesn't need a property based testing dependency? Perhaps using |
ast/annotations_test.go
Outdated
| @@ -1095,6 +1097,319 @@ func TestAnnotations_toObject(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestNewAnnotationsSet_WithOptions(t *testing.T) { | |||
There was a problem hiding this comment.
I'm not sure about this test name, how about TestNewAnnotationsRef_JSONOptions?
Also, I think that this test might be best located in ast/marshal_test.go. We added some pretty similar tests in there and I think it'd make sense for this to be located with them.
ast/annotations_test.go
Outdated
|
|
||
| asJSON := toJSON(ref) | ||
|
|
||
| assert.JSONEq(t, tc.expected[i], asJSON, "serialized AnnotationsRef differs, expected:\n%s\n got\n%s\n", tc.expected[i], asJSON) |
There was a problem hiding this comment.
OPA's depending on github.com/stretchr/testify is, unfortunately, rather misleading. I will try and explain why we don't use it for such assertions.
The package is in our go.mod since we vendored and altered gojsonschema. This code came with some tests which used testify. This means that the only uses of testify are in this internal package. Ideally these tests wouldn't introduce additional dependencies to OPA.
Sorry that this likely isn't at all clear. I've added it to my todos as I'd like to not mislead future contributors in this way.
In our tests, we generally compare the JSON as strings, e.g. ast/marshal_test.go. Do you think you'd be able to do that here too?
Makes sure that `jsonOptions` are preserved in NewAnnotationsRef. Signed-off-by: Zoran Regvart <zoran@regvart.com>
|
@charlieegan3 fancy having another look? |
There was a problem hiding this comment.
Thanks @zregvart, I think this looks good now 🙂 thanks for making these adjustments so quickly.
charlieegan3
changed the title
jsonOptions in NewAnnotationsRefjsonOptions in NewAnnotationsRef
We have testify in our go.mod. This sometimes misleads contributors (including myself!) to think that we can use testify in test code. Since testify is a common testing package, many go developers default to using it. I'm not sure if we want to merge this as we might rather leave these vendored packages untouched, but it was 10 minutes of work and I was interested to see if we can avoid issues like this in future: * #5753 (comment) * #5447 (comment) * #3952 (review) Signed-off-by: Charlie Egan <charlie@styra.com>
Makes sure that
jsonOptionsare preserved in NewAnnotationsRef.