appliance: golden/snapshot testing #62034

craigfurman · 2024-04-19T11:33:37Z

Use envtest (https://book.kubebuilder.io/reference/envtest.html) to start a real Kubernetes API server, backed by a real etcd instance, in tests. The only controller running is our appliance controller, so these tests assert on the changes our controller actually makes to Kubernetes API objects in response to some input, but don't depend on complex infrastruture like container runtimes or persistent volume controllers (since no such controllers are actually running, unlike in a useful Kubernetes cluster).

This commit adds a test harness to the appliance package:

It starts the k8s API server only once, not once per test, as this is a relatively slow operation.
Helper functions to run each test in a unique namespace to avoid inter-test dependency. This opens the door to parallelism, but note that testify/suite doesn't currently support parallel tests.
Helper functions to load input ConfigMaps from fixture files.
Passing -args appliance-update-golden-files to go test will cause checked-in golden files to be updated.

See self-review notes below for more info.

Closes https://github.com/sourcegraph/sourcegraph/issues/62015

Test plan

This whole PR is about introducing a new test harness for the appliance component.

internal/appliance/blobstore.go

internal/k8s/resource/pod/pod.go

internal/appliance/testdata/sg/blobstore-default.yaml

internal/appliance/testdata/golden-fixtures/blobstore-subsequent-disable.yaml

craigfurman · 2024-04-19T11:44:53Z

internal/appliance/blobstore_test.go

+// More complex test cases involving updates to the configmap can have their own
+// test blocks
+func (suite *ApplianceTestSuite) TestBlobstoreResourcesDeletedWhenDisabled() {
+	namespace := suite.createConfigMap("blobstore-default")
+	suite.Require().Eventually(func() bool {
+		return suite.getConfigMapReconcileEventCount(namespace) > 0
+	}, time.Second*10, time.Millisecond*200)
+
+	eventsSeenSoFar := suite.getConfigMapReconcileEventCount(namespace)
+	suite.updateConfigMap(namespace, "everything-disabled")
+	suite.Require().Eventually(func() bool {
+		return suite.getConfigMapReconcileEventCount(namespace) > eventsSeenSoFar
+	}, time.Second*10, time.Millisecond*200)
+
+	suite.makeGoldenAssertions(namespace, "blobstore-subsequent-disable")
+}


Example of a more bespoke test that updates the configmap after creating it. If we ever find we need to do complex k8s resource upgrades (not just bumping versions), we can hopefully test that in this way.

internal/appliance/reconcile_test.go

internal/appliance/kubernetes_test.go

craigfurman · 2024-04-19T11:56:53Z

internal/appliance/kubernetes_test.go

+// Test helpers
+
+type ApplianceTestSuite struct {
+	suite.Suite


Note: testify/suite doesn't support parallel tests: stretchr/testify#187

If this becomes an issue later, we'll have to switch to a different harness or write our own.

I'm more than happy to revisit this down the line to get rid of testify (personally not a fan of it), but that's not a high priority 😄

Oops, apologies for not running this choice by the team before making this PR! I saw it used elsewhere in the monorepo, but of course maybe different teams have different tastes. I personally enjoy it as a lightweight alternative to macro'ing out if... t.Fail() everywhere 😅

It sounds like you're in favour of merging with testify and revisiting later, but on the other hand if we don't want to use it for assertions, maybe we switch before there's a ton of test code to change? What did you have in mind? 👀

Unpicking the suite stuff won't actually be that hard IMO.

You might find github.com/frankban/quicktest to be a nicer alternative if a simple API is all you're looking for

Are you talking about as a replacement for assertions, suite orchestration, or both?

It's just for assertion, I'm not much a fan of suites and practicaly never use them. Paired with https://pkg.go.dev/github.com/google/go-cmp/cmp it's a treat.

re: suites, this PR only uses the "run this before all tests, not each test" feature. It wouldn't be hard to unpick that, and I might do that. Ironically, the reason testify/suite doesn't support parallelism is that its implementation of "run this before each test" (the feature I'm not using) would create race conditions on the shared state between tests. These tests already share no state between themselves, because I was hoping to be able to run them in parallel 😂

internal/appliance/golden_test.go

internal/appliance/kubernetes_test.go

jdpleiness · 2024-04-19T18:15:21Z

internal/appliance/kubernetes_test.go

+		Scheme: scheme.Scheme,
+
+		// On macos with the built-in firewall enabled, every test run will
+		// cause a firewall dialog box to pop. That's incredibly annoying. This


Apparently I've pushed a very similar rock up this hill before: kubernetes-sigs/controller-runtime#322 😂

internal/appliance/kubernetes_test.go

internal/appliance/golden_test.go

internal/appliance/kubernetes_test.go

internal/appliance/reconcile.go

craigfurman · 2024-04-23T16:55:03Z

Ah, the bazel build that passes locally is failing in BK. That's surprising, I'll take a look at that. I probably shouldn't have marked this as ready until I was sure this would work!

Use envtest (https://book.kubebuilder.io/reference/envtest.html) to start a real Kubernetes API server, backed by a real etcd instance, in tests. The only controller running is our appliance controller, so these tests assert on the changes our controller actually makes to Kubernetes API objects in response to some input, but don't depend on complex infrastruture like container runtimes or persistent volume controllers (since no such controllers are actually running, unlike in a useful Kubernetes cluster). This commit adds a test harness to the appliance package: * It starts the k8s API server only once, not once per test, as this is a relatively slow operation. * Helper functions to run each test in a unique namespace to avoid inter-test dependency. This opens the door to parallelism, but note that testify/suite doesn't currently support parallel tests. * Helper functions to load input ConfigMaps from fixture files. * Passing `-args appliance-update-golden-files` to `go test` will cause checked-in golden files to be updated.

Fix bazel files and k8s resource tests.

Co-authored-by: Jacob Pleiness <[email protected]>

This reverts commit 9750e51f36ed0dd37776abf5628733f4dc69164d.

cla-bot bot added the cla-signed label Apr 19, 2024

craigfurman force-pushed the appliance-testing branch from e70016e to ed557bd Compare April 19, 2024 11:42

craigfurman commented Apr 19, 2024

View reviewed changes

craigfurman requested a review from jdpleiness April 19, 2024 11:58

craigfurman commented Apr 19, 2024

View reviewed changes

internal/appliance/golden_test.go Show resolved Hide resolved

jdpleiness reviewed Apr 19, 2024

View reviewed changes

craigfurman commented Apr 22, 2024

View reviewed changes

internal/appliance/reconcile.go Show resolved Hide resolved

craigfurman force-pushed the appliance-testing branch from 91fcaf9 to 7cd0540 Compare April 23, 2024 16:49

craigfurman marked this pull request as ready for review April 23, 2024 16:51

craigfurman requested a review from jdpleiness April 23, 2024 16:51

craigfurman force-pushed the appliance-testing branch 2 times, most recently from ab8706a to ea6b46e Compare April 24, 2024 08:40

craigfurman mentioned this pull request Apr 24, 2024

appliance: remove hash package #62151

Merged

jdpleiness approved these changes Apr 24, 2024

View reviewed changes

Craig Furman and others added 14 commits April 24, 2024 18:12

fixup! appliance: golden/snapshot testing

84b0fde

Fix bazel files and k8s resource tests.

Update internal/appliance/kubernetes_test.go

c030346

Co-authored-by: Jacob Pleiness <[email protected]>

Update internal/appliance/kubernetes_test.go

98b7cd7

Co-authored-by: Jacob Pleiness <[email protected]>

squashme: add appliance image default with TODO

f811a0f

squashme: consistent receiver name

b69e5b7

go fmt

59317ef

squashme: make exportloopref happy

e01cda6

Bazel integration for appliance tests

d3855b4

Fix deps.bzl

5d4b0a2

Forward test kube stdio to debug failure

e968c63

Give network to test

4d57d7d

Revert "Give network to test"

d64128f

This reverts commit 9750e51f36ed0dd37776abf5628733f4dc69164d.

Try binding localhost only with no test network

3278322

Incorporate golden file changes from rebase

82fd013

craigfurman force-pushed the appliance-testing branch from b267688 to 82fd013 Compare April 24, 2024 17:13

craigfurman enabled auto-merge (squash) April 24, 2024 19:51

craigfurman merged commit c961770 into main Apr 24, 2024
11 checks passed

craigfurman deleted the appliance-testing branch April 24, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

appliance: golden/snapshot testing #62034

appliance: golden/snapshot testing #62034

craigfurman commented Apr 19, 2024 •

edited

Loading

craigfurman Apr 19, 2024

craigfurman Apr 19, 2024

jdpleiness Apr 19, 2024

craigfurman Apr 22, 2024

jhchabran Apr 22, 2024

craigfurman Apr 22, 2024

jhchabran Apr 22, 2024 •

edited

Loading

craigfurman Apr 22, 2024 •

edited

Loading

jdpleiness Apr 19, 2024

craigfurman Apr 22, 2024

craigfurman commented Apr 23, 2024

appliance: golden/snapshot testing #62034

appliance: golden/snapshot testing #62034

Conversation

craigfurman commented Apr 19, 2024 • edited Loading

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhchabran Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

craigfurman Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigfurman commented Apr 23, 2024

craigfurman commented Apr 19, 2024 •

edited

Loading

jhchabran Apr 22, 2024 •

edited

Loading

craigfurman Apr 22, 2024 •

edited

Loading