Add tests for structdiff whole-block behavior - #6566
Merged
Conversation
Collaborator
Integration test reportCommit: 72aa755
Top 6 slowest tests (at least 2 minutes):
|
denik
changed the title
denik
marked this pull request as ready for review
September 9, 2026 12:21
andrewnester
approved these changes
Sep 9, 2026
denik
enabled auto-merge
September 9, 2026 12:34
When a nested struct is nil on one side, structdiff records a single change
at the level where the nil appears instead of descending to the differing
leaves. Because per-field rules and update masks are keyed by path, a change
recorded at a parent path misses any rule targeting a nested field.
Reproducers (all capture current buggy behavior, marked with Badness):
- pipelines/recreate-{add,remove}-whole-block: adding/removing the whole
ingestion_definition block is planned as "update" instead of "recreate";
the immutable recreate rule on ingestion_definition.connection_name never
matches the block-level change path.
- postgres_projects/add_default_endpoint_settings: adding the whole
default_endpoint_settings block masks the enclosing message
(update_mask=spec.default_endpoint_settings), and the deploy fails with a
400 because the API then requires the suspension oneof in the body.
- structdiff.TestGetStructDiffWholeBlock: the root-cause granularity.
- direct.TestPrepareChangesWholeBlockOverlap: the three-way merge emits an
overlapping parent+child entry instead of merging into leaves.
Co-authored-by: Isaac <no-reply@databricks.com>
…tests Replace the inline heredocs with committed templates. The added block is carried as #TO_ADD: comment lines and activated with update_file.py; the removed block is tagged # TO_DELETE and dropped with grep -v. Co-authored-by: Isaac <no-reply@databricks.com>
The direct engine's behavior is only surprising next to terraform's, so run
both and capture the divergent output in per-engine files:
- pipelines/recreate-{add,remove}-whole-block: plan verdict per-engine
(out.plan.{direct,terraform}.txt) — direct "update", terraform "recreate".
- postgres_projects/add_default_endpoint_settings: deploy per-engine
(out.deploy.{direct,terraform}.txt) — direct fails with 400, terraform
succeeds. The plan verdict is identical, so it stays in output.txt.
Co-authored-by: Isaac <no-reply@databricks.com>
For the pipeline tests, keep the direct engine's JSON plan (its per-field "changes" map is the interesting part) and terraform's text verdict; drop the redundant direct text and terraform JSON. Also drop the "cat databricks.yml" traces since the template is committed. Co-authored-by: Isaac <no-reply@databricks.com>
Use plan -o json for both engines instead of branching on the engine name: direct shows action=update plus the whole-block changes entry, terraform shows action=recreate. Co-authored-by: Isaac <no-reply@databricks.com>
jobs/whole-block-overlap manufactures mismatched diff granularity: a local change to schedule.timezone_id (leaf) plus the schedule block dropped on the remote out-of-band (via edit_resource.py). The direct plan's "changes" then carries both the whole "schedule" block and the "schedule.timezone_id" leaf as separate update entries instead of merging into leaves. Co-authored-by: Isaac <no-reply@databricks.com>
The plan verdict is identical on both engines, so output.txt showed nothing structdiff-specific. Capture the plan JSON per-engine instead: the direct "changes" map records default_endpoint_settings as one whole-block entry (the structdiff artifact), and out.deploy.*.txt keeps the consequence (direct 400 vs terraform success). Co-authored-by: Isaac <no-reply@databricks.com>
The testserver's message-mask rejection was hand-coded from a one-time probe; inheriting Cloud = true keeps it honest against the real API. The direct deploy is expected to 400 on cloud (tolerated by errcode); terraform succeeds. Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/diff-whole-block
branch
from
September 9, 2026 12:58
d744b6d to
e42bd11
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
denik
added a commit
that referenced
this pull request
Sep 9, 2026
Growing a config with a nested block — adding default_endpoint_settings with
just autoscaling limits — failed the deploy with
400 INVALID_PARAMETER_VALUE
Field 'spec.default_endpoint_settings.suspension' is in update_mask
but not provided in request
#6440 masks only leaves when a child of the message changed, but a block added
as a whole leaves a single change on the message itself, and that was masked
as-is. Expand such a change to the fields the request body carries.
Two things do not expand. A message the body populates completely, because no
requirement the API places on a masked field can then go unmet and replacing
the message is what the config declares — that keeps spec.settings as the mask
for endpoint settings. And a map, a repeated field or a wrapper like
duration.Duration: the first two are addressable only as a whole, the last is a
struct in Go but a scalar on the wire.
The expansion reads the spec the request body carries rather than the plan's own
copy of the new value, because a plan read back from disk carries that copy as
deserialized JSON with the types erased; deploying a saved plan would otherwise
still send the message path and fail. The READPLAN cell of the acceptance test
covers that path.
This turns the postgres_projects/add_default_endpoint_settings case that #6566
recorded as broken into a passing one; its Badness marker is removed.
Co-authored-by: Isaac
Collaborator
Integration test reportCommit: b506d33
1069 interesting tests: 934 MISS, 134 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
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
3 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.
Why
We have some issues with interaction between planning and classifying fields. They are recorded in the tests below.
pipelines/recreate-{add,remove}-whole-block— adding/removing the wholeingestion_definitionblock is planned asupdateinstead ofrecreate; the immutable recreate rule oningestion_definition.connection_namenever matches the block-level change path. Runs on both engines (directupdatevs terraformrecreate).postgres_projects/add_default_endpoint_settings— adding the wholedefault_endpoint_settingsblock masks the enclosing message (update_mask=spec.default_endpoint_settings), and the direct deploy fails with a 400 (Field 'spec.default_endpoint_settings.suspension' is in update_mask but not provided in request); terraform masks the leaf and succeeds.jobs/whole-block-overlap— the three-way merge in action: a local change to theschedule.timezone_idleaf plus the wholescheduleblock dropped on the remote out-of-band. The plan'schangesthen carries both the wholescheduleblock and theschedule.timezone_idleaf as separateupdateentries instead of merging into leaves.structdiff.TestGetStructDiffWholeBlock— the root-cause granularity.direct.TestPrepareChangesWholeBlockOverlap— the same overlap as a focused unit test.Terraform classifies all of these as expected; the surprising behavior is in the direct engine.
This pull request and its description were written by Isaac.