Auto test every field mutation - #6452
Draft
denik wants to merge 131 commits into
Draft
Conversation
Recreating a resource re-keys the parent but not its permissions/grants child: the child's state entry keeps the old id, DoRead echoes it back, and the planner re-proposes the same update on every deploy. The ACL itself is applied to the recreated object, so this is stale bookkeeping rather than lost access, and terraform converges -- hence the per-engine plan files. Co-authored-by: Isaac
model_with_permissions.yml.tmpl was model.yml.tmpl plus a permissions block, so the two tested the same resource twice. Invariant configs carry permissions or grants by default; a permissions-free variant is not a separate case. continue_293 now excludes the model config outright, since model permissions did not work until 0.297.0. Co-authored-by: Isaac
Moves every field of every single-resource invariant config through each ordered pair of a small value set -- absent included, so add and remove are just the pairs with absent on one side -- and records whether the plan proposed anything, the apply succeeded, and the next plan was clean. Runs in-process against CalculatePlan/Apply rather than shelling out to bundle deploy, so 5502 observations across 22 resource types take 35s instead of being dominated by bundle file sync. Permissions and grants are stripped first; a bad outcome does not fail the test, it lands in out.<resource_type>.fields.txt, and the committed goldens are what turn an SDK bump into a visible diff. Co-authored-by: Isaac
Every field of catalog.UpdateSchema is omitempty and DoUpdate forwards only the config's own ForceSendFields, so removing the comment sends an empty PATCH body and UC keeps the old value. The deploy reports success and every later plan re-proposes the same update. Co-authored-by: Isaac
The earlier approach: "# ACTION:" annotations in the invariant configs drive apply_update and apply_remote_update, which plan, apply and re-plan one field at a time through the full CLI. apply_update is now covered far more thoroughly by bundle/direct/dresources_test. apply_remote_update is not -- it drifts the remote by restoring a state snapshot, which the Go catalog does not do yet -- so this is kept for that half. Drop the commit if the remote case lands there instead. Co-authored-by: Isaac
A harness rebuilt inside a field-level subtest goes on being used by later fields, but it captured that subtest's t.Context(), which is cancelled the moment the subtest returns -- 56 observations came back as "context canceled" from the SDK rate limiter rather than a real verdict. Thread the resource-type context in instead. With those unmasked, model serving telemetry turned out to be rejected 36 times: the block only validates against a catalog and schema that exist and are writable. Skip the subtree, which needed skip keys to support a "<prefix>.*" form. Co-authored-by: Isaac
…text cloud: true was the wrong default -- a resource type should be covered everywhere, and the expensive ones still have to be covered. Replaced with slow: true, which only drops a type under -short: the PR cloud leg skips them, the nightly does not. Files that said nothing but cloud: true are gone. t.Context() is cancelled just before Cleanup functions run, so every harness destroy was issuing its API calls on a dead context and silently leaking the resource; a harness rebuilt inside a field subtest hit the same thing once that subtest returned. context.WithoutCancel keeps the values and drops the deadline. Reports move to output/, so the package directory reads as seven entries instead of thirty. Co-authored-by: Isaac
It drives CalculatePlan and Apply, so it tests the engine rather than any one resource implementation, and it belongs beside the engine instead of under dresources. The old dresources_test directory name also misapplied Go's _test package-name convention to a directory. Value libraries move to testdata/fields, which the go tool ignores by name. Goldens stay in output/. Every file in the package is a _test.go, so GoFiles is empty and none of this reaches the binary. Co-authored-by: Isaac
The committed output/<type>.txt now lists just the results worth looking at, one line each, with from and to as their own columns. output/<type>.full.txt keeps everything plus the summary and the not-covered list, and is gitignored since it moves whenever a passing row does. 304 findings out of 5442 observations. A SUPPRESSED whose reason is "empty" is not one of them: both sides were empty, so nothing the user expressed was lost. DRIFT becomes POST_DEPLOY_DRIFT, which is what it measures. Transition subtests are named absent_to_true rather than with an arrow, so a single case re-runs without shell quoting, and now -v prints the post-deploy plan behind any finding. Dropped the slow flag: every resource type runs everywhere. Renaming an app is a real operation, so it is no longer skipped for being slow. It records DEPLOY_ERROR "timed out: app is deleting", which is the finding. A per-operation deadline backs that up with a TIMEOUT verdict. Main fixed clearing a schema comment, so that test becomes clear_field: it now asserts comment converges and records that custom_max_retention_hours does not. Co-authored-by: Isaac
They were committed by accident: output/<type>.txt is the file worth diffing. Co-authored-by: Isaac
They were committed by accident: output/<type>.txt is the file worth diffing. Co-authored-by: Isaac
Slices and maps are covered two ways. The container becomes a field whose values are the config's own and that value minus its last entry, so with absent in the set one field covers adding and removing an entry as well as the whole container -- all with data the backend already accepted. Separately, a pattern like tasks[*].description is expanded against the deployed config to the indices that exist, so fields inside an element are tested like any other. A pattern with nothing behind it is reported as not covered rather than silently passing. base: in a value library seeds a block the invariant config does not declare, so its fields become reachable. A job's git_source needs a provider, a url and exactly one ref, and cannot be built up one field at a time from nothing; seeded, its skip list drops from nine entries to the two that are genuinely a different side of the same oneof. Value pairs are now walked as a single chain instead of staged one at a time. The values form a complete digraph, so one Eulerian circuit covers every ordered pair exactly once and each move starts where the last ended -- one deploy per transition instead of two, which took jobs from 381s to 233s. Walk order is shuffled from a seed derived from the field path: fixed per field so goldens stay byte-stable, different between fields so the suite is not always taking the same path. TestTransitionsCoverEveryPairInOneChain is what guarantees nothing is left for a second chain to pick up. Configs of a resource type now run in parallel, so the report is guarded. out.full.txt indents the evidence under each finding: the post-deploy plan for drift, the whole API error for a rejection. Co-authored-by: Isaac
One config per resource type -- the simplest one -- and whatever structure that config lacks is seeded through testdata/fields instead. A slice or map is a field in its own right, whose values are the config's own and that value minus its last entry, so with absent in the set one field covers adding and removing an entry as well as the whole container. Patterns like tasks[*].description expand against the deployed config, so fields inside an element are tested like any other, and what no config declares is named rather than passed over. Findings are classified until each verdict names one mechanism: UPDATE_IGNORED (write accepted, remote unmoved on two reads) separated from STALE_READ (unmoved on the first read only), COLLATERAL_DRIFT (updating this field left another one drifting -- 119 rows were one field wearing other fields' names), BASELINE_DRIFT (drifts with nothing changed), and OK_INERT / INERT_NOT_HONOURED, which check the ignore_local_changes declarations rather than trusting them. Order comes from git HEAD so a finding cannot be retried away, and the pairs are walked as one Eulerian circuit, halving the deploys. Three bugs in the harness, each of which quietly under-reported coverage: map wildcards never expanded, since splitPattern emitted the wildcard twice; the not-covered list was a union across configs, so it named fields other configs were testing; and neither a map key nor a slice element could be removed, since dyn's path parser reads brackets as indices and deletePath only handled keys. libs/testserver grows SettleAsyncImmediately, off by default so acceptance keeps exercising the waiters. The SDK's poller has a hardcoded one-second backoff with no exported way to shorten it, so one in-progress read per update cost serving endpoints more wall time than every other resource type combined: 512s to 5s. 7052 observations over 22 resource types in 76s. Co-authored-by: Isaac
The generic "x"/"y" defaults are rejected outright on a real workspace for a field that names another object, and the retention period is validated in days, so those fields reported nothing but BACKEND_ERROR on cloud. Co-authored-by: Isaac
FindStructFieldByKeyType already recursed into embedded structs, but Get and Set stopped after one level, so a field of resources.PostgresProject -- which embeds a config struct that embeds the SDK spec -- was reported as not found. Both now walk embedding recursively and track the struct that declares the field, which is also the one whose ForceSendFields governs it: an outer struct that shadows the name (PostgresProjectConfig) tracks only its own fields. Co-authored-by: Isaac
The suite now writes values through libs/structs (structaccess over the resource's own Go type, structpath for paths) and syncs the result into the dynamic tree the way a mutator does, instead of manipulating dyn.Value directly. Nothing in the package depends on libs/dyn any more. That also removes every hand-written substitute for something dyn lacked: a path parser that could hold a map key, a delete-by-path, a parent-creating walk. And it makes "absent" the distinction the API actually sees -- the zero value with the field out of ForceSendFields -- so a list element or map entry now becomes settable where it previously reported UNSETTABLE: 91 of those are gone and the suite records 252 more observations, in a third of the wall time. The order seed is a constant now rather than HEAD. Some verdicts depend on the order -- a field the API cannot clear leaves the remote holding an old value -- and committing a report changes HEAD, so a HEAD-derived seed could never validate at the commit that carried the report. Co-authored-by: Isaac
MLflow refuses to clear a description, and the update sends the field unconditionally, so a config that drops it fails on a real workspace while the fake server accepted it. readplan-permissions changed the description instead of removing it, since removing it can no longer converge. Co-authored-by: Isaac
From the AWS comparison: a skip key naming a field inside a slice
("aliases[*].id") never matched the concrete "aliases[0].id" it expands to, so the
field was tested anyway and the backend rejected every value.
Keys are now matched the way the planner matches its own field rules, which also
makes naming a block skip everything beneath it -- so the ".*" suffix is gone.
Co-authored-by: Isaac
From the AWS comparison: an alias belongs to a model version and is managed through its own API, so UC does not echo aliases on a plain GET -- which is why the direct engine passes include_aliases=false and declares the field input_only. The fake server returned them anyway, so the remote appeared to hold what the config asked for and the field's real behaviour was hidden. registered_models now matches AWS row for row. Co-authored-by: Isaac
From the AWS comparison: an experiment name is an absolute workspace path and an artifact location needs a scheme, so the generic "x" was rejected outright. The UC trace location needs its catalog and schema together, like a job's git_source, so it is seeded rather than built up one field at a time. experiments matches AWS row for row. Co-authored-by: Isaac
…on values
From the AWS comparison. A retention period is validated in days (0, or 7 to 30), so
the generic 1 and 2 were rejected. The rest need workspace state this suite does not
provision: a Delta Sharing provider and share, a connection, an external location, a
customer-managed key.
options is skipped for a different reason worth noting: UpdateCatalog rejects the
field outright ("UpdateCatalog options can not be provided"), and the engine sends it
on every update whenever the config declares it -- so seeding it made every other
field of the catalog fail its update too.
catalogs matches AWS row for row.
Co-authored-by: Isaac
From the AWS comparison: external_locations cannot deploy against a real workspace at all -- it needs a storage credential with cloud IAM behind it -- so every field reported one BASE_ERROR and the golden diverged for a reason that says nothing about the engine. The invariant suite already excludes the same config from its cloud run. A type now declares "local_only: <reason>" in its value library and is skipped on cloud; its local golden stands, since a cloud run neither confirms nor contradicts it. Co-authored-by: Isaac
Two fixes from the AWS comparison, both about the suite rather than the engine.
An alert or dashboard is created inside ${workspace.resource_path} and the backend
404s on a missing parent, which is why a real deploy runs deploy.ResourcePathMkdir
first. This suite plans and applies directly, so it now runs that step too --
otherwise the whole type reported one BASE_ERROR that said nothing about any field.
A seeded value naming tester@databricks.com only exists on the fake server, so the
base fragment is now expanded with the same $VARS the corpus configs use.
Also: a base error's evidence went through firstError, which truncates to 140
characters -- so the full report, whose whole purpose is to carry the backend's own
words, showed a cut-off message.
Co-authored-by: Isaac
…brary From the AWS comparison. An enum field was getting the generic "x" and "y", which a real backend rejects or silently ignores, so the field reported nothing about whether the engine handles a change to it -- the alerts report was full of those. Every generated SDK enum declares its own values through a Values() method, so they are read from there instead, which fixes enum fields across every resource type. The alerts library also skips what the suite cannot provision: a second warehouse or workspace folder (and the backend refuses to move an alert between folders at all), and a registered notification destination. A subscription and a run_as now name the workspace's own user. Co-authored-by: Isaac
…texts Both from the adversarial review. containerValues recorded a slice or map with its elements shallow-copied, so the pointers inside an element were shared with the live resource: editing a field under clusters.init_scripts[0].workspace then reached into the value saved for the init_scripts container's own transitions, and that container was tested against corrupted data. A plan the caller never applies was dropping its pendingApply on the floor, leaking the operation context and its ten-minute timer -- thousands of them on a cloud run. Such a caller now goes through readPlan, and the two paths that plan and then return early cancel explicitly. Co-authored-by: Isaac
…_REACHED From the adversarial review: a transition assumed its setup deploy established the starting value, checking only that the API did not error. When the API accepts a write and ignores it -- clearing a field it refuses to clear -- the field kept its old value and the move was then recorded under a label describing something that never happened: "absent to 168" while the remote still held 720. The starting value is now confirmed before the move under test. A transition that cannot be set up is retried once on a fresh resource, since a resource created without the field genuinely starts absent, and only then recorded as START_NOT_REACHED -- its own verdict, because unlike BASE_ERROR nothing was rejected. Also from the review: isRequired compared a concrete path against the generated required-field data, which is keyed by pattern, so a required field inside a slice (tasks[*].task_key) looked optional and got an absent transition no user can deploy. Co-authored-by: Isaac
Both from the adversarial review. The committed report listed only findings, so a change in passing behaviour was invisible: a field that starts being recreated instead of updated moved one OK to OK_RECREATE and nothing in the compared file mentioned either. That matters most for the cloud run, which is held to the same golden -- a real workspace behaving unlike the fake server in a passing row could not fail the test. The counts of every verdict are now part of the committed file. A missing golden was also treated as an empty one, so adding a resource type without generating its report, or deleting a report, silently passed. Co-authored-by: Isaac
… lists From the adversarial review: a field that ended up with no transitions disappeared without a trace -- no verdict, no coverage gap. Two shapes did this. A type with no generic value (an `any` field like serialized_dashboard) got none, and a required field the library gives a single value has no second value to move to and no absent to move from, so volumes catalog_name, schema_name and volume_type ran nothing while counting as covered. Both are reported as not covered now. While there: a list of scalars the config leaves empty had nothing to trim, so the whole field and everything under it was a gap. Two elements of its own type need no invention and cover adding, growing, shrinking and removing -- 75 gaps closed and 444 more observations. Co-authored-by: Isaac
From the adversarial review, which found the verdict documented but never produced. I first implemented it as "the path does not exist in the adapter's state type", and it was unsound: secret_scopes renames its fields in PrepareState (name to scope, backend_type to scope_backend_type), so three fields that deploy fine -- OK_RECREATE on every transition -- were relabelled as never reaching the API. A name-identity check is not a reachability check. There is no sound way to derive this from the types: what makes an alert's file_path invisible is that the engine reads the file during initialize and its contents become other fields, so the path itself is genuinely absent from the state and NOT_OBSERVABLE is accurate. Documented that case there and removed the verdict. Co-authored-by: Isaac
From the AWS comparison: an AZURE_KEYVAULT scope is backed by a real vault, so the API rejects one created without its metadata. The fake server accepted it, so the suite reported the backend type as freely settable. secret_scopes now matches AWS row for row. Co-authored-by: Isaac
Three files moved under the branch. One needed a real decision: structaccess/get.go -- main replaced .Interface().([]string) with reflect.TypeAssert in code this branch had already deleted, so the branch's version stands and main's form is applied to the three sites the branch still has. Consistent with the same change elsewhere in libs/structs. Rebasing was the wrong shape here: the conflict lands in a function the branch rewrites several times over, so it recurs commit by commit with no intermediate state worth resolving. One golden moves, in the branch's favour. #6441 masks a Lakebase map field as a whole rather than per entry, which is exactly the bug the catalog had recorded: four postgres_projects custom_tags rows go from BASE_ERROR to OK. Co-authored-by: Isaac
#6443 masks a Lakebase oneof member under its group name, and #6089 adds ./task doctest. The first lands on rows this catalog was already recording as errors: nine of them, across postgres_projects, postgres_branches and postgres_endpoints, become OK or SUPPRESSED. Which is the point of the exercise -- those rows named the bug in the API's own words ("Unknown field path in update_mask") and now name its absence. Co-authored-by: Isaac
email_notifications was in the serving endpoint's base for no reason: the leaves are reachable without it, because the suite grows the container itself. Measured -- 32 field rows either way; the only difference was one BASELINE_DRIFT row. Seeding it made the endpoint drift from the moment it existed, and every other field for that type was then measured against a resource already pending. The behaviour it exposed is real and stays recorded, in acceptance/bundle/resources/model_serving_endpoints/update/email-notifications, where the create is shown not applying the notifications and the update applying them. Also fixes eleven comments across seven fixtures that still explained themselves in terms of "the corpus config". The catalog stopped reading the invariant corpus when the fixtures were folded, so those sentences described a mechanism that no longer exists -- including a skip whose stated reason was that a corpus config "is not the simplest one", a rule that no longer exists either. Co-authored-by: Isaac
…rest output/ carried both the goldens and the full reports, with the latter gitignored by pattern -- so 34 tracked files sat among 91, and telling them apart meant knowing the pattern. Now output/ is exactly the 34 goldens and logs/ is ignored wholesale, which is a rule rather than a pattern to remember. The full report is written straight to logs/<type>.full.txt, and its previous copy is kept as .backup: reading what a change did to a report means diffing the two, and the full report is not in git to diff against. Co-authored-by: Isaac
…not testable
The cloud run could not create a secret at all: "Cannot create 1 Secret(s) in Schema ... (estimated
count: 100, limit: 100)". The limit is per schema and main.default on a shared workspace is full of
other runs' leftovers, the oldest from three weeks ago. The fixture now creates its own schema as a
dep, which is empty by construction.
That exposed the next layer: catalog_name and schema_name name objects that have to exist, so the
generic x and y are refused ("Catalog 'x' does not exist", "Schema 'main.x' does not exist"). The fake
server takes any string, so those four rows had only ever measured the fake. Skipped with the reason.
secrets now matches on cloud.
Co-authored-by: Isaac
… and a clear the API drops
budget_policy_id names an account-level policy ("Failed to validate budget policy x"), which a test
workspace does not have and cannot be given from here. Skipped, as on apps.
target_qps is omitempty, so a config that clears it drops it from the body and the backend keeps the
value it had. The fake server was assigning the zero value instead, which made the field look freely
clearable; two rows now record the clear being ignored, as the cloud run does. Setting and changing
the value still works on both.
Co-authored-by: Isaac
…suite You were right that a poller was the cost. The fake server returns RUNNING on the first poll of a run and the terminal state on the second, and the engine's waiter uses the SDK's backoff -- attempt times a second, plus jitter -- so every run costs about a second of sleeping. A job run is immutable, so 109 of its 131 observations recreate it: roughly 110 runs, roughly 110 seconds. job_runs was 183s of the suite's 185s. SettleAsyncImmediately already existed for exactly this and job runs did not consult it. Now they do: the run is terminated and reported terminal on the first poll. job_runs 183s -> 2s, the whole suite 185s -> 11s. Verdicts are unchanged, which is the point -- the wait was pure latency. The acceptance suite leaves the simulation on, so the waiter and its state logging stay covered there. My first attempt at this made it slower, 183s -> 261s, because it skipped the whole block including the call that terminates the run: the run then stayed RUNNING forever and every create waited out the cap instead of one backoff. Measured both ways before believing either. Also adds a header to each committed report, since it is read in a diff with nothing around it to say what the columns are or that a bad outcome here is recorded rather than failed. No counts, times or dates in it: a header that moved when something unrelated changed would be noise in every review. Co-authored-by: Isaac
… the backend keeps
Removing email_notifications from base cost more than the drift row it saved: with no value to clone,
the container's values came from the generic slice defaults, so the suite sent one- and two-element
lists of "x" and "y" and the API refused all 22 rows ("Invalid email format: x"). The list needed its
own declared values, not just the address inside it.
Declared alphabetically, because the backend returns the list sorted. The first attempt used
[notify, alerts] and every two-address list read back as [alerts, notify], so the endpoint drifted on
the ordering alone, for good -- a real property, and one that makes the field untestable if the
fixture fights it. Written down where the values are.
Co-authored-by: Isaac
…to be real
The clusters cloud run failed at the create: "Init scripts failed ... Tree node with path
/Users/<user>/init.sh does not exist". Three fixtures named that path -- clusters, jobs, pipelines --
and nothing ever created the file; the mock server accepts any path, so it had never mattered. They now
point at a real script the bundle uploads.
It lives in the suite's own testdata/data, not the shared invariant data directory: everything there is
uploaded by every invariant config too, so putting it there changed "Files: 2 uploaded" to 3 across
that whole suite. Found by running it.
policy_id has to name a cluster policy that exists ("'x' is not a valid cluster policy ID"), so its six
rows had only ever measured the fake. Skipped with the reason.
Also stops the previous commit's .backup files landing in output/, which is meant to hold only what is
committed -- and had duly committed 34 of them. They belong beside the logs they are backups of, where
git is not already the history.
Co-authored-by: Isaac
… be real
The clusters cloud run failed at the create even after the previous commit pointed the init script at
the file the bundle uploads. The reason is in the harness's own doc comment: this suite deliberately
uploads nothing, because a sync per permutation would dominate the run. So no destination a fixture
could name is ever there, and the mock server's acceptance of any path is the only thing that made the
field look testable. init_scripts is skipped in all three fixtures that seeded it, with that reason,
and the data directory the previous commit added is gone with it.
user_api_scopes needed real OAuth scopes: the API refuses anything else outright ("The specified scope
x is not a valid scope"), and the mock server takes any string, so eight rows had only measured the
fake. sql and dashboards.genie both verified against the API, declared for the list as well as the
element.
Co-authored-by: Isaac
…s reason The cloud runs found 400 diverging rows across jobs, pipelines, apps and clusters, and every one is a field the mock server accepts and a real workspace does not. Skipped, one entry per field or block, with the API's own words in the comment above it. Two shapes: Rejected outright -- a cross-field constraint (a job's continuous block against the seeded trigger, an environment spec that needs exactly two of three, a shared job cluster needing more than one task), a value format (metadata and notebook paths must be absolute, a cluster label must be default or maintenance, a webhook id must be a UUID), or a field that does not apply to the resource this fixture builds (is_single_node and use_ml_runtime need a cluster kind; an event log needs a UC pipeline). Accepted and then ignored -- jobs' email_notifications, edit_mode, format, health.rules, max_concurrent_runs, trigger.pause_status; pipelines' dry_run, maven libraries, some cluster attributes. These are the interesting ones: the API takes the write and drops it while the mock server applies it, so a local row said OK where a workspace says nothing happened. Modelling each in libs/testserver is the real fix, and the skip reason records the finding until then. vector_search_indexes keeps one field and skips the rest. Every field of an index is immutable, so each transition recreates it behind an endpoint that has to be provisioned first: 102 rows took three hours and then hit the test timeout. primary_key is the cheapest that still means something -- required, so two transitions rather than six, both through create, recreate and converge. genie_spaces needed no change. Its title rows diverged because the backend changed mid-sweep: the failing run saw a space default-named New Agent and refused to rename it, and a probe an hour later got New Agent with a timestamp and renamed it fine. It passes on a re-run. Co-authored-by: Isaac
…ce said jobs went from 199 diverging rows to none: - email_notifications was seeded in base and the create does not apply it, so the job drifted from the moment it existed and two rows recorded that rather than a field. Removed from base; the field was already skipped for transitions, so seeding it bought nothing else. - clearing the name does not converge. The CLI defaults a nameless job to Untitled, the remote holds that, and the plan keeps proposing an update; the mock server stores the cleared value and settles. The field's other five transitions are fine on both. pipelines went from 161 to none: notification alerts and recipients are validated (Invalid ... notifications: x), serverless cannot be set on a pipeline that declares clusters, the name cannot be cleared, a parameter is accepted and not applied, and the autoscale mode's drift is attributed to num_workers, which cannot move with it. sql_warehouses had one row: a new warehouse is STARTING rather than RUNNING, so a config asking for started: false is already satisfied on cloud and suppressed, where the mock server plans a stop. Modelling it needs the mock server to move a warehouse from STARTING to RUNNING across reads, which the engine's waiter depends on, so this one is timing rather than behaviour. Co-authored-by: Isaac
The autotest package is already in the three integration tasks, with no -sample, so a PR's integration run was attempting the full cloud sweep inside a 2h timeout -- and the full sweep takes longer than that. Sampling by default fixes it; running everything has to stay possible, which is what this marker is for. A cloud run now tests two fields per type unless the commit title contains AUTOTEST_ALL, and every field when it does. Locally nothing changes: a full run is 11 seconds, so there is nothing to save, and ./task test keeps comparing the committed goldens in full. -sample still overrides both, and the nightly task passes -sample 0 explicitly now that the cloud default is not "all". Co-authored-by: Isaac
A commit whose title carries the marker, so the gate is exercised the way a PR would use it: this commit's own integration run should drive every field rather than a sample. Co-authored-by: Isaac
Down from 51 diverging rows to none: - schema is refused on a storage-backed pipeline, which base declares; the API points at its migrate-to-dpm documentation. - both autoscale bounds have their drift attributed to num_workers, which base sets and no transition moves with them -- the same reason autoscale.mode was already skipped. - parameters was seeded in base and the create does not apply it, so the pipeline drifted from the moment it existed. Dropped from base; the field was already skipped, so seeding it bought only that drift row. Same shape as jobs' email_notifications. jobs is clean on cloud now, from 199 diverging rows to none. Co-authored-by: Isaac
CI has been red since the commit that taught libs/testserver the SQL warehouse create validations: TestAll/sql_warehouses builds a resource from the zero value, and a warehouse with no name is now refused, as the API refuses it. The fix belongs in the test, which has a per-type table for exactly this: a name, a cluster_size and max_num_clusters, the three the API requires. I should have caught this when the validations landed. I ran the field catalog and the acceptance suite and not the package whose own unit test creates every resource type from nothing. Co-authored-by: Isaac
The PR was CONFLICTING, and GitHub does not run pull_request workflows on a PR whose merge commit it cannot create -- which is why no build had run since 428c5f0, and why the six red builds before that were the last word from CI. Merging main is what unblocks it. set.go conflicted because main made the same assign-before-ForceSendFields fix independently, with a shorter comment and a different internal signature for updateForceSendFields. This branch keeps its own version: it carries that fix plus the breadth-first embedded lookup main does not have, and no test exists only on main. Verified the package builds against main's other files and every structaccess test passes. Co-authored-by: Isaac
…tter CI's check job failed on YAML formatting, which ./task lint does not cover -- ./task fmt does. Ran it; the changes are quoting and blank lines only. docker_image needs custom containers enabled on the workspace, which this one does not have. vector_search_indexes is local_only now. Every field of an index is immutable, so a transition recreates it, and a real workspace deletes an index asynchronously: the create half then waits on "still exists, waiting for deletion to complete" until the transition times out. No field can converge there, whichever one is kept, so the type keeps its one field locally -- where the recreate path is exercised in a second -- and does not run on cloud. That also spares the sweep 70 recreates behind an endpoint that takes five minutes to provision, which is what made this type a three-hour outlier. Co-authored-by: Isaac
…lability "The field 'aws_attributes.availability' cannot be supplied when an instance pool ID is provided", and base names a pool -- the pool decides availability for the instances it hands out. Skipped, the same reason node_type_flexibility already is. The clusters run's other three rows were network timeouts. The title carries AUTOTEST_ALL so this commit's integration run drives every field against a real workspace rather than the two-per-type sample a PR gets by default. Also documents what cost me most in this sweep: the workspace is shared, and running several types at once makes it push back in ways that look exactly like findings. Three types "diverged" under three parallel lanes and were clean serially -- catalogs 5024s then 19s, registered_models 7094s then 14s, experiments 166s then 17s. The runtime is the tell, so the README now says to sweep serially and to read a failure's runtime before believing it. Co-authored-by: Isaac
The gate read only HEAD's subject, which is wrong for the checkout CI is likely to use: a pull request's merge ref has a synthetic "Merge <sha> into <base>" at HEAD, carrying nothing the author wrote. A PR titled AUTOTEST_ALL would then have sampled anyway, silently. It now reads HEAD's subject and, when HEAD is a merge, its parents' too. Verified by building such a merge commit and running against a real workspace from it: the marker is found through the parent. Only a merge's parents are consulted, not the branch's history, so using the marker once does not keep triggering full runs on the commits after it. Co-authored-by: Isaac
…t reuse one A rename recreates the app, and a real workspace holds a deleted app's name for up to twenty minutes while it sits in DELETING. The chain of transitions therefore hits "An app with the same name already exists" the moment it returns to a name an earlier transition used. The mock server deletes outright when this suite asks it to -- which is deliberate, a suite doing thousands of updates cannot wait -- so the chain works locally and cannot on cloud. The run's other five rows were the workspace at its 100-app limit. Co-authored-by: Isaac
denik
changed the title
A row read "cannot update resources.postgres_projects.foo: updating id=projects/test-pg-project-[UNIQU ...ovided in request", which is the question rather than the answer: the reason was "Field 'spec.budget_policy_id' is in update_mask but not provided in request", and the cut fell in the middle of it. The engine's own framing was the thing crowding it out, and it says only what the row already says -- the resource is the report's own filename, the operation follows from the transition. Dropping that prefix and widening the cap to 200 characters leaves every message in the catalog intact: 49 elided rows become 0, and the longest surviving detail is 213 characters. Worth the change beyond legibility. With the message whole, one row now reads "Field 'spec.default_endpoint_settings.suspension' is in update_mask but not provided" against a transition of autoscaling_limit_min_cu -- the engine naming a different field than the one being changed, which the elision had been hiding. Also restores the rune-safe truncation from an earlier commit, lost in a merge resolution: the ellipsis substituted for a URL is multi-byte, so a byte index can split it. Co-authored-by: Isaac
A field the API will not clear leaves the resource drifted, and the suite replaced the resource for it. That was the wrong response twice over. It was wasteful: on apps it cost a fresh app per non-converging field, 31 in one run, on a shared workspace that allows 100 in total. The sweep kept hitting "reached the maximum limit of 100 apps" for no reason but this. And it was wrong. The drift was real, and destroying the resource that carried it meant the next resource had none -- so the drift reappeared later and was attributed to whichever field was under test at the time. database_instances shows it plainly: COLLATERAL_DRIFT 46 rows to 0, OK 2 rows to 26, and capacity now reads UPDATE_IGNORED, which is what the API actually does with it, rather than POST_DEPLOY_DRIFT. A resource is replaced when it will no longer deploy at all -- which is what the original comment was really guarding against, and is rare. Drift on its own goes into the baseline, where remeasure already knew how to put it, and every later field is measured against the drift that is really there. Across the catalog: COLLATERAL_DRIFT 46 to 20. Co-authored-by: Isaac
…n assume it
A failed deploy is not evidence that a resource is unusable -- the API may simply refuse to put one
field back. So ask it something it must be able to answer.
Three outcomes now, and only the last spends a new resource:
- Base comes back. Any remaining drift is real and goes into the baseline.
- Base does not, but the state the transitions left does. The API had already accepted that state, so
a deploy of it succeeds unless the resource itself is broken -- it answered, so the resource is
alive and updatable and only this one field is stuck. Base advances to what is reachable, so later
fields start from a state the API accepts instead of re-failing on this field all run.
- Neither deploys: past repair, replace it.
The probe is a snapshot rather than a reconstruction -- the config as the field's transitions left it is
already the state the remote holds -- so the whole thing is a switch with three arms.
Verdicts improve again, because a discarded resource was taking its drift with it and the next field was
being blamed for it: OK 1486 to 1488, and postgres_projects' BASE_ERROR 14 to 12 with the remaining ones
naming autoscaling_limit_min_cu, the field actually under test, instead of suspension.
Co-authored-by: Isaac
… does "use_ml_runtime is not allowed with unspecified kind" -- the same constraint already recorded for is_single_node, on a fixture whose cluster has no kind. Found by the cloud run; the mock server stores the field either way. Co-authored-by: Isaac
Review found the three-arm switch was doing more than it needed. The first arm deployed, then called converged(), which deployed again -- and converged() answers false for both "the deploy failed" and "the plan is dirty", so a second failure was being recorded as drift. The arms also differed in whether they refreshed the baseline, so a clean restore left stale entries behind, and driftDetail would have suppressed a later drift on one of those paths. It is now one condition and one shared conclusion: try base, fall back to the state the transitions left, rebuild only if neither deploys -- then re-measure the baseline from whatever landed. Always replacing the baseline is what fixes the stale entries, and it is also simpler than deciding when not to. converged() and hasDrift() have no callers left, which is the clearest evidence the shape was wrong: their whole job was the second deploy this no longer does. No golden moves. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: a5a8668
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
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.
Adds
bundle/direct/autotest: a per-field catalog of how the direct engine handles every field a usercan set. For each field it walks every ordered pair of a small value set —
absentincluded, so addingand removing a field are just the pairs with
absenton one side — and records three things per move:did the plan propose a change, did the apply succeed, did the next plan converge.
The outcome is a verdict word in a committed golden. A bad outcome is recorded, not failed, so the
report is a map of the engine's behaviour rather than a wall of failures; what fails the test is a
verdict changing. All 34 resource types the engine supports, ~3900 observations.
Both a local run against
libs/testserverand a real-workspace run are held to the same golden, so adivergence means the fake server does not match the API. That is where most of this PR's findings come
from — 14 fake-server fidelity fixes, each carrying the API's own error text:
cluster_size, andmax_num_clustersin 1..40, and thebackend defaults
enable_photonandspot_instance_policycreated with neither gets storage, and the later update is then refused
email_notifications; an update doesautotermination_minuteshas a 10-minute minimumtarget_qpson a vector search endpoint is omitempty, so the API drops a clearThree of those exposed acceptance fixtures asserting behaviour the API does not have. All were
Cloud = false, so nothing had ever checked them:bind/pipelines/updatebound a storage-defaultedpipeline to a config asking for a catalog;
create-scopesent the bundle schema's field name to a rawAPI call, so the CLI dropped the metadata and the whole script aborted once the fake required it; and
the serving-endpoint fix sharpened a test whose own comment describes a TF bug the fake had been hiding
half of.
Engine findings the goldens record: 27
postgres_projectsrows name a field inupdate_maskand thenomit it from the body (#6441 and #6443 fixed 13 of these while this branch was open);
pipelines.catalogis absent fromrecreate_on_changes, so the engine sends an update the API refuses.Running it
./task testcovers the local run. Two new tasks for the real-workspace runs:./task autotest-cloud— every field; nightly./task autotest-cloud-pr—-sample 2, for PRs: two fields per type, picked from HEAD sosuccessive commits cover different ground while one run's picks follow from its SHA
output/holds the 34 committed goldens and nothing else;logs/holds the full report per type andis gitignored wholesale.
State of the cloud verification
In progress. 14 of the 34 types declare
local_onlywith a reason — an external location needsIAM-backed storage, an instance pool cannot be deleted again, a postgres project holds its name after
deletion, a job run would start a real job per transition. Of the 20 that can be driven, verification
against the current goldens is under way; each one that diverged so far has been a fixture value the
fake accepts and the API refuses, or a fake-server gap, and is fixed in this branch.
Test-only; no changelog fragment.
This pull request and its description were written by Isaac.