integration: add e2e tests for environments setup-local - #6155
Conversation
Approval status: pending
|
Integration test reportCommit: a3549c1
10 interesting tests: 4 SKIP, 3 RECOVERED, 1 KNOWN, 1 FAIL, 1 flaky
|
rclarey
left a comment
rclarey
left a comment
There was a problem hiding this comment.
LGTM, but I didn't run it
## Changes Two related changes to ownership of the DB Connect / local-environments code: 1. **Define `team:ide`** in `.github/OWNERTEAMS` — `@rugpanov @rclarey @anton-107 @misha-db`. 2. **Own `/integration/cmd/environments/`** so the `setup-local` integration tests are reviewed by the same people who own the code they exercise. The three existing localenv paths each repeated the same four handles, so a roster change meant four separate edits that could silently drift apart. They now reference the alias instead: ``` /libs/localenv/ team:ide /cmd/environments/ team:ide /acceptance/localenv/ team:ide /integration/cmd/environments/ team:ide # new ``` **Ownership is unchanged** for the three pre-existing paths — same four people, one source of truth. The rest of `/integration/` stays with `team:platform`. ### Why the new entry sits below `/integration/` `findOwners` is **last-match-wins**. Putting the new rule next to the other localenv lines (line ~17) looks natural but silently does nothing: the broader `/integration/ team:platform` rule at line 65 matches later and wins. My first attempt did exactly that, and `owners.js validate` passed anyway — the misplacement is invisible to the validator. Hence the position after `/integration/`, plus a comment so it doesn't get "tidied" back up. ### No GitHub org team yet There's no `github.com/orgs/databricks/teams/ide` page, so the validator emits its non-blocking `no GitHub team-page URL` warning — the same one `team:ai-training` already produces. Per `validateOwners`, this is by design: *"A team may legitimately predate its GitHub team page, so this never blocks a merge."* The alias is fully functional standalone, which is how the existing aliases already work — neither `cli-maintainers` nor `cli-platform` (both listed in the OWNERTEAMS header) exists as an org team today, and OWNERTEAMS is explicitly the source of truth because `GITHUB_TOKEN` can't resolve org team membership. Creating the real org team needs org-owner access; the header URL can be added later. ## Tests - `node .github/scripts/owners.js validate` → passes (2 non-blocking warnings: the new `team:ide` and the pre-existing `team:ai-training`). - `node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js` → 64/64 pass. - Resolution verified with `findOwners`: | path | owners | | --- | --- | | `libs/localenv/uv.go` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `cmd/environments/setup_local.go` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `acceptance/localenv/…` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `integration/cmd/environments/setup_local_test.go` | `rugpanov rclarey anton-107 misha-db` ✅ new | | `integration/cmd/jobs/foo_test.go` | `team:platform` (unchanged) | ## Note on the base branch Based on `dbconnect/setup-local-integration` (#6155) rather than `main`, because `owners.js validate` errors on rules whose path doesn't exist in the tree, and `integration/cmd/environments/` is created by that PR. Once #6155 lands this can be retargeted to `main`. _This pull request and its description were written by Isaac._
## Changes Two related changes to ownership of the DB Connect / local-environments code: 1. **Define `team:ide`** in `.github/OWNERTEAMS` — `@rugpanov @rclarey @anton-107 @misha-db`. 2. **Own `/integration/cmd/environments/`** so the `setup-local` integration tests are reviewed by the same people who own the code they exercise. The three existing localenv paths each repeated the same four handles, so a roster change meant four separate edits that could silently drift apart. They now reference the alias instead: ``` /libs/localenv/ team:ide /cmd/environments/ team:ide /acceptance/localenv/ team:ide /integration/cmd/environments/ team:ide # new ``` **Ownership is unchanged** for the three pre-existing paths — same four people, one source of truth. The rest of `/integration/` stays with `team:platform`. ### Why the new entry sits below `/integration/` `findOwners` is **last-match-wins**. Putting the new rule next to the other localenv lines (line ~17) looks natural but silently does nothing: the broader `/integration/ team:platform` rule at line 65 matches later and wins. My first attempt did exactly that, and `owners.js validate` passed anyway — the misplacement is invisible to the validator. Hence the position after `/integration/`, plus a comment so it doesn't get "tidied" back up. ### No GitHub org team yet There's no `github.com/orgs/databricks/teams/ide` page, so the validator emits its non-blocking `no GitHub team-page URL` warning — the same one `team:ai-training` already produces. Per `validateOwners`, this is by design: *"A team may legitimately predate its GitHub team page, so this never blocks a merge."* The alias is fully functional standalone, which is how the existing aliases already work — neither `cli-maintainers` nor `cli-platform` (both listed in the OWNERTEAMS header) exists as an org team today, and OWNERTEAMS is explicitly the source of truth because `GITHUB_TOKEN` can't resolve org team membership. Creating the real org team needs org-owner access; the header URL can be added later. ## Tests - `node .github/scripts/owners.js validate` → passes (2 non-blocking warnings: the new `team:ide` and the pre-existing `team:ai-training`). - `node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js` → 64/64 pass. - Resolution verified with `findOwners`: | path | owners | | --- | --- | | `libs/localenv/uv.go` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `cmd/environments/setup_local.go` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `acceptance/localenv/…` | `rugpanov rclarey anton-107 misha-db` (unchanged) | | `integration/cmd/environments/setup_local_test.go` | `rugpanov rclarey anton-107 misha-db` ✅ new | | `integration/cmd/jobs/foo_test.go` | `team:platform` (unchanged) | ## Note on the base branch Based on `dbconnect/setup-local-integration` (#6155) rather than `main`, because `owners.js validate` errors on rules whose path doesn't exist in the tree, and `integration/cmd/environments/` is created by that PR. Once #6155 lands this can be retargeted to `main`. _This pull request and its description were written by Isaac._
2e14b95 to
5c1abb1
Compare
The setup-local pipeline was covered hermetically (unit + acceptance via --dry-run/stubs) and by one env-gated real-provision test in libs/localenv, but nothing exercised the command end to end through the CLI entrypoint in the cli-isolated integration suite. Add integration/cmd/environments so the feature runs in the isolated AWS/Azure/GCP e2e workflow. Three CLOUD_ENV-gated tests, all against the real public databricks/environments repo (serverless needs no running compute — the version is used verbatim): - serverless full provision: resolve -> fetch -> uv sync -> validate, asserting a real .venv/uv.lock and the --output json contract (ok, compute, resolved). - --dry-run writes nothing. - unpublished version -> E_ENV_UNSUPPORTED at the fetch phase with a non-zero exit. They skip cleanly when CLOUD_ENV is unset, so unit-test CI is unaffected. Co-authored-by: Isaac
5c1abb1 to
dcd31eb
Compare
…ricks#6187) ## Changes Defines `team:ide` in `.github/OWNERTEAMS` — `@rugpanov @rclarey @anton-107 @misha-db` — and points the three localenv / DB Connect paths at it: ``` /libs/localenv/ team:ide /cmd/environments/ team:ide /acceptance/localenv/ team:ide ``` Each of those lines previously repeated the same four handles, so a roster change meant three separate edits that could silently drift apart. Now there's one source of truth. **Ownership is unchanged** — all three paths still resolve to the same four people. ### No GitHub org team yet There's no `github.com/orgs/databricks/teams/ide` page, so the validator emits its non-blocking `no GitHub team-page URL` warning — the same one `team:ai-training` already produces. Per `validateOwners` this is by design: *"A team may legitimately predate its GitHub team page, so this never blocks a merge."* The alias is fully functional standalone, which is how the existing aliases already work — neither `cli-maintainers` nor `cli-platform` (both listed in the OWNERTEAMS header) exists as an org team today, and OWNERTEAMS is explicitly the source of truth because `GITHUB_TOKEN` can't resolve org team membership. Creating the real org team needs org-owner access; the header URL can be added later. ## Tests - `node .github/scripts/owners.js validate` → passes (2 non-blocking warnings: the new `team:ide` and the pre-existing `team:ai-training`). - `node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js` → 64/64 pass. - `findOwners` confirms `libs/localenv/uv.go`, `cmd/environments/setup_local.go` and `acceptance/localenv/…` all still resolve to `rugpanov rclarey anton-107 misha-db`. ## Follow-up Ownership of `/integration/cmd/environments/` is intentionally **not** included here: that path doesn't exist on `main` yet (it's added by databricks#6155), and `owners.js validate` errors on rules whose path is missing from the tree. It'll be a small follow-up PR against `main` once databricks#6155 lands. _This pull request and its description were written by Isaac._
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
Nice addition — this fills a real gap: nothing previously drove setup-local through the CLI entrypoint with a real uv sync, and the serverless-only choice correctly avoids a cluster dependency. The assertions are also deliberately loose where the upstream artifact is mutable, which is the right instinct.
Four things I'd fix before merge (inline). The first is the one I care about most — the test installs uv on a developer's machine as a side effect, which the sibling libs/localenv/provision_integration_test.go went to deliberate lengths to avoid.
Nothing here is a correctness bug in the code under test.
On the two dry-run tests overlapping acceptance/localenv/serverless-check and env-unsupported: I'd keep them. The real-network fetch is genuine added value — the unpublished-version test verifies raw.githubusercontent.com actually 404s (rather than serving a 200 HTML page) for a missing key, which the stub server can't check.
|
|
||
| // Let uv bootstrap itself if the runner's PATH lacks it; CI installs uv, but | ||
| // this keeps the test robust on a developer machine that opted in. | ||
| t.Setenv(localenv.EnvAutoInstallUv, "1") |
There was a problem hiding this comment.
This installs uv on a developer's machine as a side effect, and the comment has it backwards: the test sets the opt-in itself, so nobody opted in.
On a machine without uv, EnvAutoInstallUv=1 makes EnsureAvailable run curl -LsSf https://astral.sh/uv/install.sh | sh, mutating ~/.local/bin (libs/localenv/uv.go:363). The sibling test avoids exactly this — requireRealProvision gates on an explicit env var and skips when uv isn't discoverable (provision_integration_test.go:27-34).
Since CI already installs uv (.github/actions/setup-build-environment/action.yml:73), the robustness this buys applies only to the case where the side effect is unwanted. Suggest dropping the Setenv and skipping instead:
if _, err := exec.LookPath("uv"); err != nil {
t.Skipf("uv not found on PATH (%v)", err)
}There was a problem hiding this comment.
Done in 9d9c0ac — replaced with an exec.LookPath("uv") skip. You're right that the comment had it backwards: the test set the opt-in itself, so the "opted in" claim was wrong, and on a machine without uv this ran the remote installer and mutated ~/.local/bin.
| // The artifact came from a successful fetch. The cache is shared (UserCacheDir), | ||
| // so a prior run may have seeded it; accept either source rather than assuming | ||
| // a cold cache and flaking on re-runs. | ||
| assert.Contains(t, []string{"network", "cache"}, res.Resolved.ArtifactSource) |
There was a problem hiding this comment.
This assertion is tautological and can never fail. ArtifactSource is set from artifactSource(c.FromCache) (libs/localenv/pipeline.go:191), whose entire range is those two values (pipeline.go:27-30), and it's assigned in exactly one place.
The reasoning in the comment is sound (avoid flaking on a warm shared cache) — the problem is the resulting assertion reads like coverage but checks nothing.
Either isolate the cache so "network" is deterministic, or drop the line. Note there's currently no cache-dir override knob — cmd/environments/sync.go:136 uses os.UserCacheDir() unconditionally — so isolating means adding one or leaning on HOME/XDG_CACHE_HOME, neither portable. Deleting is probably the honest option.
There was a problem hiding this comment.
Done in 9d9c0ac — deleted. artifactSource() only ever returns artifactCache or artifactNetwork (pipeline.go:535-540) from a single assignment site, so the membership assertion could never fail. Agreed that deleting beats adding a cache-dir override just to make it meaningful.
| assert.Equal(t, localenv.PhaseFetch, res.Error.FailurePhase) | ||
|
|
||
| // Even a failed fetch must not have provisioned anything on a dry run. | ||
| assert.NoFileExists(t, filepath.Join(dir, ".venv", "bin", "python")) |
There was a problem hiding this comment.
This hardcodes the Unix layout, defeating the venvPython helper defined 5 lines below (and used in test 1). On Windows this checks a path that could never exist, so the assertion is vacuous there.
Since this is a --dry-run — preflight's ensureWritable is skipped and cache writes are suppressed (pipeline.go:141-147) — the directory should be completely empty. The stronger and simpler assertion is the same os.ReadDir + assert.Empty that TestSetupLocalDryRunWritesNothing already uses.
There was a problem hiding this comment.
Done in 9d9c0ac — replaced with the os.ReadDir + assert.Empty check, matching TestSetupLocalDryRunWritesNothing. It was bypassing venvPython and vacuous on Windows; since --dry-run skips ensureWritable and suppresses cache writes, asserting the dir is empty is strictly stronger.
| @@ -0,0 +1,141 @@ | |||
| package environments_test | |||
There was a problem hiding this comment.
Not this file, but related: OWNERS routes this file to team:platform rather than team:ide.
/integration/ → team:platform (.github/OWNERS:65) and findOwners is last-match-wins (.github/scripts/owners.js:97-106), so this file — squarely localenv territory — won't route to the team that owns /libs/localenv/, /cmd/environments/, and /acceptance/localenv/ (added in #6187).
Worth adding, and it must go below line 65 to win:
/integration/cmd/environments/ team:ide
There was a problem hiding this comment.
Done in 9d9c0ac — added /integration/cmd/environments/ team:ide below the /integration/ rule so it wins under last-match-wins. Verified with findOwners: this file now resolves to rugpanov rclarey anton-107 misha-db, while integration/cmd/jobs/ still resolves to team:platform.
…venv path Per review on #6155: - Skip when uv is absent instead of setting EnvAutoInstallUv. The opt-in made the pipeline run the remote installer and mutate ~/.local/bin; CI installs uv, so the only case it covered was one where that side effect is unwanted. The old comment also claimed the developer had opted in, when the test set the flag. - Drop the artifactSource assertion. artifactSource() only ever returns "cache" or "network", so asserting membership in that set could never fail. - Assert the dry-run project dir is empty rather than checking a hardcoded .venv/bin/python. That path bypassed the venvPython helper and was vacuous on Windows; --dry-run skips ensureWritable and suppresses cache writes, so the directory must be empty outright. - Route /integration/cmd/environments/ to team:ide. It sits below /integration/ because findOwners is last-match-wins.
|
@anton-107 all four points addressed in 9d9c0ac — thanks for the review, especially the uv one.
Verified on real runners rather than just locally — all three tests pass in all six integration environments (aws/azure/gcp × linux/windows), with One note on the OWNERS line: it won't affect this PR's own approval routing, since The red checks are unrelated — every failing job is |
4 participants
Changes
Adds
integration/cmd/environments/setup_local_test.go— end-to-end integration tests fordatabricks environments setup-localthat run in the cli-isolated e2e workflow (isolated AWS/Azure/GCP workspaces). ThreeCLOUD_ENV-gated tests, all against the real publicdatabricks/environmentsrepo:--serverless-version 5 --output json: resolve → fetch →uv sync→ validate, asserting a real.venv/uv.lockand the JSON contract (ok,compute.source/envKey,resolved.pythonVersion/dbconnectVersion/artifactSource, all phasesok).--dry-runwrites nothing — asserts the temp project dir stays empty.--serverless-version 9999→E_ENV_UNSUPPORTEDat thefetchphase with a non-zero exit.Why
The pipeline was covered hermetically (unit + acceptance via
--dry-run/stubs) and by one env-gated real-provision test inlibs/localenv, but nothing drove the command end to end through the CLI entrypoint in the integration suite. Serverless needs no running compute (the version is used verbatim), so these give real e2e coverage without a cluster dependency. They skip cleanly whenCLOUD_ENVis unset, so unit-test CI is unaffected.Tests
devworkspace + the public repo (full provision ~6.6s; dry-run and error-path fast).go vet+golangci-lintclean; skip verified withCLOUD_ENVunset.This PR was written by Claude Code.