structwalk: pin the shadowed-embed double-visit bug in unit tests - #6478
Draft
denik wants to merge 5 commits into
Draft
structwalk: pin the shadowed-embed double-visit bug in unit tests#6478denik wants to merge 5 commits into
denik wants to merge 5 commits into
Conversation
resources.Pipeline embeds BaseResource and pipelines.CreatePipeline, both declaring json:"id". resources.Cluster embeds BaseResource.Lifecycle and its own Lifecycle *LifecycleWithStarted, both resolving to json path "lifecycle.prevent_destroy". encoding/json resolves this by using only the shallower field (direct named field beats promoted embedded one); structwalk visits both. Unit test (libs/structs/structwalk/shadow_test.go): TestShadowedEmbedIsVisitedTwice — walks a resources.Cluster with both shadowed fields non-zero and logs that lifecycle.prevent_destroy is visited twice. Asserts structwalk and encoding/json agree about which paths exist (they don't, and the assertion records that mismatch as the current behaviour). TestShadowedEmbedCausesStructdiffDuplicate — diffs two resources.Pipeline values with both id fields non-zero. structdiff receives the id path twice (once from each shadowed embed). prepareChanges in the direct engine maps changes by path string, so the second entry silently overwrites the first; which value wins is implementation-order-dependent. The test asserts that "id" currently appears twice in the raw diff output, so a fix must remove it. Acceptance test (acceptance/bundle/resources/clusters/shadow_lifecycle/): bundle plan -o json on a cluster with lifecycle.prevent_destroy: true shows prevent_destroy_in_state: "ABSENT" — the field is not in ClusterState (PrepareState only copies lifecycle.started), so the direct engine cannot detect future changes to it. The golden pins that absence; when the bug is fixed, prevent_destroy would appear in the state and this golden needs updating. Both tests are meant to FAIL when the bug is fixed, forcing an update. Co-authored-by: Isaac
The previous script used jq to extract specific fields and format them as a custom object. That obscured the actual plan output and made it hard to see what was and wasn't tracked. Now the script feeds the full cluster plan entry straight into the golden, so it is clear what new_state.value carries. lifecycle.prevent_destroy is absent from new_state.value because ClusterState only tracks lifecycle.started; the golden makes that visible at a glance.
lifecycle.prevent_destroy is intentionally absent from state — it is a deploy-time guard that tells the CLI to refuse to destroy the resource, not a field the remote API tracks. An acceptance test asserting its absence was testing correct behavior, not a bug. The two unit tests remain: they pin the actual double-visit behavior in structwalk and structdiff over config types that have shadowed embedded fields.
denik
changed the title
- slices.Sort instead of sort.Strings (repo convention) - nolint:staticcheck on the explicit embedded-field selectors — the test intentionally addresses the shadowed fields by their full qualified names to set both independently; the shorter form would silently hit only one of them
The test previously imported bundle/config/resources and the SDK's
pipelines package to get the real embedding shapes. A change to those
types could break this test for unrelated reasons.
Replaced with local exported struct types (Shadow*) that mirror the exact
embedding patterns, with comments pointing to the originals:
ShadowPipeline ← resources.Pipeline: ShadowBase + ShadowSDKPipeline,
both with json:"id" → ambiguous, dropped by encoding/json
ShadowCluster ← resources.Cluster: ShadowBase.Lifecycle + named
Cluster.Lifecycle, both at json:"lifecycle.prevent_destroy"
Also strengthened the first test: the assertions now record the exact
buggy visit counts (2 for both cases), so the tests pass with the bug
present and must be updated when the bug is fixed, matching the intent of
TestShadowedEmbedCausesStructdiffDuplicate.
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
1 participant
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.
Two unit tests in
libs/structs/structwalk/shadow_test.gothat pin the current (buggy) behaviour and must be updated when the bug is fixed.TestShadowedEmbedIsVisitedTwiceWalks a
resources.Clusterwith bothBaseResource.LifecycleandCluster.Lifecycle *LifecycleWithStartedpopulated. Both resolve tolifecycle.prevent_destroy; structwalk visits that path twice whileencoding/jsonuses only the shallower (direct named) field. The test asserts structwalk and json agree — they don't, and the assertion records the mismatch as the expected current state.TestShadowedEmbedCausesStructdiffDuplicateDiffs two
resources.Pipelinevalues with bothBaseResource.IDandCreatePipeline.Idnon-zero ("old-base-id"vs"new-base-id","old-sdk-id"vs"new-sdk-id"). Both fields carryjson:"id"at the same embedding depth: ambiguous toencoding/json(neither serialized), visited twice bystructdiff. The raw diff slice containsidtwice;prepareChangesin the direct engine silently overwrites the first entry with the second via a map, so which value wins is order-dependent. The test assertsidcurrently appears twice — the fix must make this list empty.Note:
lifecycle.prevent_destroyis intentionally absent from state (ClusterStateonly trackslifecycle.started). An acceptance test asserting its absence inbundle plan -o jsonwas testing correct behavior, not a bug, and was removed.This pull request and its description were written by Isaac.