Skip to content

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .github/workflows/test-regression.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
name: test-regression

on:
workflow_dispatch:
pull_request:
merge_group:
push:
branches:
- main

jobs:
test-regression:
runs-on: ubuntu-latest
steps:

- uses: actions/checkout@v4

- uses: actions/setup-go@v5
with:
go-version-file: go.mod

- name: Run basic unit tests
run: |
make test-regression

- uses: codecov/[email protected]
with:
disable_search: true
files: coverage/unit.out
flags: unit
token: ${{ secrets.CODECOV_TOKEN }}
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,6 @@ site
.tiltbuild/
.catalogd-tmp/
.vscode

# Tmporary files and directories
/test/regression/convert/testdata/tmp/*
14 changes: 6 additions & 8 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Jul 22, 2025

Choose a reason for hiding this comment

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

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.

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

# Renders registry+v1 bundles in test/convert
# Used by CI in verify to catch regressions in the registry+v1 -> plain conversion code
.PHONY: generate-test-data
generate-test-data:
go run test/convert/generate-manifests.go

.PHONY: fix-lint
fix-lint: $(GOLANGCI_LINT) #EXHELP Fix lint issues
$(GOLANGCI_LINT) run --fix --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS)
Expand Down Expand Up @@ -209,7 +203,7 @@ verify-crd-compatibility: $(CRD_DIFF) manifests
#SECTION Test

.PHONY: test
test: manifests generate fmt lint test-unit test-e2e #HELP Run all tests.
test: manifests generate fmt lint test-unit test-e2e test-regression #HELP Run all tests.

.PHONY: e2e
e2e: #EXHELP Run the e2e tests.
Expand Down Expand Up @@ -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/...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Jul 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


.PHONY: image-registry
E2E_REGISTRY_IMAGE=localhost/e2e-test-registry:devel
image-registry: export GOOS=linux
Expand Down
7 changes: 0 additions & 7 deletions test/convert/README.md

This file was deleted.

120 changes: 120 additions & 0 deletions test/regression/convert/convert_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*
## registry+v1 bundle regression test

This test in convert_test.go verifies that rendering registry+v1 bundles to manifests
always produces the same files and content.

It runs: go run generate-manifests.go -output-dir=./testdata/tmp/rendered/
Then compares: ./testdata/tmp/rendered/ vs ./testdata/expected-manifests/

Files are sorted by kind/namespace/name for consistency.

To update expected output (only on purpose), run:

go run generate-manifests.go -output-dir=./testdata/expected-manifests/
*/
package main

import (
"bytes"
"fmt"
"os"
"os/exec"
"path/filepath"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
)

// Test_RenderedOutputMatchesExpected runs generate-manifests.go,
// then compares the generated files in ./testdata/tmp/rendered/
// against expected-manifests/.
// It fails if any file differs or is missing.
// TMP dir is cleaned up after test ends.
func Test_RenderedOutput_MatchesExpected(t *testing.T) {
tmpRoot := "./testdata/tmp/rendered/"
expectedRoot := "./testdata/expected-manifests/"

// Remove the temporary output directory always
t.Cleanup(func() {
_ = os.RemoveAll("./testdata/tmp")
})

// Call the generate-manifests.go script to generate the manifests
// in the temporary directory.
cmd := exec.Command("go", "run", "generate-manifests.go", "-output-dir="+tmpRoot)
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout

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

// Compare structure + contents
err = compareDirs(expectedRoot, tmpRoot)
require.NoError(t, err, "rendered output differs from expected")
}

// compareDirs compares expectedRootPath and generatedRootPath directories recursively.
// It returns an error if any file is missing, extra, or has content mismatch.
// On mismatch, it includes a detailed diff using cmp.Diff.
func compareDirs(expectedRootPath, generatedRootPath string) error {
// Step 1: Ensure every expected file exists in actual and contents match
err := filepath.Walk(expectedRootPath, func(expectedPath string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}

relPath, err := filepath.Rel(expectedRootPath, expectedPath)
if err != nil {
return err
}
actualPath := filepath.Join(generatedRootPath, relPath)

expectedBytes, err := os.ReadFile(expectedPath)
if err != nil {
return fmt.Errorf("failed to read expected file: %s", expectedPath)
}
actualBytes, err := os.ReadFile(actualPath)
if err != nil {
return fmt.Errorf("missing file: %s", relPath)
}

if !bytes.Equal(expectedBytes, actualBytes) {
diff := cmp.Diff(string(expectedBytes), string(actualBytes))
return fmt.Errorf("file content mismatch at: %s\nDiff (-expected +actual):\n%s", relPath, diff)
}
return nil
})
if err != nil {
return err
}

// Step 2: Ensure actual does not contain unexpected files
err = filepath.Walk(generatedRootPath, func(actualPath string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}

relPath, err := filepath.Rel(generatedRootPath, actualPath)
if err != nil {
return err
}
expectedPath := filepath.Join(expectedRootPath, relPath)

_, err = os.Stat(expectedPath)
if os.IsNotExist(err) {
return fmt.Errorf("unexpected extra file: %s", relPath)
} else if err != nil {
return fmt.Errorf("error checking expected file: %s", expectedPath)
}
return nil
})
return err
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
// generate-manifests.go
//
// Renders registry+v1 bundles into YAML manifests for regression testing.
// Used by tests to make sure output from the BundleRenderer stays consistent.
//
// By default, writes to ./testdata/tmp/generate/.
// To update expected output, run:
//
// go run generate-manifests.go -output-dir=./testdata/expected-manifests/
//
// Only re-generate if you intentionally change rendering behavior.
// Note that if the test fails is likely a regression in the renderer.
package main

import (
"cmp"
"flag"
"fmt"
"os"
"path/filepath"
Expand All @@ -16,11 +29,26 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/registryv1"
)

// This is a helper for a regression test to make sure the renderer output doesn't change.
//
// It renders known bundles into YAML files and writes them to a target output dir.
// By default, it writes to a temp path used in tests:
//
// ./testdata/tmp/rendered/
//
// If you want to update the expected output, run it with:
//
// go run generate-manifests.go -output-dir=./testdata/expected-manifests/
//
// Note: Expected output should never change unless the renderer changes which is unlikely.
// If the convert_test.go test fails, it likely means a regression was introduced in the renderer.
func main() {
bundleRootDir := "testdata/bundles/"
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move testdata/bundle and testdata/expected-manifests under testdata/regression-test or something like that? Just keep those two directories grouped under the same heading/purpose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should keep this test data in test/regression/convert/.
This way, it's clear the data is only for the convert test.

If one day we have another regression test that also needs this data (which seems unlikely; it is very specific use case), then we can move it to the main regression/ folder.
That would show it's shared by more than one test.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's all good - I thought it was in the /testdata. This is perfect!

outputRootDir := "test/convert/expected-manifests/"
defaultOutputDir := "./testdata/tmp/rendered/"
outputRootDir := flag.String("output-dir", defaultOutputDir, "path to write rendered manifests to")
flag.Parse()

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

os.Exit(1)
}
Expand Down Expand Up @@ -52,7 +80,7 @@ func main() {
},
} {
bundlePath := filepath.Join(bundleRootDir, tc.bundle)
generatedManifestPath := filepath.Join(outputRootDir, tc.bundle, tc.testCaseName)
generatedManifestPath := filepath.Join(*outputRootDir, tc.bundle, tc.testCaseName)
if err := generateManifests(generatedManifestPath, bundlePath, tc.installNamespace, tc.watchNamespace); err != nil {
fmt.Printf("Error generating manifests: %v", err)
os.Exit(1)
Expand Down
Loading