Design: a disruptive-operation test DSL for MCPLO e2e tests

gist-disruptive-test-framework-design.md

Design: a disruptive-operation test DSL for MCPLO e2e tests

Supporting design doc for the "e2e: add upgrade test framework" issue. Covers the concrete types, package layout, and scheduling semantics; the issue itself stays at the requirements/acceptance-criteria level and links here.

Principle: hide orchestration, don't reinvent it

Test authors declare what to check, not how to schedule it. A Suite type hides all t.Run/t.Parallel/channel wiring behind a declarative struct -- the same goal Knative's Suite/Operation/BackgroundOperation types serve. The difference from a naive port: the Suite's internals are implemented on top of sigs.k8s.io/e2e-framework (already used by this project's e2e suite) and native Go testing primitives (t.Run, t.Parallel(), plain channels) rather than a competing execution engine or raw unmanaged goroutines.

Suite API

Field names generalize Knative's Installations/Tests split -- this also makes the "install the starting state" phase explicit, which a flat list glossed over. Unlike Knative, the fields are named after the disruption in general (Disrupt/PreDisrupt/PostDisrupt) rather than "upgrade" specifically, so the same Suite type serves chaos/soak scenarios later without renaming.

// Operation produces an e2e-framework Feature. Used for install/disrupt
// actions and for PreDisrupt/PostDisrupt assertions alike. Both the regular
// e2e suite and a disruptive-scenario suite can share the same Operation
// functions.
type Operation = func(ns string) features.Feature

// T is a deliberately restricted view of *testing.T, passed to
// ContinualCheck.Check. It excludes Fatal/Fatalf/FailNow/SkipNow so a single
// missed sample cannot compile-time-allow prematurely ending the polling
// window -- callers can only report via Errorf, never abort via Fatal.
// *testing.T satisfies this interface structurally; no adapter needed.
type T interface {
    Errorf(format string, args ...any)
    Logf(format string, args ...any)
    Helper()
    // [..] more methods will be added as needed (e.g. TempDir)
}

// ContinualCheck runs Setup once (full *testing.T -- a genuine setup failure
// should stop immediately via t.Fatal), then Check on every tick until the
// disruptive operation completes, then does one final Check call.
type ContinualCheck struct {
    Name     string
    Setup    func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context
    Check    func(ctx context.Context, t T, cfg *envconf.Config) // t.Errorf on violation; cannot Fatal
    Interval time.Duration
}

// Installations mirrors Knative's naming: the mechanics of getting the
// cluster into its starting state, then performing the disruption.
type Installations struct {
    Base    []Operation // install the current/base release
    Disrupt []Operation // perform the disruptive operation (e.g. an upgrade)
}

// Tests mirrors Knative's naming: assertions run at each phase.
type Tests struct {
    PreDisrupt  []Operation
    PostDisrupt []Operation
    Continual   []ContinualCheck
}

type Suite struct {
    Installations Installations
    Tests         Tests
}

func (s Suite) Run(t *testing.T, testenv env.Environment) {
    t.Helper()
    ns := s.createNamespace(t) // single namespace for the whole run; t.Cleanup deletes it at the end

    // Base install and PreDisrupt run synchronously and gate everything else -- no t.Parallel() here.
    t.Run("InstallingBase", func(t *testing.T) {
        testenv.Test(t, s.build(s.Installations.Base, ns)...)
    })
    if t.Failed() {
        return // can't even install the base state: don't proceed
    }

    t.Run("PreDisrupt", func(t *testing.T) {
        testenv.Test(t, s.build(s.Tests.PreDisrupt, ns)...)
    })
    if t.Failed() {
        return // baseline invalid: don't start Continual or the disruption
    }

    stopCh := make(chan struct{})

    t.Run("Continual", func(t *testing.T) {
        t.Parallel() // pause this whole group until Suite.Run's own body returns, same as DisruptFlow below --
        // without this, this call would run synchronously and block Suite.Run before DisruptFlow (and its
        // close(stopCh)) ever starts, deadlocking every Continual check against the stop signal it's waiting for.
        for _, c := range s.Tests.Continual {
            c := c
            t.Run(c.Name, func(t *testing.T) {
                t.Parallel() // real subtest goroutine -> t.Fatal/Errorf safe
                runContinualCheck(t, testenv, ns, c, stopCh)
            })
        }
    })

    t.Run("DisruptFlow", func(t *testing.T) {
        t.Parallel()
        defer close(stopCh) // always fires, even if a nested Assess step calls t.FailNow()
        testenv.Test(t, s.build(s.Installations.Disrupt, ns)...)
        testenv.Test(t, s.build(s.Tests.PostDisrupt, ns)...)
    })
}

Usage:

suite := disruptive.Suite{
    Installations: disruptive.Installations{
        Base:    []disruptive.Operation{defaultBaseInstall},
        Disrupt: []disruptive.Operation{PerformUpgradeFeature(installer)},
    },
    Tests: disruptive.Tests{
        PreDisrupt:  []disruptive.Operation{f.HappyPathFeature},
        PostDisrupt: []disruptive.Operation{f.HappyPathFeature},
        Continual: []disruptive.ContinualCheck{
            {
                Name:     "MCPServer stays Ready",
                Setup:    setupProbeServer,
                Check:    assertReady, // signature: func(ctx, disruptive.T, cfg)
                Interval: 2 * time.Second,
            },
        },
    },
}
suite.Run(t, testenv)

Continual checks run as real subtests

Each ContinualCheck runs as its own t.Run subtest with t.Parallel(), not a hand-spawned goroutine, so it participates in the standard Go test lifecycle (reporting, panics, -run filtering) instead of a custom concurrency mechanism. Internally, the framework calls Setup once (against a full *testing.T, since a genuine setup failure should abort immediately), then ticks Check at the configured Interval (against the restricted T interface) until the disruptive operation signals completion, then calls Check one final time so the very last sample lands right at the moment the operation finished. Because Check only sees T, not *testing.T, a bad sample can only report via Errorf -- it can never call Fatal/FailNow and end the polling window early, whether by mistake or convention drift.

disruptive.T compatibility with e2e-framework

cfg *envconf.Config and r *resources.Resources are completely unaffected by the disruptive.T restriction -- they stay the exact concrete types e2e-framework and the rest of test/e2e/framework already use. sigs.k8s.io/e2e-framework's own primitives (e.g. wait.For, conditions.New(r)) don't take *testing.T at all; they return error and let the caller decide how to report it. So disruptive.T doesn't create friction with e2e-framework itself.

The real friction is with this project's own shared helpers in test/e2e/framework/ (WaitForMCPServerCondition, SetupMCPServer, AssertConditionStable, etc.) -- every one of them takes a concrete t *testing.T and calls t.Fatalf internally on failure (verified: all ~15 call sites across helpers.go/assertions.go/k8s.go follow this pattern). A Check typed against disruptive.T cannot pass its t to these directly -- but that's not a new problem the interface introduces: calling a t.Fatalf-based helper from inside a per-tick loop was already semantically wrong (one missed tick would abort the whole continual check and end sampling early), independent of whether the type system allows it.

The fix is to extract the pure condition-checking logic (no t dependency) out of the existing Fatal-based helpers, so both styles share it:

// HasCondition is pure predicate logic, usable from both Fatal-based waits
// and Errorf-based continual checks.
func HasCondition(s *mcpv1alpha1.MCPServer, condType string, status metav1.ConditionStatus) bool {
    for _, c := range s.Status.Conditions {
        if c.Type == condType && c.Status == status {
            return true
        }
    }
    return false
}

// WaitForMCPServerCondition (existing, Fatal-based) now delegates to HasCondition:
func WaitForMCPServerCondition(ctx context.Context, t *testing.T, r *resources.Resources, server *mcpv1alpha1.MCPServer, condType string, status metav1.ConditionStatus, timeout ...time.Duration) {
    // ... wait.For(conditions.New(r).ResourceMatch(server, func(obj k8s.Object) bool {
    //         return HasCondition(obj.(*mcpv1alpha1.MCPServer), condType, status)
    //     }), ...)
    // t.Fatalf(...) on error, unchanged
}

// CheckMCPServerCondition (new, Errorf-based, disruptive.T-compatible) for continual checks:
func CheckMCPServerCondition(ctx context.Context, t disruptive.T, r *resources.Resources, server *mcpv1alpha1.MCPServer, condType string, status metav1.ConditionStatus) {
    var cur mcpv1alpha1.MCPServer
    if err := r.Get(ctx, server.Name, server.Namespace, &cur); err != nil {
        t.Errorf("get MCPServer: %v", err)
        return
    }
    if !HasCondition(&cur, condType, status) {
        t.Errorf("MCPServer %s/%s: expected %s=%s, condition not met", cur.Namespace, cur.Name, condType, status)
    }
}

This gives genuine logic reuse (the same HasCondition predicate backs both the one-shot Fatal-based assertion used in Setup/PreDisrupt/PostDisrupt and the per-tick Errorf-based check used in Continual) without any type incompatibility.

Concretely, Setup and Check deliberately use different helper styles for the same ContinualCheck:

// Setup: full *testing.T. A failure here means the continual check can't even
// start -- Fatal is the correct behavior, so it reuses the EXISTING helper as-is.
func setupProbeServer(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
    ctx = f.SetupMCPServer(ctx, t, cfg, "upgrade-probe", true /* waitForReady, uses WaitForMCPServerCondition internally */)
    return ctx
}

// Check: restricted disruptive.T. Called every tick -- must never abort the
// whole continual check on one bad sample, so it uses the NEW Errorf-based helper.
func assertReady(ctx context.Context, t disruptive.T, cfg *envconf.Config) {
    server := f.ServerFromContext(ctx)
    f.CheckMCPServerCondition(ctx, t, cfg.Client().Resources(), server, "Ready", metav1.ConditionTrue)
}

Namespace and environment lifecycle

The upgrade package gets its own TestMain, deliberately without the regular suite's per-test BeforeEachTest/AfterEachTest namespace automation (that model tears down a namespace after every testenv.Test() call, which would race a Continual check that must survive the whole disruption window). Instead:

• Suite.Run creates one namespace up front and captures it by closure into both the PreDisrupt/DisruptFlow track and each ContinualCheck.
• Cleanup is registered via t.Cleanup() on the top-level t, which Go guarantees runs only after every subtest (including the parallel Continual/DisruptFlow tracks) has returned.

Pluggable installation and upgrade

Base install and the disruptive operation itself are pluggable, not hardcoded to any specific mechanism or API version transition:

type Installer interface {
    Install(ctx context.Context, cfg *envconf.Config) error
}

Upstream defaults, for the upgrade scenario:

• Base: resolves the latest stable GitHub release and applies its install.yaml.
• Upgrade: builds the operator image from the current working tree, loads it into Kind, applies CRDs/controller from current code.

Anti-false-green requirement: the default Installer implementations must guarantee the base and upgrade images/versions are actually distinct (e.g. a content/SHA-based tag for the working-tree build, not the shared example.com/mcp-lifecycle-operator:e2e tag the regular suite reuses). Include a sanity-check step after the upgrade runs that asserts the controller's reported version/image actually changed, to catch silent no-op "upgrades" in CI.

Vendor distributions swap Installer implementations (OLM, a parent operator) without touching Suite, Operation, or ContinualCheck definitions. Any Installer implementation is free to shell out to make or other tooling internally -- only the Suite/Operation/ContinualCheck orchestration around it needs to stay plain Go.

Reusing regular e2e tests

Operation functions are shared between the regular suite and the disruptive-scenario suite. Concretely: extract the features.New()...Feature() assembly currently inline in each Test* function (e.g. the body of TestMCPServerHappyPath) into namespace-parameterized builders in test/e2e/framework/ (e.g. f.HappyPathFeature(ns string) features.Feature). Both e2e and e2e/upgrade call the same builder; neither references the other's package-level testenv.

Package layout

The generic scheduling primitives (Suite, Installations, Tests, Operation, ContinualCheck, Installer, T) live in test/framework/disruptive/, independent of any specific scenario. The upgrade scenario itself -- the concrete Installer implementations, ContinualCheck definitions, and the Suite wiring that uses them -- lives in test/e2e/upgrade/, which imports test/framework/disruptive and has its own TestMain:

• test/framework/disruptive/: reusable across scenarios (upgrade now, chaos/soak later); no MCPServer- or upgrade-specific knowledge.
• test/e2e/upgrade/: the actual upgrade test binary; owns its own TestMain, namespace lifecycle, and scenario-specific Installer/ContinualCheck implementations.

Reasons for keeping the scenario binary itself separate from the regular test/e2e/ suite:

• The regular suite runs all tests when no profile filter is specified; a test that swaps the operator mid-run would be destructive if included there.
• The disruptive-scenario namespace/environment lifecycle is fundamentally different (one namespace for the whole run, no per-test teardown), which doesn't fit the regular suite's TestMain.
• A separate binary makes it easy for vendor distributions to plug in their own upgrade/downgrade machinery.

Some of the existing test/e2e/framework/ helpers (HasCondition, CheckMCPServerCondition, etc.) may also end up better organized into their own subpackages as this grows; not required for this issue, left as a follow-up.

Dedicated Makefile targets and build tag

test/e2e/upgrade/ builds under its own Go build tag (e2e_upgrade), separate from the regular suite's e2e tag, so it can never be pulled in by go test -tags=e2e ./... or any other build-tag filter the regular suite uses:

//go:build e2e_upgrade

package upgrade

.PHONY: test-e2e-upgrade
test-e2e-upgrade: ## Run upgrade e2e tests (requires Kind cluster)
	go test -tags=e2e_upgrade ./test/e2e/upgrade/ -v -count=1 -timeout 1h

Opt-in only, never triggered by unfiltered test-e2e.