refactor(e2e): add internal/fixture with the CR and tenancy builders - #2299
Conversation
Wave B of kube-logging#2291: a new package, no suite touched yet. Builders for Logging, FluentdSpec, Buffer, HTTPOutput and Flow, with the defaults counted across the suites rather than chosen. Workers 2 (four sites against three using 1), Timekey 1s (eight against three and one), TimekeyWait 0s (nine against three), and the 500m/200M + 250m/50M resources used by twelve of the CR call sites. Anything that diverges passes an option. The package does not import common. The image constants are duplicated for as long as both exist, and a test asserts they stay equal so the two cannot drift the way setup.defaultImages and common did over the syslog-ng reloader name. TestBuilderReproducesTheMultiWorkerLiteral asserts the builder produces exactly the Logging fluentd-aggregator writes by hand today, which is the §6 safety net against a default quietly absorbing a divergence. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Ports common.LoggingInfra, LoggingTenant and LoggingRoute as pure builders returning the objects in creation order, so the caller does the creating and the package needs no client, context or testing.T. tenancyFluentdSpec is deliberately separate from FluentdSpec: both tenancy aggregators disable the PVC and request 50m/50M with no limits, where the twelve single-tenant call sites use 500m/200M and 250m/50M. Folding them into one default is the drift §6 warns about, and a test fails if it happens. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
e2e/internal/fixture/tenancy_test.go:78
- This assertion currently expects
WatchNamespacesto be["tenant"]even though the builder is called withnsTenant= "tenant-ns". IfLoggingTenantis corrected to watch the passed tenant namespace, this expectation should match the argument value.
lg := objs[2].(*v1beta1.Logging)
require.Equal(t, TenantRef, lg.Spec.LoggingRef)
require.Equal(t, "tenant-ns", lg.Spec.ControlNamespace)
require.Equal(t, []string{TenantRef}, lg.Spec.WatchNamespaces)
}
e2e/internal/fixture/tenancy.go:125
LoggingTenanttakesnsTenantbut hard-codesWatchNamespacestoTenantRef("tenant"). If a suite uses a tenant namespace name other than "tenant", the tenant Logging will stop watching the namespace where the Flow/Output are created (it will watch "tenant" instead), breaking routing/aggregation.
Spec: v1beta1.LoggingSpec{
LoggingRef: TenantRef,
ControlNamespace: nsTenant,
WatchNamespaces: []string{TenantRef},
FluentdSpec: tenancyFluentdSpec(),
Review feedback on kube-logging#2299. LoggingTenant took nsTenant but set WatchNamespaces to TenantRef, so the aggregator watched a namespace named after the loggingRef instead of the one its Flow and Output are created in. It came over that way from common/helpers.go, which hard-codes "tenant" there and gets away with it only because both multi-tenant suites happen to name the namespace "tenant" too; any other name leaves the Flow never picked up. Nothing imports the package yet and the suites pass "tenant", so the objects built for them are identical either way. The shape test now asserts WatchNamespaces against the Flow's own namespace, and guards that the fixture namespace differs from TenantRef so the assertion cannot go vacuous if the two are ever aligned. Reverting the builder to TenantRef fails it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
Both suppressed comments on the last review were right. Fixed in 96b3131.
The second comment was the more useful half: the test passed Both directions have a negative control:
|
csatib02
left a comment
There was a problem hiding this comment.
Please sweep the code and clean it up as much as you can, there is a lot of unnecessary commenting going on.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
e2e/internal/fixture/fixture_test.go:146
- This literal includes
ObjectMeta.Namespace, butLoggingis cluster-scoped. Keeping a namespace here makes the struct-equality safety net assert an invalid object shape (and would fail against a correctedLogging()builder).
want := &v1beta1.Logging{
ObjectMeta: metav1.ObjectMeta{
Name: "fluentd-aggregator-multiworker-test",
Namespace: ns,
},
Spec: v1beta1.LoggingSpec{
e2e/internal/fixture/fixture_test.go:60
Logging()should not populate.Namespace(Logging is cluster-scoped), so this assertion bakes in an invalid shape and will start failing onceLogging()is fixed to match the CRD scope.
This issue also appears on line 141 of the same file.
func TestLoggingDefaults(t *testing.T) {
l := Logging("ns", "lg")
require.Equal(t, "lg", l.Name)
require.Equal(t, "ns", l.Namespace)
require.Equal(t, "ns", l.Spec.ControlNamespace)
require.True(t, l.Spec.EnableRecreateWorkloadOnImmutableFieldChange)
Review feedback on kube-logging#2299. Logging is scope: Cluster, so ObjectMeta.Namespace is meaningless on it: the API server clears the field for cluster-scoped kinds, which is why the suites have always set it without anything failing. The tenancy builders here already omitted it, so the base builder now matches, and its first parameter is named controlNamespace to say what it actually sets. The literal test drops the same field from its expected object, leaving that the only intended difference from what fluentd-aggregator writes by hand. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review feedback on kube-logging#2299. The comments narrated the restructuring plan the package came from: section numbers, per-suite counts, the history of an earlier drift. None of that means anything to someone reading the code on its own, so it is gone. 78 non-header comment lines down to 46, and what is left is mostly doc comments on exported identifiers. Assertion messages that only restated the expected value are gone with them; require prints the value on failure anyway. The copyright year said 2025 on all six files, which were written in 2026. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Second sweep. 46 non-header comment lines to 11. What went was doc comments restating the signature they sat above, on identifiers whose names already say it: LoggingOption, WithFluentbit, WithFluentd, Workers, Drain, Buffer, BufferOption, Timekey, TimekeyWait, ReceiverURL, HTTPOutput, Flow, LoggingRoute and the two tenancy builders. The package matches the rest of e2e now, where exported identifiers carry no doc comment. What stayed is four points the code cannot state: Logging being cluster-scoped, why FluentdOption is exported, why the image constants are duplicated, and why WatchNamespaces takes a namespace rather than the ref. Three in the tests explain an assertion that would otherwise look arbitrary. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
|
Seems like the ci is failing :/ |
test-e2e-nodeps selects every package in the module and filters the helpers back out, but the filter names common. A package under internal/ is therefore out of its reach, so the e2e job also builds and runs internal/kind and internal/wait today, and internal/fixture from this branch. Harmless in itself, those tests are hermetic and take about a second between them, but it re-runs under a 20 minute cluster job what make test already covers, and every later internal/ package joins them. Matching the directory rather than the prefix, so a suite whose name merely starts with those words is not dropped. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The builders landed in kube-logging#2299 with tests that assert the shape this package chose, not that the choice matches what common creates. A suite moving onto them could stop testing what it used to and still pass.This records what common.LoggingInfra, LoggingTenant and LoggingRoute hand to Create and requires the builders to equal it, object for object and in order. It passes as written, so the builders are a faithful stand-in today. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Adds
e2e/internal/fixture, builders for the CR literals the suites repeat. Nothing imports it yet, so no existing behaviour changes.Logging,FluentdSpec,Buffer,HTTPOutputandFlow, with functional options.LoggingInfra,LoggingTenantandLoggingRouteported fromcommon/helpers.goas pure builders returning the objects in creation order, so the caller creates them and the package needs no client, context ortesting.T.common. The image constants are duplicated while both exist and a test pins them equal, so they cannot drift the way the syslog-ng reloader name did.13 tests, no cluster.
make checkpasses.