Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tests to not use kind #258

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor tests to not use kind #258

wants to merge 1 commit into from

Conversation

kokes
Copy link
Contributor

@kokes kokes commented Mar 6, 2024

The problem with tests here is that they use shared state - there is one big bulk of secrets created and reused for most tests. This creates issues with debugging, adding new tests, false positives etc.

Since it's not feasible to start with a clean slate for each test when using "real" Kubernetes clusters (such as kind, in this instance), I refactored all tests to use client-go's so-called fake clientset. This behaves like a proper clientset for most of the basic operations - and I could simulate RBAC issues by using PrependReactor, a builtin feature of this fake clientset.

This means that:

  1. Each test starts with a fresh set of secrets, no shared state
  2. Each test's setup is clearly visible in the test body, so it's super clear what's being tested
  3. Performance is pretty good - the tests run in 4.5 seconds (and most of that is spent sleeping)
  4. Test invocation does not require a Kubernetes cluster of any kind (whoops)
  5. CI runs are now a little simpler - I removed the kind dependency and switched to using setup-golang's cache instead of actions/cache

(Note on test performance: tests could run in parallel and pretty much instantly, but that would require a bit of metrics refactoring [making them native Prometheus objects, not instant metrics] and I didn't want to complicate the PR any more.)

I ran these tests locally + tested all relevant CI jobs on my fork.

The problem with tests here is that they use shared state - there is one big bulk of secrets created and reused for most tests. This creates issues with debugging, adding new tests, false positives etc.

Since it's not feasible to start with a clean slate for each test when using "real" Kubernetes clusters (such as kind, in this instance), I refactored all tests to use client-go's so-called fake clientset. This behaves like a proper clientset for most of the basic operations - and I could simulate RBAC issues by using `PrependReactor`, a builtin feature of this fake clientset.

This means that:

1. Each test starts with a fresh set of secrets, no shared state
2. Each test's setup is clearly visible in the test body, so it's super clear what's being tested
3. Performance is pretty good - the tests run in 4.5 seconds (and most of that is spent sleeping)
4. Test invocation does not require a Kubernetes cluster of any kind (whoops)
5. CI runs are now a little simpler - I removed the kind dependency and switched to using setup-golang's cache instead of actions/cache

(Note on test performance: tests could run in parallel and pretty much instantly, but that would require a bit of metrics refactoring [making them native Prometheus objects, not instant metrics] and I didn't want to complicate the PR any more.)

I ran these tests locally + tested all relevant CI jobs on my fork.
@kokes kokes requested a review from a team as a code owner March 6, 2024 07:38
Copy link
Contributor

@plaffitt plaffitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Removing the need of a k8s cluster for unit test is a good thing, even if we should have some integration tests too.

I didn't review all of the tests in details, but overall it looks ok, and since it didn't break anything I assume they are fine for now.

Code reuse

I've spotted that you often reuse the same loop to create secrets :

n := 5
// create 5 secrets in ns0, ns2, ..., ns9
for j := 0; j < 10; j++ {
	if err := addKubeSecrets(client, n, fmt.Sprintf("ns%d", j)); err != nil {
		t.Fatal(err)
	}
}

It would be nice if you could extract this loop in a dedicated function in order to improve readability and reduce file size.

Coverage

It is nice because you improved code coverage on some part of the code, namely exporter.go, going from 92.5% to 97.2%. However you also removed some integration tests that were lost in unit tests and this decreased the coverage, I think about kubenetes.go here.

If you could reintroduce those tests using build tags to filter them, it would be lovely! (see Separate Your Go Tests with Build Tags for more details on how to do). @npdgm Maybe you would like to add something about this one? Especially concerning the CI pipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants