🌱 Migrate e2e tests to Godog BDD framework

[pedjak]

Description

Replace traditional Go e2e tests with Godog (Cucumber for Go) to improve test readability and maintainability through behavior-driven development.

Benefits:

• Living Documentation: Test scenarios serve as up-to-date documentation of system behavior
• Better Collaboration: Product owners can read and validate test scenarios
• Reduced Duplication: Reusable step definitions eliminate code repetition
• Improved Maintainability: Changes to common patterns happen in one place
• Clearer Intent: Gherkin syntax makes test purpose immediately obvious
• Easier Debugging: Clear separation between what is tested (features) and how (steps)
• Concurrent Execution: Set the ground work for running tests in parallel

Changes:

• Convert existing test scenarios to Gherkin feature files
• Implement reusable step definitions in steps/steps.go
• Add scenario hooks for setup/teardown and feature gate detection
• Provide comprehensive documentation in test/e2e/README.md
• Remove legacy test files (cluster_extension_install_test.go, etc.)
• Added detailed README covering:
  • Architecture and design patterns
  • How to write new tests
  • Running tests with various options
  • Best practices and troubleshooting
• Go test driving code reduced by ~1000 lines

Migration Notes

• No changes to test behavior or coverage - this is purely a refactoring
• All existing test scenarios are preserved with equivalent Gherkin implementations
• Test execution remains the same via make test-e2e

Assisted-By: Claude noreply@anthropic.com

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign perdasilva for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	c272a60
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/692f04fc4548390008ca34c3
😎 Deploy Preview	https://deploy-preview-2365--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.

[Copilot]

Pull request overview

This PR migrates the e2e test suite from traditional Go testing framework to Godog (BDD/Cucumber framework), enabling behavior-driven development with Gherkin feature files. The migration maintains test coverage while reorganizing tests into feature files with step definitions.

Key Changes

• Replaced traditional Go test functions with Godog scenarios and step definitions
• Added Gherkin .feature files describing test behavior in a more readable format
• Introduced new test infrastructure (steps.go, hooks.go) to support BDD testing

Reviewed changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test/e2e/features_test.go	New test entry point initializing Godog suite with scenario and suite initializers
test/e2e/features/steps/steps.go	Implements step definitions mapping Gherkin steps to Go functions
test/e2e/features/steps/hooks.go	Provides scenario lifecycle hooks and feature gate detection
test/e2e/features/*.feature	Gherkin feature files defining test scenarios (install, update, recover, metrics)
test/e2e/features/steps/testdata/*.yaml	YAML templates for test resources (catalogs, RBAC)
test/e2e/*_test.go	Removed traditional test files migrated to feature files
test/e2e/network_policy_test.go	Added client initialization and helper function
go.mod, go.sum	Added Cucumber/Godog dependencies

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.38%. Comparing base (34394ce) to head (c272a60).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2365      +/-   ##
==========================================
+ Coverage   70.61%   74.38%   +3.77%     
==========================================
  Files          93       93              
  Lines        7333     7333              
==========================================
+ Hits         5178     5455     +277     
+ Misses       1720     1443     -277     
  Partials      435      435              

Flag	Coverage Δ
e2e	43.66% <ø> (-0.88%)	⬇️
experimental-e2e	47.93% <ø> (+33.97%)	⬆️
unit	58.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.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 22 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Potential nil pointer dereference when extracting stderr from error. If the error is not of type *exec.ExitError, this will panic. Consider adding a nil check:

waitFor(ctx, func() bool {
    _, err := kubectlWithInput(yamlContent, "apply", "-f", "-")
    if err == nil {
        return false
    }
    var exitErr *exec.ExitError
    if errors.As(err, &exitErr) && strings.Contains(string(exitErr.Stderr), errMsg) {
        return true
    }
    return false
})

Suggested change

	if stdErr := string(func() *exec.ExitError {
	target := &exec.ExitError{}
	_ = errors.As(err, &target)
	return target
	}().Stderr); !strings.Contains(stdErr, errMsg) {
	var exitErr *exec.ExitError
	if errors.As(err, &exitErr) {
	stdErr := string(exitErr.Stderr)
	if !strings.Contains(stdErr, errMsg) {
	return false
	}
	return true
	}
	return false

[Copilot]

Potential nil pointer dereference when asserting error type. If the error is not of type *exec.ExitError, this will panic when accessing .Stderr. Consider checking if the type assertion succeeded:

if _, err := kubectl("delete", r.kind, r.name, "-n", sc.namespace); err != nil {
    var exitErr *exec.ExitError
    stderr := ""
    if errors.As(err, &exitErr) {
        stderr = string(exitErr.Stderr)
    }
    logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", stderr)
}

[Copilot]

Potential nil pointer dereference. If ctx.Value(scenarioContextKey) returns nil, this will panic. Consider adding a nil check and returning a helpful error:

func scenarioCtx(ctx context.Context) *scenarioContext {
    val := ctx.Value(scenarioContextKey)
    if val == nil {
        panic("scenario context not found in context")
    }
    sc, ok := val.(*scenarioContext)
    if !ok {
        panic("scenario context has wrong type")
    }
    return sc
}

Suggested change

	return ctx.Value(scenarioContextKey).(*scenarioContext)
	val := ctx.Value(scenarioContextKey)
	if val == nil {
	panic("scenario context not found in context")
	}
	sc, ok := val.(*scenarioContext)
	if !ok {
	panic("scenario context has wrong type")
	}
	return sc

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Potential nil pointer dereference. If errors.As(err, &target) returns false, target will be nil and accessing target.Stderr will cause a panic.

Suggested fix:

var stderrStr string
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
    stderrStr = string(exitErr.Stderr)
}
logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", stderrStr)

Suggested change

	logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", string(func() *exec.ExitError {
	target := &exec.ExitError{}
	_ = errors.As(err, &target)
	return target
	}().Stderr))
	var stderrStr string
	var exitErr *exec.ExitError
	if errors.As(err, &exitErr) {
	stderrStr = string(exitErr.Stderr)
	}
	logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", stderrStr)

[joelanford]

Looks like a really nice improvement overall. Just some minor comments/questions.

[joelanford]

Is this the same image we've always used?

I recall there being issues in the past with rate limiting from Docker Hub.

[pedjak]

Yes, that is the one.

[dtfranz]

I think we were hitting rate-limiting because we had the image untagged/set to latest, so it would pull every time no matter what. This is tagged so it should be fine.

[joelanford]

Unrelated to this PR, but something we need to fix separately. This message makes it sound like 1.2.0 just doesn't exist. But it does. It just isn't a successor of the currently installed version.

[joelanford]

Avoid new zap dependency? I think we're using klog in our main.go's. Can we use that here too?

[joelanford]

Kill and wait processes concurrently? Or does order matter?

[pedjak]

Actually I think we do not need to wait at all.

[joelanford]

Same here: Can we delete objects concurrently?

[pedjak]

We could try, but not sure what we are gonna gain with it?

[joelanford]

Deletion can sometimes take a little while if finalizers need to be processed. I assume we want foreground deletion so that we can be sure cleanup is complete before we move on. Seems like it could speed up cleanup considerably in those cases.

[pedjak]

Sure, we could do it.

[joelanford]

I wonder if we could strategic merge patch instead and have a smaller yaml to make it more clear what's changing?

[tmshort]

That would be an improvement.

[pedjak]

I wonder if we could strategic merge patch instead and have a smaller yaml to make it more clear what's changing?

We could ofcourse, we craft the grammar and the semantic: how would like to look like? If we would writing user docs, how this should be read by users?

[tmshort]

Just nothing the inconsistency here:

• resource is applied
  vs
• ClusterExtension is applied

The first one is generic, the second one is indicating a very specific resource. What's the reasoning behind this?

[pedjak]

The first one is generic, the second one is indicating a very specific resource. What's the reasoning behind this?

Improved readability/focus on what matters. The fact that we use the same go code under the hood for both step is not important here - the reader should immediately understand what a step is about. Hence, we could even replace the generic step "resource is applied" with something like ([[:alnum:]]+) is applied or even ([[:alnum:]]+) is available so that we better document what is going on. Also, in this particular case, we could even create very concrete step namespace ([[:alnum:]]+) is available that is going to assure that the given namespace is created if not exists already.

[tmshort]

That would be an improvement.

[tmshort]

There's no distinction here between namespace-scoped resources (e.g. SAs), and cluster-scoped resources (e.g. CE). 'kubectl' may not complain about the -n argument for cluster-scoped resources, but it's basically wrong.

[tmshort]

I could see us wanting to expand this list, or even to make this a bit more dynamic. But probably not now.

[tmshort]

Also noting an inconsistency in substitution mechanisms. Here it's $VAR, where as at: https://github.com/operator-framework/operator-controller/pull/2365/files#diff-37528e433a53ab946ef66fda327001b3a125c05c7ac9dfd2b49529fbfdc50cd3R378 it's {var}

[pedjak]

Also noting an inconsistency in substitution mechanisms. Here it's $VAR, where as at: https://github.com/operator-framework/operator-controller/pull/2365/files#diff-37528e433a53ab946ef66fda327001b3a125c05c7ac9dfd2b49529fbfdc50cd3R378 it's {var}

IMO, bash-style like variables are understandable/known for a wider audience. We could use those in testdata templates as well.

[tmshort]

Since we run these tests in other environments, we might need to consider the use of oc as a substitute for kubectl. Here and elsewhere.

[pedjak]

added --k8s.cli command arg that can tweak that.

[jianzhangbjz]

Hi team, what’s the reason for choosing Cucumber instead of Ginkgo? What’s the background behind this decision? Thanks! I’m asking because the OpenShift QE team used the Cucumber framework for E2E testing before 2020, but it took the team almost a year to fully migrate to Ginkgo. The main reasons for choosing Ginkgo include:

1. Go-Native design, which fits better with Go-based projects.
2. Expressive BDD-Style Syntax that is cleaner and easier to maintain.
3. More powerful and stable test organization compared to Cucumber.
4. Rich Assertion Library (Gomega) that simplifies writing test expectations.
5. Built-in Parallelization for faster test execution.
6. Better for Unit and Integration Tests
7. Widely adopted in the Kubernetes ecosystem, making it a de-facto standard.
8. Progressive enhancement, with active development and community support.

[pedjak]

Hi team, what’s the reason for choosing Cucumber instead of Ginkgo?

@jianzhangbjz thanks for reaching out. I think I have summarized the motivation in the PR description, let me know if some of the points you raised are unanswered. IMO Ginko for e2e is still less readable for non-devs (and for devs as well)

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The feature tag @BoxcutterRuntime is used in the update.feature file but is not documented in the README.md. The README mentions @WebhookProviderCertManager and @SingleOwnNamespaceInstallSupport as examples of feature tags, but @BoxcutterRuntime is missing.

Consider adding this tag to the documentation or verifying that it's initialized in the featureGates map in hooks.go.

Suggested change

	Scenario: Install operator having webhooks
	Scenario: Install operator having webhooks
	@BoxcutterRuntime
	Scenario: Test Boxcutter runtime feature

[Copilot]

The BeforeSuite function silently returns on errors without logging or reporting them. This could make it difficult to debug issues during test initialization, especially if feature gate detection fails.

Consider logging errors before returning:

func BeforeSuite() {
    logger = zap.New(zap.UseFlagOptions(&logOpts))
    
    raw, err := kubectl("get", "deployment", "-n", olmNamespace, olmDeploymentName, "-o", "json")
    if err != nil {
        logger.Error(err, "Failed to get OLM deployment for feature gate detection")
        return
    }
    d := &v1.Deployment{}
    if err := json.Unmarshal([]byte(raw), d); err != nil {
        logger.Error(err, "Failed to unmarshal OLM deployment")
        return
    }
    // ... rest of function
}

Suggested change

	return
	}
	d := &v1.Deployment{}
	if err := json.Unmarshal([]byte(raw), d); err != nil {
	logger.Error(err, "Failed to get OLM deployment for feature gate detection")
	return
	}
	if err := json.Unmarshal([]byte(raw), d); err != nil {
	logger.Error(err, "Failed to unmarshal OLM deployment")

[Copilot]

Potential nil pointer dereference when extracting error from errors.As. If errors.As returns false (err is not an ExitError), the anonymous function returns nil, and accessing .Stderr will cause a panic.

Consider this safer approach:

waitFor(ctx, func() bool {
    _, err := kubectlWithInput(yamlContent, "apply", "-f", "-")
    if err == nil {
        return false
    }
    var exitErr *exec.ExitError
    if errors.As(err, &exitErr) {
        return strings.Contains(string(exitErr.Stderr), errMsg)
    }
    return false
})

Suggested change

	if stdErr := string(func() *exec.ExitError {
	target := &exec.ExitError{}
	_ = errors.As(err, &target)
	return target
	}().Stderr); !strings.Contains(stdErr, errMsg) {
	return false
	}
	return true
	var exitErr *exec.ExitError
	if errors.As(err, &exitErr) {
	if !strings.Contains(string(exitErr.Stderr), errMsg) {
	return false
	}
	return true
	}
	return false

[Copilot]

Potential nil pointer dereference when extracting error from errors.As. If errors.As returns false (err is not an ExitError), the anonymous function returns nil, and accessing .Stderr will cause a panic.

Consider this safer approach:

if _, err := kubectl("delete", r.kind, r.name, "-n", sc.namespace); err != nil {
    var exitErr *exec.ExitError
    if errors.As(err, &exitErr) {
        logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", string(exitErr.Stderr))
    } else {
        logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "error", err.Error())
    }
}

Suggested change

	logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", string(func() *exec.ExitError {
	target := &exec.ExitError{}
	_ = errors.As(err, &target)
	return target
	}().Stderr))
	}
	var exitErr *exec.ExitError
	if errors.As(err, &exitErr) {
	logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "stderr", string(exitErr.Stderr))
	} else {
	logger.Info("Error deleting resource", "name", r.name, "namespace", sc.namespace, "error", err.Error())
	}