test(packaging): unit-test bundled-CLI version logic, isolate the real-binary smoke test - #2134
Conversation
…l-binary smoke test *Why*: * The `bundled CLI version` tests spawned the real ~50MB databricks CLI under mocha's 2s default timeout, so on the slow Windows unit runner the first cold spawn intermittently exceeded it — an intermittent flake (alternated pass/fail across commits with the file untouched), unrelated to any change. Spawning a real binary is also not a unit test. *What:* * Extracted pure `parseCliVersion` and `isBundledCliVersionMismatch` and unit-tested them without spawning; `getBundledCliVersion` / `checkBundledCliVersion` now delegate to them (behavior unchanged). * Moved the real-binary assertions into a clearly-labeled smoke describe with a 30s timeout (they validate the `package:cli:fetch` step, not unit logic), and kept the non-spawning gate paths as fast unit tests. *Verification:* * yarn test:unit — 866 passing, 0 failing; eslint + prettier clean. Co-authored-by: Isaac
GitHub Actions
Error
|
🤖 Integration tests triggered for |
*Why*: * Review of the split found small regressions/nits: parseCliVersion swallowed malformed JSON so getBundledCliVersion no longer logged a parse failure (Codex); the getBundledCliVersion JSDoc was orphaned above parseCliVersion (Claude); and a test used a hard-coded POSIX path (CODE_CONVENTIONS §9). *What:* * getBundledCliVersion logs a debug line when the CLI output is unparseable (restores the observability the pre-split catch gave); moved its JSDoc back above it. * Test: hard-coded "/nonexistent/databricks" -> path.join(__dirname, ...); added the both-unknown case to the isBundledCliVersionMismatch truth table; reworded the gating-describe comment to be accurate about the fail-fast missing-binary case. *Verification:* * yarn test:unit — 867 passing, 0 failing; eslint + prettier clean. Co-authored-by: Isaac
GitHub Actions
Error
|
🤖 Integration tests ✅ all 41 test jobs passed for |
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
Approving — correct diagnosis, behavior-preserving, and CI is green on both unit-test jobs. Comments are quality/convention only; none block.
Things I checked rather than took on faith:
- Behavior really is unchanged.
!isBundledCliVersionMismatch(actual, expected)expands toactual === undefined || expected === undefined || actual === expected; the extra disjunct is already short-circuited by the earliermetaData.cliVersion === undefinedgate, so it reduces to the old condition. - The flake diagnosis holds.
src/test/suite.tsconstructs Mocha with notimeout, so the 2s default applies. Glob order putspackageJsonUtils.test.tsat index 8 andCliWrapper.test.tsat index 42 — so this file absorbed the cold-spawn cost (AV scan + cold FS cache) while the other was both warm and already carryingthis.timeout("10s"). That explains exactly why this file flaked and that one didn't. parseCliVersionhas no crash path. Exercisednull,5,[1,2],{"Version": null},"", and non-JSON — all returnundefinedvia thecatchor thetypeofguard, none throw.- No coverage dropped — all six original assertions survive, renamed and redistributed.
One structural note beyond the inline comments: the framing in the description ("isn't a unit test to begin with — it was mislabeled .test.ts") doesn't match what the change does — the tests stay in .test.ts with a raised timeout. That's the right call and it matches precedent (CliWrapper.test.ts:63 spawns the real CLI from a unit test, and CODE_CONVENTIONS.md §8's suffix table has no "smoke" row), but the stated rationale and the change disagree. Describing it as a timeout fix plus an extraction would be more accurate.
Nits, non-blocking: assert.equal is loose, so assert.equal(x, undefined) also passes for null — strictEqual is tighter, though loose equal is the established style in this file. The new "unparseable stdout" branch in getBundledCliVersion is itself uncovered — the extraction tests the parser, not the wiring to it. And 30s is generous next to CliWrapper's 10s for the same binary.
| // True only when both versions are known and differ — the case worth warning | ||
| // about. An unknown actual (CLI unreadable) or unknown expected (unpinned) is | ||
| // deliberately not a mismatch. Pure — unit-tested without spawning the CLI. | ||
| export function isBundledCliVersionMismatch( |
There was a problem hiding this comment.
This one feels over-extracted. The body is a !== undefined && b !== undefined && a !== b — a total boolean expression with no product logic in it — but it gets a name longer than its implementation, a 3-line comment, and five unit tests that largely assert the semantics of &&.
It also makes the call site harder to read rather than easier: at line 200 the reader now has to recall that expected can't be undefined there for the code to mean what it used to. Needing a parenthetical in the PR description to prove the equivalence is the tell.
parseCliVersion is a genuinely valuable extraction — real parsing, real edge cases, real tests. For this one I'd inline the compare back at the call site and keep the truth table in the head, or at least drop it to one or two tests.
| // Extracts the "Version" string from `databricks version --output json` stdout. | ||
| // Returns undefined on malformed JSON or a missing/non-string field, so callers | ||
| // treat an unreadable version the same as an absent one. Pure — unit-tested | ||
| // without spawning the CLI (getBundledCliVersion is the thin process wrapper). |
There was a problem hiding this comment.
Comment volume runs against CODE_CONVENTIONS.md §4b ("shorter than the code it guards", "explain why, never what"). Across the two new helpers it's ~16 lines of comment for ~35 lines of code, and several lines restate the signature: "Returns undefined on malformed JSON or a missing/non-string field" narrates typeof version === "string" ? version : undefined, which the return type already says.
The Pure — unit-tested without spawning the CLI notes (here and on isBundledCliVersionMismatch) describe the test file, not the function contract — precisely the kind that rots silently when tests move. Same for Both failure modes log at debug. added to the JSDoc below.
§4b is explicit that this matters more when working AI-assisted, and the description says this was written by Isaac. Suggest trimming each to the load-bearing sentence — for this one, roughly "Returns undefined so callers treat an unreadable version the same as an absent one."
| if (version === undefined) { | ||
| logging.NamedLogger.getOrCreate(Loggers.Extension).debug( | ||
| "Bundled Databricks CLI version output was unparseable", | ||
| {stdout} |
There was a problem hiding this comment.
Minor: {stdout} logs whatever the binary printed, unbounded. Harmless for databricks version output (nothing sensitive), but if cliPath ever resolves to the wrong executable this dumps arbitrary process output into the extension log. stdout.slice(0, 200) costs nothing and caps the blast radius.
| // ~50MB binary on the Windows runner exceeds the 2s mocha default, so give | ||
| // the suite a generous timeout — the default made this flake intermittently. | ||
| describe("bundled CLI (smoke — spawns the real fetched binary)", function () { | ||
| this.timeout(30_000); |
There was a problem hiding this comment.
This fixes the instance but the class of flake survives, and it's order-dependent. The cold-spawn penalty attaches to whichever spawn test glob happens to run first, and glob order isn't a stable contract — add or rename a file under src/ and the penalty migrates to CliWrapper.test.ts, whose 10s may then become the thing that flakes.
A default timeout on the Mocha instance in src/test/suite.ts (which currently passes only ui and color) would retire this failure mode repo-wide in one line, instead of per-file as each one flakes in turn. Fine as a follow-up if you'd rather keep this PR tight.
| if (originalDevFlag === undefined) { | ||
| delete process.env[EXTENSION_DEVELOPMENT]; | ||
| } else { | ||
| process.env[EXTENSION_DEVELOPMENT] = originalDevFlag; |
There was a problem hiding this comment.
The 8-line env save/restore block is now duplicated verbatim between this describe and checkBundledCliVersion gating above. Worth hoisting to a small helper in the file (or src/test/utils.ts) so the two can't drift.
*Why* Review on #2134 flagged that `isBundledCliVersionMismatch` was over-extracted — a total boolean with no domain logic, given a name longer than its body and five tests that mostly assert the semantics of `&&`, while making the call site read worse (the reader had to recall `expected` can't be undefined there). The new helper comments also ran against CODE_CONVENTIONS §4b by restating the signature and describing the test file rather than the contract, and the unparseable-output log dumped unbounded CLI stdout. *What* - Inline the version compare back at the call site (`actual === undefined || actual === metaData.cliVersion`) and drop `isBundledCliVersionMismatch` plus its five unit tests. `parseCliVersion` — the extraction with real parsing logic and edge cases — stays. - Trim the `parseCliVersion` comment and `getBundledCliVersion` JSDoc to the load-bearing "why", dropping lines that narrated the signature or the tests. - Cap the unparseable-stdout debug log at 200 chars. - Hoist the duplicated EXTENSION_DEVELOPMENT save/restore into a `restoreDevFlagAfterEach()` helper shared by both describes. *Verification* - `yarn test:unit` — 862 passing, 0 failing (parseCliVersion units + the smoke describe with the fetched binary). - `yarn fix` clean. Co-authored-by: Isaac
GitHub Actions
Queued
|
🤖 Integration tests triggered for |
*Why* Removing `isBundledCliVersionMismatch` and its tests dropped the only coverage of `checkBundledCliVersion`'s `actual === undefined` short-circuit — the "dev checkout, version pinned, but the bundled CLI is unreadable, so don't warn" case. Review of the follow-up commit flagged the gap. *What* Add a fast (no-spawn) gating test: dev flag on, a pinned `cliVersion`, and a nonexistent binary path so the version reads back unknown — asserting `checkBundledCliVersion` returns true (no warning, no throw). *Verification* - `yarn test:unit` — 863 passing, 0 failing. - `yarn fix` clean. Co-authored-by: Isaac
GitHub Actions
Queued
|
🤖 Integration tests triggered for |
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
GitHub Actions
Queued
## Why The unit suite builds Mocha with only `ui`/`color`, so mocha's **2 s default** applies. On cold CI runners (AV scan + cold FS cache) a test's first real I/O can exceed 2 s, and the penalty attaches to **whichever spawn test the glob happens to run first** — an unstable contract, since adding or renaming a file under `src/` migrates the flake to the next spawn test. Raising the timeout per-file only chases the class one file at a time (this was the follow-up flagged in review of #2134). ## What Set `timeout: 5000` on the shared Mocha instance in `src/test/suite.ts`. 5 s clears the cold-runner I/O spike while staying tight enough to fail a genuine hang fast. Tests that spawn the real CLI keep their own higher overrides (`CliWrapper.test.ts` 10 s, the packaging smoke describe 30 s), which still win per-suite. ## Verification - `yarn test:unit` — 877 passing, 0 failing. - `eslint` + `prettier` clean. This pull request and its description were written by Isaac.
2 participants
Why
The
bundled CLI versiontests inpackageJsonUtils.test.tsspawned the real ~50 MBdatabricksCLI under mocha's 2 s default timeout. On the slow Windows unit runner, the first cold spawn (AV scan + cold FS cache) intermittently exceeded 2 s →Timeout of 2000ms exceeded. It's a genuine flake — the same untouched file alternated pass/fail across recent commits (Linux always passes; it spawns faster).Spawning the real CLI from a unit test is itself established here (
CliWrapper.test.ts:63does the same, and it already carries a 10 s timeout), so the fix is a timeout bump plus an extraction that gives the version parse real coverage without spawning — not a relabel.What
package:cli:fetchstep (the "re-fetch the CLI" pitfall), not unit logic. Non-spawning gate paths (dev-checkout gate, unpinned version, missing binary) stay as fast unit tests.parseCliVersion(stdout)— thedatabricks version --output json→Versionparse (undefined on malformed/missing/non-string) — and unit-tested it without spawning.getBundledCliVersiondelegates to it and logs a (capped) debug line when the output is unparseable.checkBundledCliVersionkeeps its behavior: an unknown version (CLI unreadable) is treated as "not stale", and the dev-checkout / unpinned gate short-circuits before any spawn.Net: the version parse gets real, fast, deterministic unit coverage; the one honest real-binary check is isolated and no longer flakes on the 2 s default.
Verification
yarn test:unit— 862 passing, 0 failing (the newparseCliVersionunits + the smoke describe with the fetched binary).eslint+prettierclean.This pull request and its description were written by Isaac.