Update golden test framework #418

justinsb · 2025-04-25T15:59:13Z

* Move to envtest by default (instead of mock-kubeapiserver)

* Create shared code for capturing requests / normalization.

* Use a more conventional env var WRITE_GOLDEN_OUTPUT

* Support object rewriting

justinsb · 2025-04-25T16:02:51Z

I'm not sure whether anyone is using this framework, I suspect not because it is tied to mock-kubeapiserver. I think it's easier to just offer envtest by default, and there are a few other simplifications and requirements that brings (e.g. starting the manager).

If we can, I'd also like to change the directory from tests to testsdata and move to a directory-based format (as we do in KCC), rather than having a bunch of files with the same prefixes in the same directory.

These are breaking, but they are test code and I can't imagine people are using this outside of this repo (based on my experience trying to use it!). But if anyone knows of any usage, we can always do a v2 or just iterate on this externally. But because this is breaking (albeit test code) I think we should do it after updating the version.

cc @tomasaschan

tomasaschan · 2025-04-25T16:08:35Z

I'm fine with the breaking change.

Re: updating the version, I need access if you want help :) #407 (comment)

tomasaschan · 2025-04-29T06:42:33Z

/lgtm
/hold

Marking this ready-to-go, but let's hold it until we have #417 done and a new tag available for folks who want a slower migration path (if there are any 🙃). I don't mind this breakage making it into v0.20, but agree we should have the beta.1 tagged without it (we can even tag beta.2 with it, if we want to have a prerelease that includes it).

justinsb · 2025-04-29T21:21:50Z

A massive diff, the majority of which is because I switched from json.Marshal to json.MarshalIndent. I could easily split that out into a separate PR though ... or just not do it at all. There will be changes to most golden-test-data lines (because of UID and resourceVersion simplification) which is why I thought it might be worth doing it in this PR.

tomasaschan · 2025-05-03T08:03:32Z

dev/test

@@ -27,6 +27,9 @@ cd "${REPO_ROOT}"

 set -x

+# Download the kubebuilder assets for envtest
+export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path)


As of kubernetes-sigs/controller-runtime#3135 this isn't really strictly necessary anymore. Would be worthwhile getting rid of while we're doing an overhaul, imo!

OK that is very cool. I don't think it is released yet on a controller-runtime tag (at least not that I could see). We could keep our main/master branch close to the upstream controller-runtime main/master branch, now that we have branches!

tomasaschan · 2025-05-03T08:06:56Z

ktest/testharness/kubeapiserver.go

+		})
+	} else {
+		// Removed for now...
+		t.Fatalf("mockkubeapiserver is not supported with this test harness")


Are you planning to add this back? If not, I suggest making the parameter non-nil and just getting rid of the branch here.

Pondering.... :-)

I added it back. The output is slightly different because of the api discovery v2 features added in k/k, but I figure we can tackle that separately (if we choose to)

justinsb · 2025-05-06T15:41:41Z

examples/guestbook-operator/controllers/tests/patches-stable.in.yaml

@@ -2,6 +2,7 @@ apiVersion: addons.example.org/v1alpha1
 kind: Guestbook
 metadata:
  name: guestbook-sample
+  namespace: default


So with envtest we need to create the objects for real, and kube-apiserver rejects the namespace-scoped object without a namespace. (We're not using kubectl, which would apply the default namespace for us)

We might be able to get away with not creating the object, but where there are "side objects" / dependencies those do need to created. I personally see this as the test suite now catching an invalid object.

tomasaschan

/lgtm

Great work on this! Are you happy enough with it that we should remove the hold and get it in, or should we publish a non-beta 0.20 first?

tomasaschan · 2025-05-13T21:38:31Z

ktest/httprecorder/kube_normalize.go

+// rewriteAPIV3Hashes removes some semi-random hashes from the api discovery response.
+// It is semi-random because these are the hashes of all the types in that group,
+// so should be stable for a single k8s patch release.
+func rewriteAPIV3Hashes(t *testing.T, body map[string]any) {


Given that discovery requests are omitted, this should be unnecessary, right?

Hmmm good question. So I did it because the hashes appear in the URL and they were appearing (and changing on patch kube version bumps). I think for discovery requests we only replace the body, but let me check.

(I think it's important that the discovery requests have a footprint because one of the problems we had in the past was that we were doing discovery every time. That's less important now that discovery is much more efficient, but I want to avoid regression there and on similar inefficiencies that don't change the outcome)

You were spot on. I added openapi/v3 to the list of endpoints which should be removed for length, and that let me remove this function entirely. There are a few remaining discovery queries with hashes in the URL, but query parameters are much easier to replace so I did that.

* Add support for envtest (instead of mock-kubeapiserver) * Create shared code for capturing requests / normalization. * Use a more conventional env var WRITE_GOLDEN_OUTPUT * Support object rewriting

justinsb · 2025-05-14T18:09:52Z

Personally I think we should merge this, because I think it's only going to touch test code and not the most widely used test code at that... If anything we want the envtest support because without the tests will use mockkubeapiserver, which does not accurately reflect modern kube. (I would like to fix that too, but not in this PR!)

tomasaschan

Sweet!

/lgtm

k8s-ci-robot · 2025-05-14T19:30:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tomasaschan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb,tomasaschan]

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

tomasaschan · 2025-05-14T19:42:13Z

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 25, 2025

k8s-ci-robot requested review from atoato88 and tomasaschan April 25, 2025 15:59

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 25, 2025

justinsb force-pushed the update_golden_test branch from 35b6b37 to c4ee825 Compare April 26, 2025 14:04

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 26, 2025

justinsb force-pushed the update_golden_test branch 4 times, most recently from 69e95b9 to 1574649 Compare April 26, 2025 15:24

justinsb changed the title ~~WIP: Update golden test framework~~ Update golden test framework Apr 28, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2025

justinsb mentioned this pull request Apr 28, 2025

Prepare for v0.20 #417

Merged

3 tasks

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2025

k8s-ci-robot assigned tomasaschan Apr 29, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2025

justinsb force-pushed the update_golden_test branch from 1574649 to e53951c Compare April 29, 2025 21:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2025

justinsb force-pushed the update_golden_test branch 2 times, most recently from 41cf5c3 to a1604de Compare April 29, 2025 21:17

justinsb force-pushed the update_golden_test branch 3 times, most recently from ab7c46b to 63262a9 Compare April 29, 2025 22:29

tomasaschan reviewed May 3, 2025

View reviewed changes

justinsb force-pushed the update_golden_test branch from 63262a9 to 4faf744 Compare May 6, 2025 15:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 6, 2025

justinsb force-pushed the update_golden_test branch from 4faf744 to e3ab3cc Compare May 6, 2025 15:44

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 6, 2025

justinsb force-pushed the update_golden_test branch 4 times, most recently from 34e0281 to 75d3d2f Compare May 13, 2025 11:32

tomasaschan reviewed May 14, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2025

justinsb added 2 commits May 14, 2025 13:54

Update golden test framework

2298fa3

* Add support for envtest (instead of mock-kubeapiserver) * Create shared code for capturing requests / normalization. * Use a more conventional env var WRITE_GOLDEN_OUTPUT * Support object rewriting

autogen: update golden test output

7c8ce69

justinsb force-pushed the update_golden_test branch from 75d3d2f to 7c8ce69 Compare May 14, 2025 17:54

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2025

tomasaschan approved these changes May 14, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2025

k8s-ci-robot merged commit 8710291 into kubernetes-sigs:master May 14, 2025
6 checks passed

gemmahou mentioned this pull request May 19, 2025

tests: add golden tests for operator GoogleCloudPlatform/k8s-config-connector#4528

Merged

Update golden test framework #418

Update golden test framework #418

Uh oh!

Conversation

justinsb commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinsb commented Apr 25, 2025

Uh oh!

tomasaschan commented Apr 25, 2025

Uh oh!

tomasaschan commented Apr 29, 2025

Uh oh!

justinsb commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomasaschan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinsb commented May 14, 2025

Uh oh!

tomasaschan left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 14, 2025

Uh oh!

tomasaschan commented May 14, 2025

Uh oh!

Uh oh!

Uh oh!

justinsb commented Apr 25, 2025 •

edited

Loading

tomasaschan left a comment •

edited

Loading