Separate out integration tests:

[jacobweinstock]

Issue #, if available:
#5364

Description of changes:
Without breaking any Make goals, this allows specifying an env var (INTEGRATION_TESTS_ENABLED) that will cause any test that uses envtest to either run or not. By default, make unit-test will run all tests that use envtest. i.e. INTEGRATION_TESTS_ENABLED=true. To disable tests that use envtest, set INTEGRATION_TESTS_ENABLED=false.

For example, with make, to disable envtest tests run:
INTEGRATION_TESTS_ENABLED=false make unit-test

When running go directly, the default behavior is to not run any envtest tests.
go test ./... is the same as INTEGRATION_TESTS_ENABLED=false go test ./...

To enable envtest with go directly, you'll need to start envtest and then run: INTEGRATION_TESTS_ENABLED=true go test ./...

Testing (if applicable):

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[eks-distro-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jacobweinstock. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3f055fc) 75.64% compared to head (afed219) 75.66%.
Report is 151 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6723      +/-   ##
==========================================
+ Coverage   75.64%   75.66%   +0.02%     
==========================================
  Files         474      475       +1     
  Lines       38303    38405     +102     
==========================================
+ Hits        28973    29060      +87     
- Misses       7723     7733      +10     
- Partials     1607     1612       +5     

Files	Coverage Δ
pkg/providers/tinkerbell/assert.go	70.03% <100.00%> (+0.10%)	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[g-gaston]

Doing something like

if os.Getenv(integrationTestEnvVar) != "true" {
		s.Skipf("set env var '%v=true' to run this test", integrationTestEnvVar)
	}

adds extra work for engineers. It's not a lot but it's one more thing to remember. And the worse thing is, if they forget, they will just break go test ./... again (which is what we are trying to solve here) but they'll probably won't realize since they'll have INTEGRATION_TESTS_ENABLED enabled, which will just work.

Alternatively, what do you think about using go build tags? That would require moving all tests using envtest to a different file, like *_integration_test.go. This requires identifying all tests that need envtest, but it seems looking at this PR that you already did all the difficult work :)

The benefit of this is there is no need for extra code in the test functions, just putting them in the right file. And if you don't put them in the right file, the tests won't even compile since it won't have access to var env *envtest.Environment. TBH I also personally like the idea of having them in a different file if we are going to start treating them differently.

[g-gaston]

Doesn't this skip all tests for the package? both the ones that use envtest and the ones that don't.

I could be wrong but I thought if you include a TestMain that executed always. And if you don't run m, then nothing runs.

[jacobweinstock]

I think you're right, good catch. i've updated this to also be able to run without envtest.

[chrisdoherty4]

I'm indifferent to the approach we use as both have tradeoffs. If we shoot for the env variable approach though, I think it would be useful to add a helper so we don't need to include the boilerplate.

func TestFoo(t *testing.T) {
	test.MarkIntegration(t)
}

Either way, thanks for this, Jacob. Here's an insight I gathered recently.

[jacobweinstock]

test.MarkIntegration(t)

Thanks for the feedback, @g-gaston . The challenge i have with build tags is that they are not easily discoverable. With this current pattern you get that discoverability. go test will clearly show the env var(s) that need to be set in order to run the integration tests. Reference.

In regard to the extra work; using envtest is way more work than adding a one-liner test.MarkIntegration(t). Reference.

The breaking of go test if an engineer forgets to add test.MarkIntegration(t) is actually a good thing. With this pattern they will now see that they wrote an integration test without marking it accordingly and can fix it. Instead of currently go test will never work. Couple that with in the future we would have make unit-test only run unit tests and be a part of the github action so that CI catches this as well.

[chrisdoherty4]

This approach seems sound to me.

[g-gaston]

this made me think
What if we made engineers have to call this/some other method in order to use envtest?

Right now this method just skips the test if the env var is not set. But if the engineer doesn't add this line, the test will compile and try to run.

We could however "require" this method. Right now all tests using envtest need a package level variable called env, that is setup from TestMain. What if we set a pattern where instead of accessing this variable directly, we call a method to retrieve it? That way engineers will "need" to call this method if they want access to envtest. Of course, they could still look for the package level variable, but if we name it something like internalEnvtest and add a decent comment, I doubt this will happen by mistake.

Quick example of what I'm proposing:

func TestCloudStackDatacenterReconcilerSetupWithManager(t *testing.T) {
  env := requireEnvtest(t)

The in this package main_test.go:

// internalEnvtest allows to use envtest when integration
// tests are enabled.
// DO NOT use it directly, instead use requireEnvtest.
var internalEnvtest *envtest.Environment

func requireEnvtest(tb testing.TB) *envtest.Environment {
  test.MarkIntegration(t)
  return internalEnvtest
}

[chrisdoherty4]

I'm not sold on the value. envtest features in a subset of integration tests so its not a silver bullet. Moreover, developers have the freedom to tweak envtest configuration dependent on their usecase which may mean not using any of the existing setup (package state isn't required for envtest; see Hegel integration tests as an example).

I suggest we reserve this change for a separate PR.

[g-gaston]

Thanks for the feedback, @g-gaston . The challenge i have with build tags is that they are not easily discoverable. With this current pattern you get that discoverability. go test will clearly show the env var(s) that need to be set in order to run the integration tests. Reference.

I think this hints to our main discrepancy on this topic. Please correct me if I'm making the wrong assumption, but I feel you see go test as the top level entry point to our repo tests, while for me, make is such entry point.

From where I stand, make help (or make [tab]) is how we discover the different sets of tests one can run. And running those targets takes care of setting up the environment so they run properly. Instead of discovering "how to configure the tests", you just discover "the available tests".

Not trying to convince anyone here, just pointing where I think the disagreement comes from :)

I still prefer build tags, but if you are firm on using env vars + t.Skip, please take a look to my comment above about a possible iteration of your idea, that I think makes it more obvious/robust.

In regard to the extra work; using envtest is way more work than adding a one-liner test.MarkIntegration(t). Reference.

There might be something I'm missing. Running tests with envtest should only require running the right make target. And if it's about writing tests with envtest, at most, is adding the a main_test.go file to the right folder. If we are talking about using envtest with go test, it only requires a one time setup and exporting an envvar.

I'm not sure what extra work envtest involves (the reference link seems to be just terry's opinion, which I appreciate but it's just one opinion) but I'm interested in hearing the issues you faced because I would like to fix them (orthogonally to this PR).

[jacobweinstock]

Thanks for the feedback, @g-gaston . The challenge i have with build tags is that they are not easily discoverable. With this current pattern you get that discoverability. go test will clearly show the env var(s) that need to be set in order to run the integration tests. Reference.

I think this hints to our main discrepancy on this topic. Please correct me if I'm making the wrong assumption, but I feel you see go test as the top level entry point to our repo tests, while for me, make is such entry point.

From where I stand, make help (or make [tab]) is how we discover the different sets of tests one can run. And running those targets takes care of setting up the environment so they run properly. Instead of discovering "how to configure the tests", you just discover "the available tests".

Not trying to convince anyone here, just pointing where I think the disagreement comes from :)

I still prefer build tags, but if you are firm on using env vars + t.Skip, please take a look to my comment above about a possible iteration of your idea, that I think makes it more obvious/robust.

In regard to the extra work; using envtest is way more work than adding a one-liner test.MarkIntegration(t). Reference.

There might be something I'm missing. Running tests with envtest should only require running the right make target. And if it's about writing tests with envtest, at most, is adding the a main_test.go file to the right folder. If we are talking about using envtest with go test, it only requires a one time setup and exporting an envvar.

I'm not sure what extra work envtest involves (the reference link seems to be just terry's opinion, which I appreciate but it's just one opinion) but I'm interested in hearing the issues you faced because I would like to fix them (orthogonally to this PR).

Thanks for the reply @g-gaston. There is a lot going on in this PR but the main goal is to update make unit-test to only run unit tests. I think the only outstanding concern is whether to use an env var or build tags, no?

[chrisdoherty4]

@g-gaston Are we OK with this change?

[g-gaston]

@g-gaston Are we OK with this change?

TBH, I'm not sure the net change here is positive. But if you are both believe it is and are both aligned on the solution, let's try it and see what happens. Just make sure we send a message to the team informing of the new requirement, since this will affect everyone writing tests with envtest.