Skip to content

refactor(e2e): move three more suites onto the harness - #2306

Open
vyncint wants to merge 4 commits into
kube-logging:masterfrom
vyncint:e2e-migrate-batch-1
Open

refactor(e2e): move three more suites onto the harness#2306
vyncint wants to merge 4 commits into
kube-logging:masterfrom
vyncint:e2e-migrate-batch-1

Conversation

@vyncint

@vyncint vyncint commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Three more suites onto the harness, and the three things they needed that it did not have.

suite lines
fluentbit-hotreload 206 → 81
watch-selector 226 → 159
syslog-ng-aggregator 269 → 176

Each addition lands with a caller. wait.SyslogNGAggregator had none until now, and wait.PodRunning still has none by design — it is the escape hatch the named constructors fall back on.

  • wait.Pod(namespace, name). watch-selector waits on two pods by name rather than by selector.
  • Env.Receiver. MustReceive and MustNotReceive, replacing WaitForReceiverLogs. The tail moves inside the type: one value of 100, which is stricter than the 30 an absence check would otherwise get and no weaker for presence, since more lines only make a tag easier to find.
  • WithOperatorArgs already existed and gets its first use in watch-selector.

What was left alone on purpose

watch-selector keeps its Logging literal. fixture.WithFluentbit() would add Network.Keepalive and WithFluentd() would add Resources, BufferVolumeMetrics and Workers: 2, none of which that suite has today. syslog-ng-aggregator keeps its SyslogNGSpec for the same reason: the statefulset overrides and buffer volumes have no builder, and one caller does not justify inventing one.

fluentbit-hotreload's combined predicate is split, MustReceive(tagInfra) then MustNotReceive(tagTenant). Worked through the three orderings — infra first, tenant first, tenant never — the two agree in every case; the split fails immediately rather than after five minutes, and says which half broke.

Two divergences are changed rather than preserved, so they are stated rather than silent: syslog-ng-aggregator polled at 2s and now polls at the shared 3s, and its aggregator check was cluster-wide and is now scoped to the one namespace it uses. Neither can change a verdict. The syslong_ng_aggregator package typo is untouched; renaming belongs with the rest of the naming work.

Verification

make check passes, golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 in e2e/ reports 0 issues, and every commit builds, vets and tests on its own.

Against real clusters: syslog-ng-aggregator 112s and watch-selector 64s both pass.

fluentbit-hotreload does not pass locally, and neither does the version on master. Same failure, same place, 4s apart: the tenant pipeline delivers nothing on this machine — 601 receiver deliveries all tag_infra, zero tag_tenant, against 605/0 for the unmigrated suite. So the migration is not the difference, but the local run is a before/after control rather than a pass. CI is the verdict for that one, where the suite has been green on six consecutive runs.

vyncint added 4 commits August 4, 2026 20:59
watch-selector waits on pods by name rather than by selector, and
fluentbit-hotreload asserts that a tag has not arrived, neither of which the
harness could express.Env.Receiver replaces WaitForReceiverLogs and takes the tail with it: one value
of 100, stricter than the 30 an absence check would otherwise get and no weaker
for presence, since more lines only make a tag easier to find.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
206 lines to 81. The combined predicate becomes MustReceive(tagInfra) then
MustNotReceive(tagTenant); the two agree on every ordering of the tags, and the
split fails immediately rather than after five minutes.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
269 lines to 176, and the first caller of wait.SyslogNGAggregator. The
SyslogNGSpec literal stays: its statefulset overrides and buffer volumes have
no builder, and one caller does not justify inventing one.Two divergences change rather than survive. The poll interval was 2s and is now
the shared 3s, and the aggregator check was cluster-wide and is now scoped to
the one namespace this suite uses. Neither can change a verdict.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
226 lines to 159, and the first use of WithOperatorArgs. The Logging literal
stays: fixture.WithFluentbit would add Network.Keepalive and WithFluentd would
add Resources, BufferVolumeMetrics and Workers, none of which this suite has.

Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
@csatib02

csatib02 commented Aug 4, 2026

Copy link
Copy Markdown
Member

The migration itself looks right and I would take it. What I want to flag is what these three suites reveal about the layer below the harness, because two of them argue against the fixture design and I would rather settle that before nine more suites go through it.

The harness is earning its keep. -248 lines here, -132 before, and it retired three bugs structurally rather than per suite: the teardown panic stranding a cluster, t.FailNow off the test goroutine, and unbounded waits. Nothing in this PR changes my view there. The declared-order and panic tests are the right tests.

But internal/fixture just failed its purpose twice, in this PR. You declined WithFluentbit() in watch-selector and the SyslogNGSpec builder in syslog-ng-aggregator, both for the same reason: the defaults inject fields those suites do not have — Network.Keepalive, Resources, BufferVolumeMetrics, Workers: 2. That was the correct call, and declining it twice out of three suites is the signal. The package is 655 lines with no non-test callers, and both suites that could have used it decided it would change what they test.

The cause is its shape rather than its content. #2291's §6 lists nine fields where suites genuinely diverge. A defaulted builder over that much divergence is either wrong for most callers or needs an option per field, and neither saves anything.

Suggestion: invert it to minimal rather than defaulted. fixture.Logging(ns, name) returns the smallest valid object and nothing else; a suite adds exactly what it needs. Nothing is hidden, so there is nothing to diverge from, and the "a default silently absorbed a divergence" failure mode disappears rather than being avoided by hand each time. Then extract only what genuinely never varies — the image blocks, 181 references by the census, which no suite overrides — and leave the rest as literals.

That also answers the two literals you deliberately kept: under a minimal fixture they stop being exceptions and become the normal way a suite states its own spec.

Thing that is not the fixture's problem, listed so it doesn't get folded in:

watch-selector's installFluentdSts shells out to helm.New(...), the e2e-framework helm binary, which no Makefile installs — an undeclared prerequisite. The harness already installs the operator through the helm SDK, so third-party chart installs belong there too, on the same mechanism. That removes the second helm path rather than moving it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants