CMP-4630: Fix stale ResultAfterRemediation carryover in assertion generator - #84
Merged
yuumasato merged 1 commit intoSep 2, 2026
Conversation
…erator GenerateAssertionFileFromResults declared a single RuleTest struct outside the per-rule loop and mutated it in place. ResultAfterRemediation was only assigned when a rule's final result differed from its initial result, and it was never reset between iterations, so a rule whose result did not change inherited the previous rule's ResultAfterRemediation. Because Go map iteration order is randomized, the corruption was nondeterministic: it hid real regressions (FAIL->FAIL shown as PASS-after-remediation) and invented false INCONSISTENT entries in the generated <profile>.yml assertion files. Move the RuleTest declaration inside the loop so each rule gets a fresh struct, and add a regression test that interleaves changed and unchanged rules and asserts unchanged rules carry no result_after_remediation. Also wire a non-cluster unit-test path, mirroring compliance-operator: add TESTABLE_PKGS (packages with tests, excluding vendor and the root e2e package) and a `make test-unit` target so the regression test can run in CI without a live cluster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
Author
|
/test e2e-aws-openshift-platform-compliance |
Member
Author
|
I think the |
taimurhafeez
approved these changes
Sep 1, 2026
Collaborator
|
/retest-required |
Collaborator
|
/test e2e-aws-openshift-node-compliance |
|
@yuumasato: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Member
Author
|
The platform-compliance test is being fixed here: #86 |
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
2 participants
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.
What
GenerateAssertionFileFromResultsdeclared a singleRuleTeststruct outside the per-rule loop and mutated it in place.ResultAfterRemediationwas only assigned when a rule's final result differed from its initial result, and it was never reset between iterations — so a rule whose result did not change inherited the previous rule'sResultAfterRemediation.Because Go map iteration order is randomized, the corruption was nondeterministic. In practice it:
FAILbefore and after remediation were written out as PASS-after-remediation, andINCONSISTENTentries in the generated<profile>.ymlassertion files.Fix
RuleTestdeclaration inside the loop so each rule gets a fresh struct.helpers/utilities_test.go— a regression test that interleaves changed and unchanged rules (with many rules, to beat the randomized map order) and asserts unchanged rules carry noresult_after_remediationwhile changed rules record exactly their own final result. It fails on the old code and passes on the fixed code.CI wiring
There was previously no CI job running unit tests — the presubmits only run live-cluster e2e jobs (and
verifyruns golangci-lint). Mirroringcompliance-operator, this PR adds:TESTABLE_PKGS(packages that have test files, excluding vendor and the root e2e package) and amake test-unittarget.A companion
openshift/releasePR adds a non-clusterunitpresubmit (make test-unit) so this regression test runs on every PR.Test
🤖 Generated with Claude Code