[VPEX][7/8] Add local-env acceptance tests#5833
Merged
Merged
Conversation
Collaborator
Integration test reportCommit: 3bde07c
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
This was referenced Jul 6, 2026
rugpanov
force-pushed
the
dbconnect/06-command
branch
from
2fd09fe to
ae5563c
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
6c2fadc to
c45ca0c
Compare
rugpanov
force-pushed
the
dbconnect/06-command
branch
from
ae5563c to
a7fa50c
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
c45ca0c to
25d07fe
Compare
ZaSkittles
pushed a commit
to ZaSkittles/cli
that referenced
this pull request
Jul 7, 2026
databricks#5823) ## Why - The `local-env` feature needs a shared vocabulary before any behavior can be built: the result shape, the error taxonomy, and how a compute target maps to an environment key. - Landing these contract types first lets every later layer (resolve / fetch / merge / pipeline / command) depend on stable, reviewed definitions. - Kept deliberately minimal and dependency-free so it reviews on its own and stays `unused`/`deadcode`-clean with no consumers yet. ## What - **`result.go`** — the `--json` / `E_*` output contract: `Result`, `PipelineError`, `ErrorCode`, `PhaseName`, `PhaseStatus`, `Mode`, `TargetInfo`, `ResolvedInfo`, `Plan`, `Warning`; plus the command-path constants (`local-env` / `python` / `sync`) defined in one place. - **`envkey.go`** — `EnvKeyForServerless` / `EnvKeyForSparkVersion` / `NormalizeServerless`, and `PythonMinorFromRequires` (clause-aware: returns the effective highest lower bound of a `requires-python`). - No wiring into `cmd/`, so the CLI is unchanged. Filesystem/artifact constants and the phase-order slice deliberately live with their consumer (PR 5). ## Testing strategy - Unit tests for the error/type contract (`result_test.go`) and env-key mapping incl. multi-clause / strict-`>` / no-floor `requires-python` cases (`envkey_test.go`). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex across several rounds to convergence (all findings fixed or explicitly rejected as speculative). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | **databricks#5823 ← you are here** | foundation: result types + env-key mapping | | 2 | databricks#5824 | compute-target resolution | | 3 | databricks#5826 | constraint fetch + offline cache | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
ZaSkittles
pushed a commit
to ZaSkittles/cli
that referenced
this pull request
Jul 7, 2026
## Why - `local-env` must turn the user's compute selection into a single environment key before it can fetch anything, and the selection can come from several places with a defined precedence. - Isolating resolution behind a narrow seam keeps it testable without a live workspace and keeps SDK details out of the engine. ## What - **`target.go`** — `ResolveTarget` with ordered precedence `--cluster` → `--serverless` → `--job` → bundle target, producing a `TargetInfo` + env key. - Compute lookups go through the narrow `ComputeClient` interface (stubbable in tests). - `ValidateTargetFlags` rejects more than one target flag; `ResolveTarget` runs it up front so a non-Cobra caller can't silently resolve the wrong target. - Classic-compute jobs read the Spark version from the documented first return of `GetJobSparkVersion` (not the recorded-version third return). ## Testing strategy - Unit tests against a stub `ComputeClient` covering each precedence branch, the mutually-exclusive-flags error, and the job classic-compute contract (`target_test.go`). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass. --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | **databricks#5824 ← you are here** | compute-target resolution | | 3 | databricks#5826 | constraint fetch + offline cache | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
ZaSkittles
pushed a commit
to ZaSkittles/cli
that referenced
this pull request
Jul 7, 2026
…icks#5826) ## Why - Once a target resolves to an env key, `local-env` needs the pinned Python version, `databricks-connect` version, and dependency constraints published for that key. - The fetch must degrade gracefully offline and distinguish “this environment isn't published” from “the network is down,” because those call for different user action. - The artifact host must be a Databricks-owned, access-controlled location and must never default to a personal repo — whoever controls the host controls what the CLI installs. ## What - **`constraints.go`** — fetches the per-environment `pyproject.toml`, parses `requires-python`, the `databricks-connect` pin, and `[tool.uv]` `constraint-dependencies`, and caches it on disk. - **Host parameterization** — no host is hardcoded: `RepoConstraintBaseURL` reads the repo (`owner/name`) from the temporary `DATABRICKS_LOCALENV_CONSTRAINT_REPO` env var and builds a `raw.githubusercontent.com/<repo>/main` URL; the built-in default is empty. When unset it returns `""` and `FetchConstraints` reports the missing source as a fetch-phase `E_FETCH` error (so there is no untrusted default, and the failure flows through the normal phase/JSON reporting). Once `databricks/environments` can publish, that becomes the hardcoded default and the env var is no longer required. - Failure classification: **404** → `E_ENV_UNSUPPORTED` (no cache fallback — a distinct non-transient condition); **transport / non-404** → `E_FETCH` with fallback to the last-good cached copy. - Robustness: validate the body (parse + require `requires-python`) **before** caching so a bad 2xx can't poison the cache; atomic cache write (mkdir + temp-file + rename); dedicated `http.Client` with a 30s timeout; body read bounded by `io.LimitReader` at 1 MiB; `databricks-connect` matched by leading package name under PEP 503 normalization (so `Databricks_Connect` matches, `databricks-connectors` does not); cache filename = readable slug + sha256 suffix to prevent collisions. ## Testing strategy - Unit tests with an `httptest` server: 200-parse, 404 → `E_ENV_UNSUPPORTED`, transport failure + cache fallback, missing-`requires-python` rejection, PEP 503 name matching, cache-dir creation, collision-free filenames, oversized-body rejection (`constraints_test.go`). - Host resolution: `TestRepoConstraintBaseURL` (env var → URL, unset → `""`, whitespace treated as unset) and `TestFetchConstraintsNoSourceConfigured` (empty host → `E_FETCH` naming the env var). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass (several fetch/cache edge-case fixes landed from review). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | databricks#5824 | compute-target resolution | | 3 | **databricks#5826 ← you are here** | constraint fetch + offline cache | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
ZaSkittles
pushed a commit
to ZaSkittles/cli
that referenced
this pull request
Jul 7, 2026
…atabricks#5827) ## Why - `local-env` must apply the resolved Python version and constraints to the user's `pyproject.toml` without disturbing their own content — comments, ordering, formatting, and unrelated config must survive untouched. - Re-running must be safe and idempotent, and a greenfield project needs a sensible file created from scratch. - This is the most intricate logic in the feature, so it lands in its own PR for focused review. ## What - **`merge.go`** — a formatting-preserving merge that rewrites only the env-owned regions (`requires-python`, the `databricks-connect` entry in `[dependency-groups].dev`, and a marker-bracketed managed `[tool.uv]` block) and preserves every other byte incl. CRLF; idempotent. `RenderFreshPyproject` builds a complete managed file for a greenfield project. - Scoping/robustness: managed `constraint-dependencies` nests header-less inside an existing user `[tool.uv]` (never a duplicate header); single- vs multi-line array detection tracks real bracket depth outside strings/comments; the `databricks-connect` rewrite is confined to `dev` and leaves trailing comments alone; `requires-python`'s inline comment is preserved; table-header parsing tolerates inline comments and recognizes `[[array.of.tables]]`. ## Testing strategy - Unit tests that parse the merged output as TOML (not just substring checks), covering idempotency, CRLF preservation, user-key preservation, the duplicate-`[tool.uv]` case, bracket-in-element arrays, sibling-group/comment non-clobbering, and `[[tool.uv.index]]` children (`merge_test.go`). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass (multiple TOML-corruption edge cases were caught and fixed). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | databricks#5824 | compute-target resolution | | 3 | databricks#5826 | constraint fetch + offline cache | | 4 | **databricks#5827 ← you are here** | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
rugpanov
force-pushed
the
dbconnect/06-command
branch
from
a7fa50c to
f1ef8fd
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
25d07fe to
da5bf75
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
da5bf75 to
b537940
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
b537940 to
1b472ab
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
1b472ab to
f450684
Compare
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
f9dd6f6 to
c6972ec
Compare
anton-107
approved these changes
Jul 16, 2026
anton-107
left a comment
anton-107
left a comment
Contributor
There was a problem hiding this comment.
Approving. Clean, idiomatic acceptance coverage — I ran the suite locally and all 12 scenarios pass, including on a synthetic "these tests + #5936's source" branch, so the goldens are robust to merge order. The stubbed endpoints match what the SDK calls, and the error goldens are consistent.
One thing I checked and want to flag as a non-issue for the record: no-target and json-error omit --check, so they hit the real preflight EnsureAvailable. That's fine — the suite already makes uv a hard prerequisite via RequireUV (acceptance_test.go), and discoverUv uses the same PATH lookup, so confirmUvInstall (added in #5936) is never reached in tests and no golden breaks when #5936 lands on top. Verified by running the combined branch.
The main gap is coverage of #5936's new serverless-from-job version path (deriving serverless-vN from environment_version, and the differing-version rejection) — none of the job scenarios here exercise it. I've left the detailed suggestion on #5936; if it's easier to add the scenario in this PR since the fixtures live here, that works too. Not blocking either way.
rugpanov
added a commit
that referenced
this pull request
Jul 16, 2026
…pt, tests Follow-ups from the #5936 review (all non-blocking): - environmentVersion now falls back to the deprecated Spec.Client field when Spec.EnvironmentVersion is empty. client is the predecessor of environment_version and some jobs still pin via it; reading both means the v4 fallback and the differing-version guard observe whichever field carries the pin, instead of treating a client-pinned job as unversioned (which would silently resolve to v4 and let two differing client values slip past the guard). base_environment stays ignored — it is a path/ID, not a version. The divergence error message is now field-agnostic ("differing versions"). - confirmUvInstall's prompt now names the OS-specific installer URL via a shared uvInstallerURL helper (install.ps1 on Windows, install.sh elsewhere), which installUv also uses, so the prompt is transparent about what actually runs. - Tests: environmentVersion field precedence (compute_test.go) and serverless --job version resolution + v4 fallback (target_test.go). The end-to-end differing-versions rejection through GetJobSparkVersion is best exercised as an acceptance scenario alongside the job fixtures in #5833; noted on the PR. Co-authored-by: Isaac
Seventh in the stacked series. End-to-end acceptance coverage for "databricks local-env python sync" against the in-process fake server, which is where the command's user-visible behavior (text + --json output, every error path) is exercised — cmd/localenv itself carries no unit tests by design. Nine scenarios: help (three-level tree), no-target, flag-conflict, manager-unsupported (conda project), env-unsupported (404 -> E_ENV_UNSUPPORTED), json-error, serverless-check (dry-run plan), serverless-json (--json plan), and constraints-only. Scripts drive the hidden command by name; goldens were generated with -update and verified stable on a clean re-run. The --json "command" field and the managed pyproject marker both render as "local-env python sync". No source changes. Co-authored-by: Isaac
Covers the job target paths that had no acceptance coverage: - job-classic-check: a single-cluster classic job resolves to its DBR-derived environment key (happy path). - job-ambiguous-compute: a job with both serverless environments and job clusters is rejected with an actionable disambiguation error. - job-multicluster-mismatch: a job whose clusters declare differing spark_version is rejected rather than silently using the first. Co-authored-by: Isaac
Rebasing onto main picked up the text-mode error dedup that landed with the uv backend: output.go now returns root.ErrAlreadyPrinted instead of the pipeline error, so the phase-loop message is no longer repeated as a trailing "Error: ..." line. Regenerate the six error-path goldens that predated that change; flag-conflict is unaffected because its error comes from Cobra flag validation, not the pipeline. Co-authored-by: Isaac
Covers the serverless-from-job path added in #5936, per the review suggestion on #5833 (the fixtures live here): - job-serverless-check: a serverless job that pins environment_version resolves to that serverless-vN (v3), not the v4 default. - job-serverless-version-mismatch: a job whose serverless environments declare differing versions is rejected with a disambiguation error. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/07-acceptance
branch
from
c6d4042 to
58b80d9
Compare
Contributor
Author
|
Thanks for the review, @anton-107. Addressed your coverage suggestion: now that #5936 has merged to main, its serverless-from-job version path is exercised here (the fixtures live in this PR, as you noted). Added two scenarios:
Also rebased the branch onto fresh main (linear history, includes #5936). The two |
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
--jsonoutput, and every error path — needs end-to-end coverage against the real CLI.cmd/localenv/carries no unit tests by design, so acceptance tests are where that surface is verified (repo convention: user-visible CLI output is covered by acceptance tests).What
acceptance/localenv/— 9 scenarios driven through the (hidden) command against the in-process fake server:help(three-level tree),no-target(E_NO_TARGET),flag-conflict(Cobra mutual-exclusion),manager-unsupported(conda project → clean P1 exit),env-unsupported(404 →E_ENV_UNSUPPORTEDat fetch),json-error(--output jsonerror object),serverless-check(dry-run plan),serverless-json(--jsonplan), andconstraints-only.local-env python syncand theDATABRICKS_LOCALENV_CONSTRAINT_SOURCEoverride; goldens show thelocal-env python synccommand field and managed marker. No source changes.Testing strategy
-updateand verified stable on a clean re-run (no-update); all 9 subtests pass.musterrguards the five expected-failure scenarios;traceshows the three output-producing ones.acceptance/localenv/.About this stack
This is one of a series of small, stacked PRs that together add the
databricks local-env python synccommand — it provisions a local Python environment (Python version,databricks-connectpin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack.Review bottom-up. Layers 1–5 have merged (the original layer-5 PR #5828 was split into 5a/5b/5c during review).
This pull request and its description were written by Isaac.