localenv: populate setup-local durationMs - #6190
Open
rugpanov wants to merge 2 commits into
Open
Conversation
durationMs was declared in the --output json contract but always emitted as 0, so the extension had no CLI-side latency figure to report (it backs the ERD's ~3-min setup claim). Stamp it from a defer in Pipeline.Run, which covers every exit path: success, a phase failure, and the E_CANCELED reclassification. The acceptance JSON goldens would otherwise carry a real, changing value, so a [DURATION_MS] repl normalizes it. It lives in acceptance/localenv/test.toml rather than the root: localenv is the only command emitting the key, and a global repl would silently normalize it for any future command without anyone opting in. Order = 8 beats the root's Order = 10 numeric repls, which would otherwise rewrite the digits first. Splitting this out from the warnings half of the same contract, which needs further work — they are independent fields and the durationMs change stands on its own. DECO-27875 Co-authored-by: Isaac
This was referenced Aug 6, 2026
Collaborator
Integration test reportCommit: 5860bfb
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 3 slowest tests (at least 2 minutes):
|
TestPipelineReportsDurationOnFailure only asserted DurationMs <= elapsed. An unset field is 0, which satisfies that bound trivially, so the test passed with the deferred stamp in Run deleted — it protected nothing on exactly the path it was named for. Fail after the delayed constraint fetch instead of at preflight. A preflight usage error returns near-instantly, so its duration truncates to 0 and no non-trivial lower bound exists; a 500 with an empty cache fails at the fetch phase with E_FETCH, after the run has done measurable work. Both duration tests now fail when the defer is removed. The 25ms delay and its server move to a shared const and helper, since both tests now need them. Co-authored-by: Isaac
anton-107
approved these changes
Aug 7, 2026
rugpanov
added a commit
that referenced
this pull request
Aug 7, 2026
Address review on #6191. The override warning was deciding for itself which databricks-connect pin the merge rewrites, using rules that did not match MergeManaged's. Where they disagreed the warning claimed a replacement that never happened — the same false claim the W_DBCONNECT_PIN_DUPLICATED split was added to remove, reached through a different door. MergeManaged now reports the requirement it rewrote in place, and the detector consumes that instead of re-deriving it. Anything the merge does not report as replaced is, by definition, still in the file beside the managed pin, so the retained set falls out by subtraction. A single-quoted element, a top-level dotted `dependency-groups.dev` key and a `Dev` group are all left in place by the merge and now report DUPLICATED rather than OVERRIDDEN. Deciding this from the returned `regions` instead does not work: regionDatabricksConnect is present for a pure insert into an empty dev group, where nothing was overridden, and absent when the first of two pins already matches, where a stale pin survives. It answers whether the region changed, not whether a pin was rewritten. W_DBCONNECT_PIN_DUPLICATED is now gated on the two pins being provably disjoint. Two pins are only a problem when nothing satisfies both; ">=16" beside "~=17.2.0" resolves at 17.2.x, so reporting it sent the user after a conflict that does not exist. Also from the same review: - Conflicts are scanned across every requirement uv locks — [project].dependencies, the optional-dependency extras, and all dependency groups. uv applies constraint-dependencies to the whole resolution, so a pin in a non-dev group fails `uv sync` identically; scanning only dev stayed silent on it. - The user's file decodes through loosely-typed containers. BurntSushi reports a type mismatch on one key as a whole-document error, so a group sub-table ([dependency-groups.docs], the PDM style) dropped every warning, including those that never read it. - Group iteration is sorted, so colliding keys cannot let map order pick which warning is reported. - One conflict per requirement, so a pin declared in several places does not inflate the code histogram consumers build from warnings[]. - The pin the merge replaces is out of scope for the conflict scan; retained pins stay in. - Messages are present tense, since the same detection runs under --dry-run. Corrects the claim that the published artifacts use bound shapes: serverless-v4 and -v5 are entirely "~=" (204/204 and 240/240), so the acceptance fixture was the only source of "pyarrow<19". The interval model still handles both; the justification was wrong, not the design. Adds a "~=" conflict case alongside the bound one. Restores the merge-warnings acceptance scenarios in both --json and text form. They are the only end-to-end coverage of the warnings[] contract, and the generated golden carries "durationMs": 0 — it needs #6190's repl to land first. Co-authored-by: Isaac
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
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.
Changes
Populates
durationMsin theenvironments setup-local --output jsoncontract. It was declared but never set (always0), so the extension had no CLI-side latency figure to report.deferinPipeline.Run, so every exit path is covered: success, a phase failure, and theE_CANCELEDreclassification added in localenv: reap uv subprocesses on SIGINT/SIGTERM instead of orphaning them #6107.[DURATION_MS]repl normalizes it. It lives inacceptance/localenv/test.tomlrather than the root, since localenv is the only command emitting the key — a global repl would silently normalize it for any future command without anyone opting in.Order = 8beats the root'sOrder = 10numeric repls, which would otherwise rewrite the digits first.Why
Backs the ERD's "setup result" metric group (the ~3-min duration claim). DECO-27875.
Split from #6176
#6176 populated both dead fields of this contract (
durationMsandwarnings). Review found thewarningshalf needs substantive work — the conflict detector can't decide the bound-style constraints our artifacts actually publish, and one warning made a factually wrong claim. Those are independent contract fields, so per the one-change-per-PR rule this half lands on its own and the warnings work gets its own PR.Tests
TestPipelineReportsDurationdelays the constraint fetch by 25ms and asserts the duration exceeds it and stays within the run's observed wall time. A>= 0assertion would have passed against the old hardcoded0, so it would prove nothing.TestPipelineReportsDurationOnFailurecovers the preflight-error path, whereRunreturns before any phase executes.Note: the injectable
Pipeline.Nowclock from #6176 is dropped. It was exported API set by exactly one test, and the repl — not the clock — is what makes the goldens deterministic.This pull request was written by Isaac.