Guideline or checklist on writing good (unit) tests #249
Conversation
Current coverage is 20.13% (diff: 100%)@@ master #249 diff @@
==========================================
Files 32 32
Lines 1167 1167
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 235 235
Misses 888 888
Partials 44 44
|
kwk
changed the title
|
|
||
| === Blackbox vs. Whitebox tests | ||
|
|
||
| In a blackbox test, you test a package as if you **only have access to the |
There was a problem hiding this comment.
I don't really see what we gain by separating blackbox from whitebox testing. Anything that isn't reachable by public API is dead code anyway and does not have to be tested. Sometimes we can write test more easily by calling internal code, but in the end, it's all about coverage, no?
There was a problem hiding this comment.
It is about coverage and also about usage help I'd say. In principle somebody who would like to know how to use a package should take a look at the blackbox tests because this is the interface that one should be using. So separating the whitebox tests helps to bring some clarity for the reader.
There was a problem hiding this comment.
If you think of packages and their exported symbols as a contract, then the more whitebox tests we have the less strict is the contract of those packages.
|
|
||
| In Go your test filenames must end with `_test.go`. In order to distinguish black- | ||
| and whitebox test files, a recommendation is to use the suffixes `_blackbox_test.go` | ||
| and `_whitebox_test.go`. |
There was a problem hiding this comment.
This makes for long filenames. As @baijum (I think) suggested, we could have whitebox as the default and call blackbox tests "_bb_test.go". If we establish this as a pattern, people will know what it means.
There was a problem hiding this comment.
I'd advocate to make the default a blackbox test because that is the test with the most value. Testing some internal functionality is also important of course. So I'd say package_test.go is the blackbox test and package_whitebox_test.go is the whitebox one. I don't care about the lengths of filenames at all and the naming pattern should be obvious. With whitebox_test the meaning is clear and except for it's length there's nothing to argue about. But wb_test could mean anything and requires explanation.
| <6> Marking the test for the `Shout` function test as: "executable in parallel". | ||
|
|
||
| A few things to notice in <<lib-blackbox-test-dot-go-source>> is the package name: `bar_test`. | ||
| This is an exception to the Go compiler because normally only package names with the same name are |
There was a problem hiding this comment.
I did not know that. To be honest: special casing this in the compiler to allow for cross-package tests seems like the wrong fix to me. But if this is the way it's meant to be, let's not try to swim against the stream.
There was a problem hiding this comment.
testing.T..Parallel() Has nothing to do with optimizing the compiler.
There was a problem hiding this comment.
This is about the _test package name, not t.Parallel()
| total: (statements) 100.0% | ||
| ---- | ||
| <1> Runs all the tests in the `foo/bar` package and collects coverage information in the `cov.out` file. | ||
| <2> Prints coverage information per function in the package. |
There was a problem hiding this comment.
As discussed in chat, the coverage numbers for a package or function we generate from our makefile do not include coverage from test located in a different package. This will ignore coverage from end-to-end and cross-package integration tests.
As a group, we need to decide if that is a problem and needs to be fixed. There is a golang bug against this: golang/go#6909 Later comments include pointers to tools that work around this.
There was a problem hiding this comment.
A package should contain the tests it needs to be verified correct. And talking test coverage, it should be said that when you run go test -coverprofile=cov.out github.com/almighty/almighty-core/models you only want the models package to be tested and print a coverage for that. I'd say, that we should at least not be blocked by this and accept it the way it is right now.
| The only reason why we still don't use the `-short` parameter is that we want to | ||
| have the ability to define more test-skipping criteria than just the time | ||
| consumed by a test. | ||
| That is why we have the `resource.require(t, resource.Database)` syntax to |
There was a problem hiding this comment.
It would be helpful to explain the "require" concept a bit more and how to use it in tests. It's not entirely clear to me.
There was a problem hiding this comment.
Sure we can do that. Feel free to look at the code. It is easy to understand and I've added more documentation in my other PR: https://github.com/almighty/almighty-core/blob/629202a7431b665de15705f7e9f0688d15c4218b/resource/require.go
There was a problem hiding this comment.
I'd add require to the test package where it should belong. But that can be done afterwards.
| that have been marked to be executable in parallel. In this example, none of our tests | ||
| has any side effects and so they can be executed in parallel. | ||
| The number of parallel executable tests can be overwritten by the `-parallel` switch. | ||
|
|
There was a problem hiding this comment.
Since tests on this level should be independent, can we switch to parallel execution by default, i.e. we would need to set serial execution explicitly in special cases.
Let's handle this in a while. I need to revise the PR. |
Hi,
while I was working on improving the coverage for some code parts, I discovered some nice things about testing in Go in particular. Those things may be fundamental to others but weren't for me when I started. Therefore, I created a document to write down my findings.
I'm going to update this document during the work I do now but I'd be happy for any comments or feedback if we want to have such a document in general.
It can be a living document for which we accept many PRs.
Unlike @sanbornsen 's email based on ALR's talk on testing, my document so far targets Go in particular. But we can incorporate the email here as well.
Regards.