feat(e2e): add internal/harness and move fluentbit-multitenant onto it - #2304

Good shape overall, and §5.5 is the right place to stop for review. Three things.

Start vs a harness-led builder. I would rather the harness led the configuration than take a Config struct, since it now owns the whole lifecycle. harness.New(t).WithNamespace(...).Start() reads better at 17 call sites than a struct literal, and it lets a suite add one option without touching Config.

On the verb, though: Build would misname it. Start creates a KIND cluster, helm-installs the operator and registers teardown. Build reads as pure construction, so a reader would not expect a cluster to exist afterwards. Builder for the config, Start for the side effect.

The teardown order is the one thing worth a unit test, and it is the one thing untested. harness.go:128 says the order is load-bearing and registers four t.Cleanups back to front. That order is exactly what leaked clusters before, and harness_test.go covers buildScheme, dumpNamespaces and artifactPath instead — private helpers that were never going to break.

Worth extracting the registration so it takes a recorder, then asserting two things: the order, and that the cluster delete still runs when an earlier step fails. The cluster and helm paths genuinely need a real cluster, so leaving those to the e2e run is right.

equivalence_test.go earns its place, but needs an expiry. It is the §6 safety net from the plan, and #2299 shipped two drift bugs that this would have caught at unit speed rather than after a 20 minute cluster run. So I would keep it.

The cost is that it pins common as the oracle, so common cannot be deleted while it exists — the same coupling as fixture_test.go importing common. That is fine as scaffolding and not fine as a permanent fixture. The plan says "delete these once the suite is migrated"; worth stating that condition in the PR body so it does not quietly become permanent.

One more, and it is the same argument as the lifecycle one: the readiness ladder should move onto the harness too.

running := []struct{ what string; cond func() bool }{ ... }
require.Eventually(t, func() bool { for _, step := range running { ... } })

That shape is in 14 of the 13 suites' tests today (fluentd-aggregator twice), and if it stays in the suite it gets reimplemented 12 more times as they migrate. env.WaitForRunning(...) writes the short-circuit, the per-step logging and the budget once.

Three things it should fix while moving, not just relocate:

Conditions should be values, not pre-bound closures. wait.AnyPodShouldBeRunning(t, env.Client, ...) takes t and the client at construction, so a condition cannot be declared as a reusable package-level value — every suite must rebuild it. If it returned a spec that the harness evaluates with its own t and client, then wait.OperatorRunning(release), wait.ProducerRunning(labels) and wait.AggregatorRunning(ns) become named things suites reference rather than assemble.

The harness should own the budget. The census counts 17 hand-written 5*time.Minute, 3*time.Second pairs. Env already knows t, so it can derive from t.Deadline() and divide across steps, which is also the esReadyBudget bug we are fixing separately: one wait must not be allowed to spend the whole package budget.

Name the outstanding step in the failure, not just the log. On timeout require.Eventually reports Condition never satisfied, and which of the four was unmet only exists in a t.Logf further up the output. Have the harness keep the last unmet step and put it in the assertion message, so a red run names the aggregator that never came up.

Rough target, for discussion rather than as a spec:

env.WaitForRunning(
    wait.Operator(release),
    wait.Producer(producerLabels),
    wait.Aggregator(nsInfra),
    wait.Aggregator(nsTenant),
)

Worth settling now, since §5.5 exists to get this API right before the other twelve suites copy it.

Four things, all on the new API rather than the behaviour.

Drop the display string from the conditions. PodRunning("the operator", client.MatchingLabels{types.NameLabel: release}) makes every call site pass prose plus a selector, and the prose reads badly in the failure. Let each condition know its own name and its own selector, and take only the part that varies:

env.WaitForRunning(
    wait.Operator(release),
    wait.Producer(producerLabels),
    wait.FluentdAggregator(nsInfra),
    wait.FluentdAggregator(nsTenant),
)

Name then comes from the constructor, not the caller: operator, producer, fluentd aggregator in infra. No suite can word it differently, and still waiting for fluentd aggregator in infra is already the sentence you want. PodRunning(name, opts...) stays as the escape hatch for one-offs.

That also removes the comment you could not parse. This one:

// Aggregator matches the fluentd statefulset's pods in one namespace, which is
// what a multi-tenant suite waits on per tenant.

is hard to read because Aggregator is ambiguous — this repo has two, and the comment is carrying the disambiguation the name should. Split it into wait.FluentdAggregator(ns) and wait.SyslogNGAggregator(ns), each with the right labels baked in, and the comment is unnecessary. A name that needs a sentence to explain which of two things it means is the wrong name.

registerTeardown is missing a type, which is why it takes two functions. cleanup func(func()) and onPanic func(step string, v any) are both injected only so the test can observe them. Give the steps a type and the injection disappears:

type teardown []step

func (td teardown) register(t *testing.T) {
    for _, s := range slices.Backward(td) {
        t.Cleanup(func() {
            defer func() {
                if v := recover(); v != nil {
                    assert.Fail(t, fmt.Sprintf("teardown step %q panicked: %v", s.name, v))
                }
            }()
            s.run()
        })
    }
}

One parameter, no seams invented for the test. Both current tests still work: register inside a t.Run subtest, let it finish, then assert the recorded order and that the panicking step did not stop the rest. You are testing t.Cleanup's real LIFO behaviour rather than a stand-in for it, which is strictly better since LIFO is the thing the comment calls load-bearing.

More generally, the function passing through this file is a symptom worth watching: cleanup, onPanic, teardownSteps(...) variadic funcs, and the setup.LoggingOperatorOptionFunc wrappers. Each one is a small hole punched for a test. A type usually closes several at once.

Comments are over the line again. Same note as before: a terse why only, and only where the code cannot say it. Several of the new ones restate the next statement, and two of them (the aggregator one above, the LIFO one) are compensating for names rather than explaining a trade-off.