Hm, I don't think we should be moving everything under test/integration... this is test code that is specifically related to this area of the code (i.e. there is one per API group) and this feels like it makes things less clear.

which mean they can't pass on a clean checkout without further setup.

Is there nothing that can be done to work around this? What's the long term plan for making this work without "further setup"? Right now this test case (in Bazel) defines what it depends on clearly in the BUILD.bazel file. Once we don't have Bazel here, how will this work? I'd really like it if we didn't have to just hide some setup code into a Makefile that's far away/not directly related to this package

I've added a comment to the main tracking issue to try and explain what I'd personally like: #4712 (comment)

I believe that the strategy of moving things that don't pass out of the box into the test/ directory is fundamentally not meeting our goal of "make go test ./... work" and is instead pushing the cognitive work onto the user to know which packages can and can't work with go test.

Can we discuss on that main issue what our intended end-state goal is before we starting moving more things around/adding tags etc?

(namely @wallrj it'd be great if you can outline the expected UX from the developer running tests as well as for developers writing tests standpoint, with the approach where we put all tests that won't work with go test ./... out the box into their own directory)

My other comment is that I can't tell what the pruning_tests do.
Do we need them any more?

They ensure that the generated OpenAPI schema is valid for the given Go API types, by generating random data into the struct (i.e. filling out all known fields in the Go type), encoding that to JSON, "pruning" unknown fields in the JSON document based on the OpenAPI schema, and then decoding it back into the Go type. It then verifies that the input and the output are identical via a DeepEqual of the old and new Go structs. This has caught bugs in the actual OpenAPI generator (aka controller-gen) in the past, and will catch any issues if we were to add a new type & field to our API definitions in such a way that it 'trips up' controller-gen :)

I will be happy if the outcome is that I can run the only the unit-tests for a particular package and sub-packages without having to use make and without having to worry about running setip scripts first. E.g. If I've been working on validation functions and their unit tests I might run:

As per my comment in #4712 (comment), that's precisely what using build tags would also achieve - go test would only execute those that can run without any extra setup.

I think we also considered adding build tags to tests that are not hermetic (leaving them among the ./pkg and ./internal directories), but we rejected that because it would be too easy to add the wrong build tag and accidentally not run tests when you intended to run the entire suite.

To this point, I don't think it would be easy to add the wrong build tag, because when you then run those test cases, the tests will fail because the appropriate setup won't be run. The only case I can think here is if somewhere were to add an integration flag when they don't need it - but that's effectively the same as if someone adds the file to test/integration by accident too (and should be picked up in review). It also doesn't mean the tests won't run (though it is obviously non-ideal to miscategorise them)

They ensure that the generated OpenAPI schema is valid for the given Go API types, by generating random data into the struct (i.e. filling out all known fields in the Go type), encoding that to JSON, "pruning" unknown fields in the JSON document based on the OpenAPI schema, and then decoding it back into the Go type. It then verifies that the input and the output are identical via a DeepEqual of the old and new Go structs. This has caught bugs in the actual OpenAPI generator (aka controller-gen) in the past, and will catch any issues if we were to add a new type & field to our API definitions in such a way that it 'trips up' controller-gen :)

TBH that sounds like a test that belongs in https://github.com/kubernetes-sigs/controller-tools not in cert-manager.
Is this an example of such a bug?

• Objectmeta of embedded object goes missing during CRD conversion kubernetes-sigs/controller-tools#448

TBH that sounds like a test that belongs in https://github.com/kubernetes-sigs/controller-tools not in cert-manager.

We did discuss adding it upstream, but crucially you need a corpus of cases to test in that project, which could always be non-exhaustive compared to our own types.

By running this test, we:

1. verify that our schema is correct (which is a good thing to have confidence in)
2. surface new bugs over time in the controller-gen tool

To avoid this going off topic though, if we think it's worthwhile shall we discuss removing them in its own issue? I am personally against removing them because the net result is less confidence in our openapi schema :)

@wallrj I've made the changes you requested and updated the PR description to reference the specific goal of moving these files. I moved the files to test/integration.

To avoid this going off topic though, if we think it's worthwhile shall we discuss removing them in its own issue?

Agreed. I won't discuss further here.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish, wallrj

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

The pull request process is described here

And given that those tests are concerned with catching bugs in controller-gen tool and may in future be moved upstream to the controller-tools project.

It'd be good to chat about this - to clarify, moving these tests to only be run upstream would not give us the level of confidence we have today. Upstream may choose to run a similar test with their own corpus of types to prevent regressions, but our testing here catches unknown/new bugs that'd otherwise specifically affect us.

So these tests shouldn't be removed in future 😊

/unhold

/test all

(check status says "Pending — Not mergeable. Retesting 5 jobs." but there don't appear to be any tests running)