fix(testing): gate MSSQL container readiness on the sa login

[ovr]

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

The tests (mssql, *) drivers-tests jobs fail intermittently with ConnectionError: Login failed for user 'sa', because both MSSQL container helpers waited on a startup log line (SQL Server is now ready for client connections / Service Broker manager has started) that SQL Server prints before the sa login from MSSQL_SA_PASSWORD is usable — reproduced 3/3 on mcr.microsoft.com/mssql/server:2022-latest, where connecting the instant that line lands fails and the same login succeeds a second later. Both cubejs-testing-drivers (runEnvironment) and cubejs-testing-shared (MssqlDbRunner) now wait on Wait.forSuccessfulCommand running sqlcmd -Q "SELECT 1" as sa, i.e. on the login itself. Verified against 2019-latest and 2022-latest (both ship /opt/mssql-tools18, and the older v17 mssql-tools path is gone) and through a real DockerComposeEnvironment using the mssql fixture's data service: ready in ~7-8.5s with an immediate sa connection returning a row, well inside the existing 30s/60s compose startup timeout. The full mssql-full suite was not run locally since it needs the cubejs/cube:testing-drivers image, so please let CI exercise it.

🤖 Generated with Claude Code

[claude]

Claude finished @ovr's task in 2m 42s —— View job

---

Approach is right; 2 medium findings on the testing-shared runner

Review details

• Gather context (diff, changed files, mssql fixture, existing mssql runners)
• Review changed files in full
• Check runtime behaviour assumptions (wait-strategy exec path, container env, tools path)
• Handle prior review threads
• Post findings

Verdict

Gating on the sa login instead of the startup log line is the correct fix for Login failed for user 'sa' — the log line genuinely precedes login availability. Wait.forSuccessfulCommand runs the string through /bin/sh -c inside the container, so the $MSSQL_SA_PASSWORD form in runEnvironment.ts:159 resolves against the data service env set in fixtures/mssql.json:25, and the fixture pins 2022-latest, so that hunk is sound as written.

Both findings are on packages/cubejs-testing-shared/src/db-container-runners/mssql.ts, where the image tag — unlike the fixture — is caller/env-configurable:

#	Severity	Site	Finding
1	Medium	mssql.ts:17-20	Hardcoded /opt/mssql-tools18/bin/ vs. TEST_MSSQL_VERSION-configurable tag: 2017-latest and 2019 tags ≤ CU27 ship the tools at /opt/mssql-tools/bin, so the probe exits 127 forever and the start fails after 120s. That path split is already handled at packages/cubejs-schema-compiler/test/integration/mssql/MSSqlDbRunner.js:101-110.
2	Medium	mssql.ts:18	Password JS-interpolated into a /bin/sh -c string: $, ", a backtick or a backslash in TEST_DB_PASSWORD silently corrupts the credential → login fails → opaque 120s timeout. The companion hunk's container-side -P "$MSSQL_SA_PASSWORD" avoids it.

Nits not posted inline: raising the startup timeout 30s → 120s quadruples time-to-fail for a container that is genuinely broken, and -o /dev/null discards the sqlcmd error that would explain such a failure — reasonable trade-offs for a readiness probe, but worth knowing when a timeout does show up. No test coverage is expected for wait-strategy plumbing, and neither hunk is the kind of change that needs a comment.

Not verified locally: yarn lint / yarn tsc were not run (dependencies are not installed in this checkout), and the mssql-full suite needs the cubejs/cube:testing-drivers image — CI is the check for both.

Review threads

The unresolved-thread listing returned 0 threads of mine (no prior threads); nothing resolved, nothing re-affirmed as a duplicate.

· branch [`fix-mssql-driver-tests`](https://github.com/cube-js/cube/tree/fix-mssql-driver-tests)

[claude]

Claude finished @ovr's task in 3m 56s —— View job

---

Approach is right — 2 low-severity findings (0 high, 0 medium, 2 low), both posted inline

Full review

• Read the diff and both changed files in context
• Check the mssql fixture env/service names against the probe command
• Cross-check the third mssql runner (cubejs-schema-compiler) for consistency
• Resolve stale review threads / de-duplicate findings
• Post inline findings and final summary

What holds up

Gating on the sa login instead of SQL Server is now ready for client connections is the correct fix for the reported Login failed for user 'sa' flake — the log line is emitted before the SA login from MSSQL_SA_PASSWORD is usable. packages/cubejs-schema-compiler/test/integration/mssql/MSSqlDbRunner.js:122-132 already gates the same image on a sqlcmd -Q "SELECT 1" healthcheck, so this brings the other two helpers in line with a pattern that is already proven in this repo.

Details that check out:

• The compose service name data and the env var MSSQL_SA_PASSWORD match packages/cubejs-testing-drivers/fixtures/mssql.json:22-27, so $MSSQL_SA_PASSWORD resolves container-side.
• The password is read from the container's environment rather than interpolated into the command string, so it never reaches a testcontainers exec log. Expansion results are not re-scanned by sh, so a password containing $ or a backtick is still passed intact.
• PATH=/opt/mssql-tools18/bin:/opt/mssql-tools/bin:$PATH covers both the v18 path on 2019-latest/2022-latest and the older v17 path — consistent with the version split documented at MSSqlDbRunner.js:101-110, so TEST_MSSQL_VERSION=2017-latest still resolves a sqlcmd.
• -C (trust server certificate) is required with mssql-tools18, which mandates encryption; without it the probe would fail against the self-signed cert on every poll.

Findings

#	Severity	Location	Issue
1	Low	packages/cubejs-testing-drivers/src/helpers/runEnvironment.ts:158	The compose-side probe keeps the global (isCI() ? 60 : 30) * 1000 budget while the shared runner was bumped to 120s for the identical readiness signal. Slow services in the same block (oracle, pinot, crate) already set a per-strategy .withStartupTimeout(); the asymmetry looks unintentional either way.
2	Low	packages/cubejs-testing-shared/src/db-container-runners/mssql.ts:17	The probe command string is duplicated verbatim in runEnvironment.ts:159. cubejs-testing-drivers already depends on @cubejs-backend/testing-shared, so this can be one exported constant; a future divergence between the two copies fails silently as a startup timeout.

Considered and not posted: -o /dev/null discards sqlcmd's output, so a readiness timeout surfaces without the underlying login error. testcontainers does not include exec output in the timeout error either way, so this costs little in practice — but dropping it would make a genuine failure (wrong password, missing tools dir) diagnosable from the debug log rather than only from a bare timeout.

Not verified here: the mssql-full suite needs the cubejs/cube:testing-drivers image and this environment has no Docker or network access, so the readiness timings in the PR description were taken as reported rather than reproduced. The one claim worth CI's attention is cold-start time on a loaded runner, which is exactly what finding 1 is about.

Review threads

The listing returned 2 of my own threads from the previous round; both are resolved, neither re-affirmed:

• Hardcoded /opt/mssql-tools18/bin/ path — addressed, line 17 now prepends both candidate dirs to PATH.
• Password spliced into the shell command — addressed, the command reads $MSSQL_SA_PASSWORD container-side.

Neither was cited as a duplicate of this round's findings.

· branch [`fix-mssql-driver-tests`](https://github.com/cube-js/cube/tree/fix-mssql-driver-tests)

[claude]

This mssql wait strategy keeps the global compose budget set at line 118 ((isCI() ? 60 : 30) * 1000), while the companion hunk in packages/cubejs-testing-shared/src/db-container-runners/mssql.ts:19 bumped the same probe to withStartupTimeout(120 * 1000). So the two helpers now wait for the identical readiness signal with very different budgets — 30s locally / 60s in CI here vs. 120s there.

That matters because the reported ready times in this PR (9.4s for 2017-latest, 9.0s for 2022-latest on a warm image) are measured after the pull; on a cold CI runner, SQL Server routinely needs well past 60s before the sa login is accepted, and the failure mode is a compose-wide startup timeout rather than the old log-line wait, which shared the same budget but tripped earlier on a weaker signal.

Other slow services in this same block already opt out of the global budget per strategy — oracle (line 171, 240s), pinot (line 176, 180s), crate (line 195, 120s) — so the mechanism is right here:

Suggested change

	compose.withWaitStrategy('data', Wait.forSuccessfulCommand(
	compose.withWaitStrategy('data', Wait.forSuccessfulCommand(

(and append .withStartupTimeout(120 * 1000) to the strategy on the closing line, matching the shared runner)

If the intent was instead that mssql is fast enough for the global budget, the 120s in the shared runner is the one that wants explaining — either way the asymmetry looks unintentional.

[claude]

This probe string is now duplicated verbatim in packages/cubejs-testing-drivers/src/helpers/runEnvironment.ts:159 — same PATH prefix, same flags, same $MSSQL_SA_PASSWORD indirection. Both copies encode three separate pieces of knowledge that were fixed together in 2dcc6bc (the two candidate tools dirs, -C for the self-signed cert, and reading the password container-side), so any future change — a fourth tools path for a new image family, dropping -o /dev/null, switching to sqlcmd -No — has to be found and applied twice, and a miss is silent: the divergent copy just times out.

cubejs-testing-drivers already depends on @cubejs-backend/testing-shared, and runEnvironment.ts imports from it, so exporting the string from here and importing it there costs nothing:

Suggested change

	'PATH=/opt/mssql-tools18/bin:/opt/mssql-tools/bin:$PATH sqlcmd -C -S localhost -U sa -P "$MSSQL_SA_PASSWORD" -Q "SELECT 1" -b -o /dev/null'
	.withWaitStrategy(Wait.forSuccessfulCommand(MSSQL_READY_COMMAND))

with export const MSSQL_READY_COMMAND = 'PATH=/opt/mssql-tools18/bin:/opt/mssql-tools/bin:$PATH sqlcmd -C -S localhost -U sa -P "$MSSQL_SA_PASSWORD" -Q "SELECT 1" -b -o /dev/null'; above the class (and re-exported from the package index), then using it at the runEnvironment.ts call site.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.69%. Comparing base (c4f6627) to head (4fe7d96).
⚠️ Report is 6 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (c4f6627) and HEAD (4fe7d96). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (c4f6627)	HEAD (4fe7d96)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11917       +/-   ##
===========================================
- Coverage   84.88%   60.69%   -24.20%     
===========================================
  Files         261      247       -14     
  Lines       87535    19763    -67772     
  Branches        0     4027     +4027     
===========================================
- Hits        74306    11995    -62311     
+ Misses      13229     7203     -6026     
- Partials        0      565      +565     

Flag	Coverage Δ
cube-backend	60.69% <ø> (?)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.