Enforce all code dirs have tests#453
Conversation
In order to get accurate coverage results we attempt to always have a _test.go file in directories with code. Add a test to enfoce this in CI and add stub_tests.go to dirs which fail this test.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: greghaynes If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow-robot
added
the
size/L
n3wscott
left a comment
n3wscott
left a comment
There was a problem hiding this comment.
the majority of the directories added will not have code in them, or they are exec package main places that are hard to unit test
|
|
||
| import "testing" | ||
|
|
||
| func TestStub(t *testing.T) { |
There was a problem hiding this comment.
cmd dirs don't tend to have tests.
| limitations under the License. | ||
| */ | ||
|
|
||
| package logkey |
There was a problem hiding this comment.
there is no code in logkey dir
| limitations under the License. | ||
| */ | ||
|
|
||
| package eventing |
| limitations under the License. | ||
| */ | ||
|
|
||
| package logkey |
| limitations under the License. | ||
| */ | ||
|
|
||
| package main |
There was a problem hiding this comment.
this has the same rules with cmd, not sure it is testable
| limitations under the License. | ||
| */ | ||
|
|
||
| package main |
There was a problem hiding this comment.
question adding stub for package main
| limitations under the License. | ||
| */ | ||
|
|
||
| package testing |
There was a problem hiding this comment.
testing inception? :)
I can add this to the skip list.
|
Theres a couple classes of untested things here - cmd/things which are difficult to test: I don't see much of a downside to having these in the coverage reports. I agree these can be a pain to test and the cost/benefit might not be there but there also shouldn't be harm in keeping them as part of the coverage reports. If they start to be a big drain on coverage amounts that's probably a good signal that we should move more things out of cmd / find a way to test parts of it. dirs which only have a register.go / constants defined (e.g. pkg/apis/eventing/): I agree its not particularly useful to assert we have tests defined for a dir containing only constants. I could add a skip for dirs which only contain a register.go (if thats a strong enough convention) or we could manually add these dirs to the skip grep. If we want to get really fancy maybe we can add some comment metadata which to these pure-constants files which tell the check to ignore them :). |
|
Oh! Neat, I missed that there was a magic script that is making these. Cool!! |
|
In that case it might also need to get hooked up to one of the hack scripts, perhaps the presubmit if we have one |
|
Yep, this script is just checking these files exist but has some support for overrides. I added it to presubmit but im not sure if we have to add it anywhere else for the CI jobs to pick it up? |
| # reports resulting in inaccurately high coverage results. Fix by adding a | ||
| # stub_tests.go with no tests to these directories. | ||
| # This function namespaced with eventing to allow adding to test-infra without name collision | ||
| function check_eventing_subdirs_without_tests() { |
There was a problem hiding this comment.
I think this kind of check is too hard. You're checking all directories and failing the build test job if there are directories without tests, even if they're unrelated to the current PR. This, at least, add a burden to PRs unrelated to these empty directories. Also, you're now enforcing a code coverage blocker, and as such I think it should be (1) agreed by the leads (instead of being just decided in a PR) and (2) only imposed after knative/eventing is in a better coverage state (so the burden is lower).
There was a problem hiding this comment.
By the way, if the goal is just to get accurate test coverage and the number is wrong because of missing test files, the code coverage tool should be responsible for dealing that, not the repo or the developers. @steuhs is the right point of contact about the coverage tool.
There was a problem hiding this comment.
The flow is fix repo to pass and add this assertion in one PR, you are then guarded against failing this test from that point on in future PR's. This aspect isn't really different than any other test.
I think its less the tool and more that you need to make a distinction between intention. Cover works by being run on a directory and (correctly) reports unknown whenever there is no coverage in a dir. We need to then distinguish between subdirs such as pkg/client where we intentionally lack coverage (not our code) and dirs where we simply haven't provided any.
There was a problem hiding this comment.
Makes sense. Please merge knative/test-infra#134 first and update the vendored test-infra to use the helper method.
|
It's been a while since the coverage job was updated to correctly handle directories without tests, so this PR can be closed. It's been stale for 6 months. |
…tion references (knative#7455) (knative#453) * Allow configuring disallow cross namespaces Brokers configuration ref Instead of always allowing to specify cross namespace configuration references for Broker allow users to configure whether to disallow such references as it might be problematic in multi tenant environments. * Codegen --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
4 participants
Proposed Changes
_test.go file in directories with code. Add a test to enfoce this in CI
and add stub_tests.go to dirs which fail this test.
Release Note