[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

I think prow makes the test output confusing here. Taking an example:

internal/apis/acme/install/pruning_test.go is marked as an integration_test in this PR. This tag is specified in the relevant BUILD.bazel, so bazel should run the test.

Bazel does run the test locally with bazel test //internal/apis/acme/install/..., bazel test //internal... and bazel test //....

Yet in the prow overview of the cert-manager-bazel test this test isn't shown. It is shown for a different overview for a different PR entirely.

In the logs for this PR, the test is shown as running.

So the test is run, but isn't reported in the JUnit section on the prow UI.

I'm going to push to this PR with the following patch to confirm that the pull-cert-manager-bazel test fails as expected.

diff --git a/internal/apis/acme/install/pruning_test.go b/internal/apis/acme/install/pruning_test.go
index 32d1a1984..b0c493857 100644
--- a/internal/apis/acme/install/pruning_test.go
+++ b/internal/apis/acme/install/pruning_test.go
@@ -31,4 +31,5 @@ import (
 func TestPruneTypes(t *testing.T) {
 	crdfuzz.SchemaFuzzTestForCRDWithPath(t, api.Scheme, apitesting.PathForCRD(t, "orders"), acmefuzzer.Funcs)
 	crdfuzz.SchemaFuzzTestForCRDWithPath(t, api.Scheme, apitesting.PathForCRD(t, "challenges"), acmefuzzer.Funcs)
+	panic("blork")
 }

If the bazel tests are run but not reported by prow, that seems acceptable to me given the bazel tests are on their deathbed. Alternatively it might be an easy fix to make them appear in the prow overview.

As expected, the test failure isn't shown in the prow overview (which reports 91/91 tests passed) but the test was run, which is visible at the bottom of the logs:

//internal/apis/acme/install:go_default_test                             FAILED in 0.1s

I'm fine with this, personally. I'll remove the test commit and remove the hold.

/test all

Thanks for all the great work on the make migration! Also, awesome PR description & comments ❤️

This PR adds go:build tags to every test I can find which has an external dependency - i.e. one which breaks or fails on a clean checkout of the repo when running go test ./..., or else everything under test/integration and test/e2e. The e2e tests are tagged differently.
This will enable us to use go test for all types of tests. Currently go test will panic due to missing dependencies if run without bazel magic happening.

I am not sure what is the general preferred approach in Go community about whether go test should run all tests in a repo or not, but from a more abstract point of view, I think it would be preferable if it would fall back on running all tests as opposed to running just some, to avoid cases where tests are not run due to forgetting to pass/misspelling a tag etc. I think that in general it is preferable to make engineers put in effort to skip a test (i.e figure out what tag to pass when running locally) as opposed to put in effort to ensure that the test they wrote actually runs because if they fail to do the latter, the effect is worse.

In this case, the obvious risk seems to be forgetting to add a tag to a Bazel rule and this will go away once we remove Bazel. But I would argue that it may be preferable to have the same approach and fall back to running all tests unless explicitly specified otherwise. At the moment we already have a mix of approaches where, for example, Venafi tests are skipped in most CI tests by passing a flag from test config, but there is also some code within the tests themselves to skip if a certain env var is not found (which introduces a risk as we could change the test pod config and accidentally remove the env var + it is not obvious that they won't be run for a person running them locally).

I had a brief look at alternative approaches and saw that it is possible to negate the build tag (i.e // +build !short and then skip with go test ./... --tags=short - I have not tested this). Also found this short article that talks about the same issue and introduces a less elegant, but imo still preferable approach using env vars.

Apologies, if some of this was already discussed earlier!

I agree with what you're saying, broadly speaking. I'd love to be in a place where go test just runs everything [1] and I want to clean up all tests so it's really obvious what's being run or skipped

My main concern ATM is that running go test on a fresh clone of the cert-manager repo will panic and it just doesn't feel good... if I switch to using t.Skip that's gonna touch so much more code.

Plus as far as I know, build tags are the only approach that works everywhere because of pkg/util/versionchecker/versionchecker_test.go. Without prior setup it won't even compile properly. Admittedly though, that's the only test with that issue.

My view is pretty much:

1. Before this PR, go test ./... is broken and to run all tests you need an external tool (bazel)
2. After this PR, go test ./... works but isn't ideal, and to run all tests you need an external tool (bazel, plus make will be an option soon)

[1] probably except the e2e tests

Discussed this in standup; consensus was to move all integration tests into test/integration, treat the versionchecker test as a special case (which might include a build tag), leave go test ./... broken but by moving all tests into test/, we'll allow go test ./pkg/... and go test ./internal/... to work as expected.

Ah, I missed this conversation previously! 😄

to avoid cases where tests are not run due to forgetting to pass/misspelling a tag etc.

I think this is an issue with both approaches though - moving into test/integration as well as marking things with a build tag. No matter what, you have to execute at least 2 commands to get all tests to run: 1) go test ./... and 2) make integration. The only difference is what that make integration runs (i.e. go build -tag ... vs go test ./test/integration/....

The drawback of not using tags is that go test ./... doesn't work (you have to explicitly say go test ./pkg/... ./cmd/... etc). For that reason, I think having a build tag means that we keep with our original goal of making go test ./... work whilst also allowing us to move forward with the make migration.

It also means that we are able to store our test files wherever we want/alongside the source code they are testing like normal too... (which plays nicer with hierarchical OWNERS files too)