Repository navigation
test(components): extract reusable profile chart contract checks - #1059
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the Helm profile meta-chart tests by extracting generic contract checks and rendering helpers from the stable-2026a test package into a new reusable profilecontract package, significantly reducing boilerplate and duplication. The feedback highlights a potential runtime panic in contract.go if s.Components is empty when setting up the invalid install tests, suggesting a guard check to prevent this issue.
VLukiantsev
force-pushed
the
test/components-profile-contract-library-b548435909
branch
from
October 2, 2026 15:36
e4a5717 to
c22047d
Compare
VLukiantsev
marked this pull request as ready for review
October 2, 2026 16:15
VLukiantsev
force-pushed
the
test/components-profile-contract-library-b548435909
branch
from
October 5, 2026 07:53
54c829e to
be440af
Compare
…26a checks Move the checks that hold for every profile meta-chart into a new testonly library, test/helm/profiles/profilecontract. It is driven by a Spec (chart path, profile name, components in render order), and profilecontract.Run runs these checks: - only Component CRs are rendered, in catalog order; - each component's defaults match its standalone chart; - reserved keys, malformed values, unknown components, a wrong platformProfile and xmc-cmp-* release names are rejected; - missing or invalid component files fail; - the catalog is consistent across components/, values and the schema; - profile metadata is consistent. The library also exports render and assertion building blocks. The stable2026a test now holds only stable-2026a expectations: its catalog, the component-specific overrides, falsy overrides, rejected 3P/workload keys, and redpanda defaults. Every previous case is kept. The generic checks now run for every component, not only the one previously picked, and the metadata check also asserts the profile.yaml name. Comment link: #1016 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Split every generic profile contract check and building block into an unexported function that returns an error and a thin exported *testing.T wrapper. Run now iterates error-returning contract cases, so the library's own tests can assert that each check detects the violation it guards against. The exported API, the stable2026a test and its subtest tree are unchanged. RenderDocs now decodes every rendered template, not just templates/components.yaml, so RendersOnlyComponents also catches a profile that emits extra manifests from another template file. ChartDir accepts an absolute chart directory. MatchesStandaloneDefaults now renders the profile with its own values.yaml. It used to pass a nil per-component override, which makes Helm drop the profile's components.<name> defaults, so a profile default that diverged from the standalone chart went unnoticed. Comment link: #1016 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Add profilecontract_test, which tests the library without relying on the stable2026a package: - A positive control runs Run and the exported assertions on an unmodified copy of the stable-2026a chart. - A negative matrix breaks one property per case in a temporary copy and checks that the guarding check reports it: an extra document in components.yaml, an extra non-Component template file, wrong order, profile/standalone default drift, a schema accepting reserved keys or any namespace, a removed release-name guard, a tolerated missing component file, catalog drift, and profile name, minXmcVersion and supportWindow violations. - Every check reports a profile it cannot find or loa 6870 d. - Building blocks are unit-tested against testdata/minichart: ChartDir (both runfiles layouts, ancestor walk, absolute path, not found), LoadProfileWithFileEdit, RenderDocs, DecodeDocs, Lookup, DocByName, error matching and ComponentOverrides. Comment link: #1016 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Run now rejects a Spec with an empty ChartRelPath, ProfileName or Components list up front, with a message naming the field, instead of running checks that would fail obscurely or pass vacuously. The xmc-cmp-* release-name case uses a fixed release name rather than the first component, so an empty catalog can no longer cause an index panic. Comment link: #1059 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
VLukiantsev
force-pushed
the
test/components-profile-contract-library-b548435909
branch
from
October 5, 2026 08:32
be440af to
f772e06
Compare
Replace testdata/minichart/files/x.txt with components/demo/values.yaml, laid out like a profile's component sources, and point the LoadProfileWithFileEdit replace, remove and unknown-file cases at it. The test checks the same behavior as before. TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
janzak-dev
previously approved these changes
Oct 5, 2026
…al callers Remove the ChartDir, LoadChart, DecodeDocs, and AssertErrorContains wrappers and unexport Render and DocName. None of them had a caller outside the package; the library's own tests now use the unexported error-returning variants directly. The remaining exported surface is what profile-specific tests actually need. Comment link: #1059 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
errorContains used to format a single message, "Render() error = %v, want error containing %q", for both a nil error and a mismatched one. Because the message embedded want, a test that matched a substring of it could pass on an unrelated failure. A nil error now reports "render succeeded, want error containing %q" and a mismatch reports "render error = %v, want error containing %q". The four DetectViolations rows that expect an invalid render to be accepted now assert the full "render succeeded" message with the exact expected text. TestErrorContains pins both messages, and a new regression test checks that a profile whose templates fail to parse is never reported as "render succeeded" by RejectsInvalidInstalls. Comment link: #1059 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
janzak-dev
approved these changes
Oct 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts the checks that hold for every profile meta-chart into a reusable, test-only Go library,
test/helm/profiles/profilecontract. The stable-2026a test is reduced to the expectations that only hold for that profile. This is a test-only refactor that addresses review feedback on #1016: it was hard to tell generic assertions from profile-specific ones. No production code changes.The library has its own tests, which show that every generic check fails on a profile that breaks it. Writing them uncovered two blind spots in the checks, both fixed here:
Motivation & Context
#1016 adds the
stable-2026aprofile meta-chart and its tests. In review, those tests mixed generic contract checks (such as "the profile renders only Component CRs" or "the catalog is consistent") with stable-2026a specifics (such as "exactly these three components" or "redpanda'skafkaPortdefault"). #1016 now labels the two groups in one file. This PR completes the split, so a future profile such asstable-2026bgets the whole contract from oneprofilecontract.Runcall. It lands as a separate PR to keep #1016 under the PR size threshold.Key Changes
test/helm/profiles/profilecontract(newgo_library,testonly = True, visible to//test/helm/profiles:__subpackages__):Spec{ChartRelPath, ProfileName, Components}describes a profile chart.Componentslists the catalog in render order.Run(t, spec)runs the generic contract as parallel, table-driven subtests:RendersOnlyComponents: only the catalogComponentCRs are rendered, in order.MatchesStandaloneDefaults: each component renders like its standalone chart.RejectsInvalidInstalls: every component rejects reserved keys (componentClass,chartVersion) and malformeddeletionPolicy/namespace. Unknown components, a wrongplatformProfileandxmc-cmp-*release names are also rejected.ComponentFileErrors: a missing or invalid componentChart.yaml,values.yamlorcomponent.yamlfails with a message naming the file.CatalogIsConsistent.MetadataIsConsistent, which now also asserts that theprofile.yamlname equalsSpec.ProfileName.LoadProfile,LoadProfileWithFileEdit,RenderDocs,DocByName,Lookup,ComponentOverrides,AssertRendersComponents,AssertMatchesStandalone,AssertRenderFails. Helpers with no callers outside the package stay unexported.chartDirresolves the chart from Bazel runfiles, or from the closest ancestor directory under plaingo test. It also accepts an absolute directory.error. The exported*testing.Thelpers are thin wrappers that report those errors, so the checks can be tested directly.RenderDocsdecodes every rendered template, not onlytemplates/components.yaml.RendersOnlyComponentstherefore fails when a profile emits a non-Component manifest from any template file.MatchesStandaloneDefaultsrenders the profile with its ownvalues.yaml. It used to pass a nil per-component override, which makes Helm drop the profile'scomponents.<name>defaults, so a profile default that drifted from the standalone chart went unnoticed.profilecontract_test(newgo_test):A positive control runs
Runand the exported assertions on an unmodified copy of the stable-2026a chart.A 14-row negative matrix copies the chart to a temporary directory, breaks one property per case, and checks that the guarding check reports it:
components.yaml;xmc-cmp-*guard;minXmcVersionandsupportWindowviolations.Each mutation fails the test if its target text is not found, so a case cannot pass vacuously. Rows that expect an invalid render to be accepted assert the full
render succeeded, want error containing "<text>"message, which an unrelated render failure cannot produce. A separate regression test checks this: it breaks the template syntax and confirms thatRejectsInvalidInstallsnever reportsrender succeeded.Every check reports an error for a profile it cannot find or load.
The building blocks are unit-tested against
testdata/minichart:chartDir(both runfiles layouts, ancestor walk, absolute path, not found),LoadProfileWithFileEdit,RenderDocs,decodeDocs,Lookup,DocByName, error matching (exact nil and mismatch messages) andComponentOverrides.test/helm/profiles/stable2026a:profile_chart_test.gonow holds only stable-2026a expectations, plusTestStable2026a_ProfileContract, which callsprofilecontract.Run. The stable-2026a expectations are:imageTagrender;values.yamldefaults.The BUILD target depends on the library.
Reviewer Guide
profilecontract/contract.go:Spec(inrender.go),Run, and the generic checks.profilecontract/catalog.go: catalog and metadata consistency.profilecontract/render.go: chart loading and rendering building blocks.profilecontract/contract_test.go: the positive control and the negative matrix, one broken property per row.profilecontract/render_test.go: building-block unit tests.stable2026a/profile_chart_test.go: what remains specific to stable-2026a.Assumptions
Spec.Verification & Testing
Subtest mapping: 30 leaf cases before → 51 after, and every previous case is kept. The increase comes from running the generic checks per component.
TestProfileChart_*)RendersOnlyCatalogComponentCRs/*(3)ProfileContract/RendersOnlyComponents/*(3) +RendersCatalogWithImageTagMatchesStandaloneComponentCharts/*_defaults(3)ProfileContract/MatchesStandaloneDefaults/<component>(3)MatchesStandaloneComponentCharts/*_overrides(3)OverridesMatchStandalone/*(3)HonorsExplicitFalsyOverrides/*(4)HonorsExplicitFalsyOverrides/*(4)RejectsInvalidInstalls/{componentClass,chartVersion,deletionPolicy,namespace,empty namespace}(5, one component each)ProfileContract/RejectsInvalidInstalls/<case>/<component>(15)RejectsInvalidInstalls/{unknown component,wrong platformProfile,release name}(3)ProfileContract/RejectsInvalidInstalls/*(3)RejectsInvalidInstalls/{workload key,unknown 3P parameter}(2)RejectsInvalidValues/*(2)ComponentFilePresence/{missing,invalid}_*(5, redpanda)ProfileContract/ComponentFileErrors/<case>/<component>(15)ComponentFilePresence/empty_values.yaml_*(2)EmptyValuesFileDefaults/*(2)CatalogIsConsistent,ProfileMetadata_IsConsistentProfileContract/CatalogIsConsistent,ProfileContract/MetadataIsConsistentLibrary coverage (from
profilecontract_testalone, without the stable2026a tests):go test -cover: 95.0% of statements;bazel coverage --combined_report=lcov: 93.9% of lines (401/427).The uncovered lines are mostly the
t.Fatal/t.Errorreporting branches of the exported wrappers. Every generic check's failure path is exercised by the negative matrix.The stable2026a subtest tree is unchanged by the refactor: 62 identical passing test and subtest names before and after.
Commands Executed:
bazel test //test/helm/profiles/... //pkg/components/linter/... //deploy/profiles/... //tools/components/...→ all pass, including//test/helm/profiles/profilecontract:profilecontract_test.go test ./test/helm/profiles/...andgo vet ./test/helm/profiles/...→ ok.helm lint --strict deploy/profiles/stable-2026a→ 0 failed.helm template→ 3ComponentCRs.component-linter --manifests-dir=deploy/profiles/stable-2026a --rules-config=deploy/profiles/stable-2026a/rules.yaml→ exit 0.Diff size vs feat(components): package stable-2026a profile meta-chart #1016 (
./tools/analyze_diff_size.py): 0 production, 1292 test lines (1292 total code), 0 docs.Note
Size exception: this PR exceeds the 1000-line total threshold. All of it is test code under
test/helm/profiles/, with 0 production lines. The library ships together with the tests that prove its checks and its two gap fixes. Splitting them would land checks that have never been shown t 10BC8 o fail.Related Issues / Bugs
Part of bug 548435909
TAG=agy
CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954