🌱 test: Improve registry+v1 render regression test #2103

camilamacedo86 · 2025-07-16T02:10:56Z

Simplify and fix render regression test

Old test made it easy to overwrite expected files via make verify.
In all Operator Framework projects, make verify is used for codegen (go fmt, controller-gen, etc), not for regenerating testdata that should never change.
See context
Expected manifests should only change when renderer logic changes on purpose.

This PR:

Moves bundles and expected output under testdata/, following Go test conventions.
Makes it clear the ArgoCD bundle is used only for this regression test.
Adds a proper test that runs the renderer and checks the output matches the expected YAMLs.

Safer, clearer, and avoids mistakes in reviews.

netlify · 2025-07-16T02:11:39Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`5a007e9`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/688b0da68a37f300081a8564
😎 Deploy Preview	https://deploy-preview-2103--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2025-07-16T02:28:34Z

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.84%. Comparing base (4382db6) to head (5a007e9).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
test/regression/convert/generate-manifests.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2103      +/-   ##
==========================================
- Coverage   73.59%   72.84%   -0.75%     
==========================================
  Files          78       79       +1     
  Lines        7260     7340      +80     
==========================================
+ Hits         5343     5347       +4     
- Misses       1567     1644      +77     
+ Partials      350      349       -1

Flag	Coverage Δ
e2e	`43.50% <ø> (+0.04%)`	⬆️
experimental-e2e	`56.21% <ø> (+0.04%)`	⬆️
unit	`58.22% <0.00%> (-0.61%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

internal/operator-controller/rukpak/render/render_test.go

camilamacedo86 · 2025-07-22T00:40:26Z

test/convert/README.md

@@ -1,7 +0,0 @@
-## registry+v1 bundle generation regression tests


we do not need the readme anymore
now, the code is documented and we can clarify on it directly to simplify

camilamacedo86 · 2025-07-22T00:47:29Z

Makefile

@@ -252,6 +246,10 @@ test-unit: $(SETUP_ENVTEST) envtest-k8s-bins #HELP Run the unit tests
                $(UNIT_TEST_DIRS) \
                -test.gocoverdir=$(COVERAGE_UNIT_DIR)

+.PHONY: test-regression
+test-regression: #HELP Run regression test
+	go test -count=1 -v ./test/regression/...


We now have a dedicated directory for regression tests, so it's easy to add more cases in the future.
It also makes it very clear what this test does and why it's there.

This does not appear to be invoked in any CI, specifically, this is only included in test, which is something we only do locally.

I did not realise that we did not call make test in the CI :-(
Good catcher 💯

Since we have a specific action for unit tests and can, in the future, expand the regression tests to cover other scenarios, I added a new action to call this one.

It is here now: https://github.com/operator-framework/operator-controller/actions/runs/16507728332/job/46682560856?pr=2103

@tmshort could we move forward with this one now?

camilamacedo86 · 2025-07-22T00:52:32Z

Makefile

@@ -172,15 +172,9 @@ generate: $(CONTROLLER_GEN) #EXHELP Generate code containing DeepCopy, DeepCopyI
 	$(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) object:headerFile="hack/boilerplate.go.txt" paths="./..."

 .PHONY: verify
-verify: k8s-pin kind-verify-versions fmt generate manifests crd-ref-docs generate-test-data #HELP Verify all generated code is up-to-date. Runs k8s-pin instead of just tidy.


If make verify failed in CI, most people would just run it locally and push the changes — that’s normal since verify usually runs things like go fmt, vet, and controller-gen (as in all OF projects). But it was also re-generating regression test output — which should never change or at least very unlikely change. Then:

If that output changed because of a bug, we could accidentally miss it and merge a regression.

It also wasn’t clear to reviewers that files in testdata/expected-manifests/ should never be edited or auto-regenerated.

Expecting everyone to remember to check that manually is risky and easy to miss.

perdasilva · 2025-07-22T08:57:59Z

/approve

openshift-ci · 2025-07-22T08:58:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva

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~~ [perdasilva]

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

test/regression/convert/covert_test.go

bentito · 2025-07-22T17:38:41Z

test/regression/convert/covert_test.go

+	cmd.Stdout = os.Stdout
+
+	err := cmd.Run()
+	require.NoError(t, err, "failed to generate manifests")


Spawning go run generate-manifests.go inside your test will recompile the whole helper and is fairly slow. You could factor the logic in generate-manifests.go into a package and call that from the test binary. This will surface compile-time errors directly in the test binary.

Thanks for the feedback!

You're right — using go run generate-manifests.go is a bit slower since it recompiles the helper. I timed it, and it's around ~500ms to 2 seconds faster, but feels fine for this test since it only runs once, not per test case.

We now have a stronger check in place for renderer output, and I think it's solid enough to move forward with this version. I do agree that may have hall for improvements, but it might be more effort than it's worth at the moment.

That said, I’d be happy if someone wants to improve it later!

WDYT — okay to move forward for now?

test/regression/convert/covert_test.go

test/regression/convert/generate-manifests.go

bentito · 2025-07-22T17:45:30Z

test/regression/convert/generate-manifests.go


-	if err := os.RemoveAll(outputRootDir); err != nil {
+	if err := os.RemoveAll(*outputRootDir); err != nil {
 		fmt.Printf("error removing output directory: %v\n", err)


should this direct to stderr so it doesn't mix with stdout at some point, or maybe even import log and log.Fatalf

The test was failing because it couldn't remove a temporary directory.

I don’t think that should be a hard failure — if this happens in CI, the container will be cleaned up anyway when the job ends. It’s not critical to the test result itself.

I also added the directory to .gitignore to make sure we don’t accidentally commit it in any case.

tmshort · 2025-07-31T01:28:00Z

.github/workflows/test-regression.yaml

+    - uses: codecov/codecov-action@v5.4.3
+      with:
+        disable_search: true
+        files: coverage/unit.out
+        flags: unit
+        token: ${{ secrets.CODECOV_TOKEN }}


I don't think we need the codecov action here. This could inadvertently trigger codecov to think it's received all its data before the (real) tests complete. If you want this here, you should increase the value here:

operator-controller/codecov.yml

Line 3 in e9c5b6e

after_n_builds: 3

The number 3 corresponds to test-unit, test-e2e and test-experimental-e2e.

Interesting now, I think I understand better why codev instead of the old coveralls :-)

I think we should either use it if we are covering more within

All done !

Actually interesting, the experimental-e2e isn't creating it's own directory, it's sharing it with e2e, so I there' still 3 files, which I'm not sure if it corresponds to three pushes...

However, I'm not sure it's a good idea to code cover the test harness, as that gives a false impression of code coverage (either positive or negative).
Regardless, I'll let this pass.

If we add more tests here, will we not be covering more scenarios and more areas of the code each time?
I think we need to fix the experimental-e2e. I can look at that in a follow-up.
Could you LGTM this one for we get it merged?

tmshort · 2025-07-31T17:06:01Z

/lgtm

camilamacedo86 requested a review from a team as a code owner July 16, 2025 02:10

openshift-ci bot requested review from bentito and oceanc80 July 16, 2025 02:11

camilamacedo86 force-pushed the remove-bundles-argo branch from 9be93c2 to f32fc0e Compare July 16, 2025 02:18

camilamacedo86 force-pushed the remove-bundles-argo branch 7 times, most recently from 311dab9 to 36ae501 Compare July 16, 2025 07:17

camilamacedo86 closed this Jul 16, 2025

camilamacedo86 reopened this Jul 16, 2025

camilamacedo86 force-pushed the remove-bundles-argo branch 3 times, most recently from f7a2e82 to 0abdf8f Compare July 16, 2025 13:12

camilamacedo86 changed the title ~~(NOT MERGE) - Test~~ 🌱 test: replace ArgoCD manifest regression test with mock-based resource validation Jul 16, 2025

camilamacedo86 commented Jul 16, 2025

View reviewed changes

internal/operator-controller/rukpak/render/render_test.go Outdated Show resolved Hide resolved

camilamacedo86 requested review from perdasilva, anik120 and thetechnick July 16, 2025 14:06

camilamacedo86 changed the title ~~🌱 test: replace ArgoCD manifest regression test with mock-based resource validation~~ 🌱 test: Improve registry+v1 render regression test Jul 22, 2025

camilamacedo86 force-pushed the remove-bundles-argo branch from 0abdf8f to 1ea2527 Compare July 22, 2025 00:39

camilamacedo86 commented Jul 22, 2025

View reviewed changes

camilamacedo86 force-pushed the remove-bundles-argo branch 2 times, most recently from 6d713af to 8a66074 Compare July 22, 2025 00:46

camilamacedo86 commented Jul 22, 2025

View reviewed changes

camilamacedo86 requested a review from perdasilva July 22, 2025 08:56

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2025