Refactor integration tests #1821
Conversation
server/service/testing_client.go
Outdated
| // an io.ReadCloser for new request body | ||
| type nopCloser struct { | ||
| io.Reader | ||
| } | ||
|
|
||
| func (nopCloser) Close() error { return nil } |
There was a problem hiding this comment.
I see you copied this from existing code, but let's use https://pkg.go.dev/io/ioutil#NopCloser
There was a problem hiding this comment.
Great catch. Btw, ioutil is deprecated, in this case we should use io.NopCloser.
Codecov Report
@@ Coverage Diff @@
## main #1821 +/- ##
==========================================
+ Coverage 48.05% 48.27% +0.22%
==========================================
Files 224 225 +1
Lines 21266 21375 +109
==========================================
+ Hits 10219 10319 +100
- Misses 9832 9840 +8
- Partials 1215 1216 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This looks really good, I think the tests are very readable, top-down, easy to understand what actions get executed and what behaviour is asserted.
Besides the couple minor things I left inline, one thing I'd question (and apologies if that had already been discussed and this ship has sailed) is the use of the testify/suite package vs tests with sub-tests. I think we can fully reproduce the setup/teardown without using a (likely less familiar) third-party for this:
- the suite-level setup/teardown is simply the code that runs in the top-level Test function and deferred execution (or registered via t.Cleanup)
- the common-to-all-tests test-level setup/teardown can be a simple setup function to be called on each test and that calls t.Cleanup to register the common teardown (also makes code more explicit vs a special naming convention)
- a "test suite" would basically be a top-level Test function composed of n sub-tests
- the suite can still be defined as a struct that holds state if needed, with methods being the subtests, if that makes the code clearer than closures defined in the top-level test
Anyway, not a big deal but I feel like we could make our tests a bit more familiar to contributors, while keeping the main benefits.
We haven't discussed it at length. The upside to test suites vs sub-tests is that we can reuse the same suite across multiple files. Which is also true for sub-tests if you define the test function separately, but it's not the pattern we've been following. Other than that, it's simply that we are already relying on testify, so this simply uses another part of it. I don't feel super strong in favor of either, to be honest. As long as the tests are readable, handle the evolution of the code gracefully, and are extensible easily... I don't care about the details. If you feel like these are not good enough arguments to go in this direction, I'm more than happy to switch to sub-tests. You have much more experience with them. It might be important that whatever we decide, though, we follow everywhere. What do you think? |
Yeah for assertions (assert/require), I think it helps keep the tests and if conditions to a minimum. It's the rest of the
Me neither, I'm not sure it's worth changing all the existing integration tests.
You mean even outside integration tests, we should also use the suites instead of sub-tests? My feeling would be more like it's not a big enough deal to rewrite what we have, and when adding new integration tests to existing suites, keep the same approach (or refactor to sub-tests if you can invest the effort), but new tests outside of the suites would use standard Go tests? |
No, not everything. Things that depend on third party services (eg mysql), the more clustered like this we can make them, the faster they will run. More like a recommendation than a rule though. |
|
Ok, I will likely tackle the mysql-related tests for the ticket on slow tests, and my plan was to do it with sub-tests, we can discuss then how we feel about those and this other approach? Basically, my hesitation is on using in a third-party package that I think might not bring enough value (even if it's in a repo we already use for other packages, we don't have to use that particular package). |
Sure! Let's see how that looks. |
There was a problem hiding this comment.
As mentioned in standup, I don't think the remaining question about using or not testify/suite should be a blocker, worst case we go back and change it to sub-tests, or we keep it around and address only when we change/add something, or we just go with it definitely. Tests are definitely looking good and super readable! 👍
Checklist for submitter
If some of the following don't apply, please write a short explanation why.
Changes file added (if needed)Documented any API changesAdded tests for all functionalityManual QA for all functionality