Improvement: Reduce Boilerplate in Tests

[richardrdev]

Okay, this is just a proof of concept, not ready to merge. Wanted to see what the response is to this before continuing.

Instead of the current method of using validateFunc to compare response bodies, I've come up with a new process.

First, in the test function, you define a custom struct that represents the expected structure of the response JSON, and then create an expectedBody value of this type:

type TestApiExpectedBody struct {
Title  string
Detail string
Status int
}

expectedBody := TestApiExpectedBody{Title: "Not Found", Detail: "Cannot GET /v1/i-dont-exist", Status: 404}

And you place this expectedBody into a TestCase instance, as an interface.

In the test runner function, you mostly do the same thing as runTestCases: iterate through the tests, run the query, get the response, compare status code and content type, etc. All that would stay the same.

But then, instead of using validateFunc to compare the expectedBodies, instead I did this:

• Use reflection to get the type of expectedBody
• Create an empty variable of the same type as expectedBody
• unmarshal the response JSON into the empty variable.
• then use an interface comparison function to compare expectedBody and the unmarshalled response json data

Using TestApi as a proof-of-concept, this seems to be working. I checked and it seems that the == operator is able to perform deep comparisons of interfaces, even if the values are complex nested structs.

Some of the reflection code is a little hard to read, and I'm not sure this is the most "golang" way to do this, but it does work. And I think this would massively reduce boilerplate in the actual test functions, and would make writing new tests or adding test cases much easier.

Please let me know what you think of this approach, if it seems like a good solution I'll try rewriting the first few cases of publishers endpoints and see how that looks.

[bfabio]

@richardrdev thanks for your work!

It's not quite what I had in mind. It's definitely an improvement, but there's still quite a lot of boilerplate because we'd need to define a type for each kind of response and instantiate it. It's kinda clean for application/problem+json, but we have other types (a lot?) of responses.

What I'd like to see is maybe a new expectedJSON field in TestCase, so that we'd use:

• expectedJSON: '{a: 1, b: 2}' to match equivalent JSON (matching {b: 2, a: 1} as well), which is the no frills test we want by default
• expectedBody: '{"this": "must", "be": "this", "exact": "raw", "json": 1}' and use it for stricter tests where we want to test for field orders (if there will be any, I don't think we do it explicitly somewhere right now)
• validateFunc for the rare custom tests

I looked around and looks like testify, which we are already using, has JSONEq. I think it's exactly what we need to implement expectedJSON.

#paranoid mode tl;dr: security wise I think it's all good

There's a potential (not sure about it though) where equivalent JSON, depending on how JSONEq is implemented, parses JSON differently than we do and maybe some potential JSON smuggling of invalid / unexpected values.

I don't think it's a problem because we're in control of the responses. Also, the client would smuggle itself, not the server. #paranoid mode off

[bfabio]

Just adding that when checking for equality, createdAt and updatedAt will be a problem. Now we just check they're valid RFC3339 strings, but that's suboptimal.

Maybe we can mock those values and check for equality.