Add version-agnostic upgrade/persistence compatibility test suite - #2715
Merged
Conversation
- npm/VSIX/Docker upgrade scenarios for blob, queue, and table data - shared UpgradeTarget abstraction (NpmProcessTarget/DockerContainerTarget) - shared blobUploader/tableValueCodec fixture handling across npm and Docker - dedicated CI workflow running on merge to main and on demand
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 11:40
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new tests/upgrade/ regression suite and CI workflow to continuously validate Azurite’s persistence/upgrade compatibility across releases by seeding data with the latest published artifacts (npm, Marketplace VSIX, MCR Docker image) and verifying it remains readable and byte/value-identical after upgrading to the local build.
Changes:
- Added version-agnostic upgrade tests for blob/queue/table (npm) and Docker image upgrade (MCR → local image with shared volume).
- Added a VSIX lifecycle test harness using
@vscode/test-electron(install VSIX → activate → start/stop services). - Added a dedicated GitHub Actions workflow plus npm scripts to run the new upgrade suites.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/upgrade/vsixLifecycle/suite/vsixLifecycle.test.js | VSIX lifecycle assertions (activate/start/stop) driven via VS Code Extension Host |
| tests/upgrade/vsixLifecycle/suite/index.js | Mocha test loader for the VSIX lifecycle suite |
| tests/upgrade/vsixLifecycle/runVsixTests.ts | Downloads VS Code test instance, installs VSIX into isolated profile, runs lifecycle suite |
| tests/upgrade/vsixLifecycle/resolveVsixToTest.ts | Resolves/creates the VSIX under test (local package vs Marketplace download) |
| tests/upgrade/vsixLifecycle/driverExtension/package.json | Minimal driver extension manifest used only to host tests |
| tests/upgrade/vsixLifecycle/driverExtension/extension.js | No-op activation entrypoint for the driver extension |
| tests/upgrade/utils/versionResolver.ts | Dynamic resolution of latest published npm/Marketplace/MCR versions |
| tests/upgrade/utils/upgradeTarget.ts | Common start/stop abstraction for npm process vs Docker container targets |
| tests/upgrade/utils/tableValueCodec.ts | Shared typed table entity payload building + verification helpers |
| tests/upgrade/utils/processHarness.ts | Generic Azurite CLI process spawn/readiness/stop harness |
| tests/upgrade/utils/npmVersionInstaller.ts | Installs a specific azurite npm version into an isolated temp directory |
| tests/upgrade/utils/integrity.ts | Byte/hash comparison and property map comparison helpers |
| tests/upgrade/utils/httpProbe.ts | HTTP readiness probing shared by harnesses |
| tests/upgrade/utils/dockerHarness.ts | Thin docker CLI wrapper for pull/build/run/stop/rm |
| tests/upgrade/utils/dataFixtures.ts | Deterministic blob/queue/table fixtures for cross-version verification |
| tests/upgrade/utils/blobUploader.ts | Shared blob seeding + byte-for-byte verification across blob types |
| tests/upgrade/tableUpgrade.test.ts | Npm-based table upgrade compatibility test (old published → local build) |
| tests/upgrade/queueUpgrade.test.ts | Npm-based queue upgrade compatibility test (old published → local build) |
| tests/upgrade/dockerUpgrade.test.ts | Docker/MCR upgrade test (old MCR tag → locally built image on same volume) |
| tests/upgrade/blobUpgrade.test.ts | Npm-based blob upgrade compatibility test (old published → local build) |
| tests/blob/upgradeRegression.test.ts | Updates existing upgrade regression test string to be version-agnostic |
| package.json | Adds @vscode/test-electron dev dependency and upgrade test scripts |
| package-lock.json | Locks new @vscode/test-electron transitive dependencies |
| docs/designs/2026-08-upgrade-compatibility-testing.md | Design doc describing architecture and rationale for the upgrade suite |
| ChangeLog.md | Notes the addition of the new upgrade compatibility regression suite |
| .github/workflows/UpgradeCompatibility.yml | New CI workflow to run upgrade compatibility jobs post-merge and on demand |
Suppressed comments (2)
tests/upgrade/vsixLifecycle/suite/vsixLifecycle.test.js:68
- This test claims it "stops all services" but it only verifies the blob port stops responding. Queue/table could remain running and the test would still pass.
const isDown = await waitUntil(
async () => !(await probeHttp(BLOB_DEFAULT_PORT)),
30000,
1000
);
tests/upgrade/utils/processHarness.ts:91
- AzuriteProcessHandle.stop() awaits the child process 'exit' event with no upper bound. If the process fails to terminate (including after the SIGKILL attempt), the promise never resolves and the whole test run can hang indefinitely.
await new Promise<void>((resolve) => {
child.once("exit", () => resolve());
child.kill();
setTimeout(() => {
if (child.exitCode === null) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- DockerContainerTarget.start() now waits for blob, queue, and table ports instead of only blob, matching the UpgradeTarget contract - AzuriteProcessHandle.start() now rejects on any early exit (including code 0) instead of only non-zero exit codes - VSIX lifecycle test now probes all three default ports on start/stop instead of only the blob port - resolveVsixToTest.ts uses npx.cmd on Windows instead of hardcoding npx
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 12:11
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/upgrade/utils/tableValueCodec.ts:67
assertEntityMatchesFixture()assumes the SDK returnsbinaryPropas aUint8Array. In practice@azure/data-tablesmay return either raw bytes (Buffer/Uint8Array) or a base64 string depending on serialization shape; if it returns a string,Buffer.from(string)will interpret it as UTF-8 and the comparison will be incorrect.
const fetchedBinary = unwrapTypedValue(fetched.binaryProp);
assert.deepStrictEqual(
Buffer.from(fetchedBinary as Uint8Array),
Buffer.from(entity.binaryProp)
);
tests/upgrade/utils/tableValueCodec.ts:24
toCreateEntityPayload()base64-encodesbinaryPropbefore passing it as an ODataBinarytyped value.@azure/data-tablesalready handlesBinaryencoding when the value is bytes (Buffer/Uint8Array); pre-encoding here risks double-encoding and storing different bytes than the fixture intended.
This issue also appears on line 63 of the same file.
binaryProp: {
value: Buffer.from(entity.binaryProp).toString("base64"),
type: "Binary" as const
}
tests/upgrade/utils/versionResolver.ts:150
- MCR/Docker Registry pagination uses a relative URL in the
Link: <...>; rel="next"header (e.g./v2/<repo>/tags/list?...). Assigning that directly tourlwill make the nextfetch()fail because it isn't an absolute URL.
const link = res.headers.get("link");
const nextMatch = link?.match(/<([^>]+)>;\s*rel="next"/);
url = nextMatch ? nextMatch[1] : undefined;
Will be reverted before merge - only needed to prove the workflow YAML itself runs correctly since workflow_dispatch can't register until this file exists on main.
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 12:17
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/upgrade/utils/versionResolver.ts:90
getLatestPublishedMarketplaceVersionsorts arbitrary Marketplace version strings usingcompareSemver, but it doesn't filter out non-plain semver values. If the Marketplace ever returns a non-x.y.zversion (preview/suffixed),compareSemvercan produceNaNand the "latest" selection becomes unreliable. Filter to plain semver (consistent with Docker tag logic) before sorting.
const filtered = versions
.filter((v) => v !== excludeVersion)
.sort(compareSemver);
tests/upgrade/utils/tableValueCodec.ts:61
@azure/data-tablesretrieves DateTime properties asDateobjects (see existing table tests), but this code coercesfetched.dateProptostringand reparses it. ParsingDate#toString()output is implementation-dependent and can be flaky across environments. Compare usingDatedirectly when available, and only parse if a string was returned.
assert.strictEqual(
new Date(fetched.dateProp as string).getTime(),
entity.dateProp.getTime()
);
.github/workflows/UpgradeCompatibility.yml:12
- PR description says this workflow should not run on
pull_request, but the workflow currently includes apull_requesttrigger (even if intended to be temporary). This will run the expensive upgrade suite on every PR update, contradicting the stated intent. Remove thepull_requesttrigger before merging.
# TEMPORARY: pull_request added to validate this workflow runs correctly on
# PR #2715 before merge. Remove this trigger before merging.
# Runs after a PR is merged to main (push), and on demand via workflow_dispatch.
on:
push:
branches:
- main
pull_request:
workflow_dispatch:
- DockerImageUpgrade_Ubuntu: docker run replaces the image's CMD entirely (no ENTRYPOINT is set), so no CLI args reached the containerized Azurite process. Re-specify the default startup args plus --skipApiVersionCheck so an older published image doesn't reject the SDK client's x-ms-version. - UpgradeCompatibility_Windows: spawning npm.cmd/npx.cmd directly without shell: true throws EINVAL on Windows (Node.js CVE-2024-27980 hardening). Added shell: true for the win32 case in both spawn sites.
…ames - Give every workflow step a descriptive name instead of the default 'Run <command>' label - processHarness.stop(): add a hard upper-bound timeout so a process that ignores SIGKILL can never hang the test run indefinitely - tableValueCodec: normalize Binary read-back to handle either raw bytes or a base64 string depending on SDK serialization shape; compare dateProp using instanceof Date before falling back to string parsing - versionResolver: resolve MCR pagination Link header URLs against an absolute base (they can be relative), and filter Marketplace versions to plain semver before sorting, consistent with the Docker tag logic
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 12:40
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/UpgradeCompatibility.yml:12
- The PR description says this workflow intentionally does not run on
pull_request, but the workflow currently includes apull_request:trigger (marked “TEMPORARY”). This will run the expensive upgrade suite on every PR update unless removed before merge.
# TEMPORARY: pull_request added to validate this workflow runs correctly on
# PR #2715 before merge. Remove this trigger before merging.
# Runs after a PR is merged to main (push), and on demand via workflow_dispatch.
on:
push:
branches:
- main
pull_request:
workflow_dispatch:
tests/upgrade/vsixLifecycle/runVsixTests.ts:61
resolveVsixToTest()can create a temporary directory for a packaged/downloaded VSIX, butrunVsixTests.tsonly cleans up the VS Code user-data / extensions / workspace dirs. This leaves behindazurite-local-vsix-*andazurite-marketplace-vsix-*temp folders on every run.
} finally {
rmSync(userDataDir, { recursive: true, force: true });
rmSync(extensionsDir, { recursive: true, force: true });
rmSync(workspaceDir, { recursive: true, force: true });
}
…ess termination - dockerUpgrade test: reset bind-mounted volume ownership back to the host runner user before removal, since the container writes as root and the CI runner user can't unlink root-owned files (EACCES) - processHarness: use taskkill without /F on Windows to request a graceful shutdown so Azurite can flush/close its persistence layer before exiting; child.kill() on Windows always force-terminates and never lets the SIGTERM handler run
resolveVsixToTest() now returns the temp directory it created (if any) alongside the vsix path, so runVsixTests.ts can remove it in the finally block. Previously azurite-local-vsix-* and azurite-marketplace-vsix-* directories were left behind on every run.
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 13:17
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/upgrade/utils/processHarness.ts:107
- In
stop(), theonExithandler referencesforceKillTimer/giveUpTimerbefore thoseconstbindings are initialized. If the child exits quickly, this can throw aReferenceErrordue to the temporal dead zone. Initialize timer variables before wiringonExit, or useletwith undefined checks and create the timers before callingrequestGracefulStop().
const onExit = () => {
clearTimeout(forceKillTimer);
clearTimeout(giveUpTimer);
resolve();
};
tests/upgrade/vsixLifecycle/runVsixTests.ts:42
resolveCliArgsFromVSCodeExecutablePath()can return a.cmd/.batCLI on Windows, and Node may fail to spawn it withoutshell: true(same issue you already handled fornpx.cmd). Add a Windows-onlyshelloption here sonpm run test:upgrade:vsixis runnable on Windows too.
{ stdio: "inherit" }
.github/workflows/UpgradeCompatibility.yml:12
- The PR description says this workflow intentionally does not run on
pull_request, but the workflow currently includes apull_requesttrigger (and even notes it as temporary). This will add the expensive runs back onto every PR. Remove the temporary comment and thepull_requesttrigger before merging.
# TEMPORARY: pull_request added to validate this workflow runs correctly on
# PR #2715 before merge. Remove this trigger before merging.
# Runs after a PR is merged to main (push), and on demand via workflow_dispatch.
The earlier taskkill-based approach still didn't reliably trigger graceful shutdown on Windows: taskkill without /f only delivers WM_CLOSE, which console (non-GUI) processes don't handle, so it fails and silently falls back to a hard kill - explaining why Windows CI still lost persisted data on stop. src/azurite.ts already supports a 'shutdown' IPC message for exactly this purpose (used elsewhere, e.g. the VS Code extension), which works identically on every platform since it doesn't depend on OS signal delivery at all. Switch to fork() so the IPC channel exists, and send 'shutdown' to request a clean close; SIGKILL remains the last-resort timeout fallback.
- processHarness.stop(): declare forceKillTimer/giveUpTimer with let before onExit is defined, instead of relying on them being assigned later in the same synchronous block before the exit event can fire. Avoids a ReferenceError if that ordering assumption ever breaks. - runVsixTests: resolveCliArgsFromVSCodeExecutablePath() can resolve to a .cmd/.bat wrapper on Windows; add shell: true there too, same as the existing npx.cmd handling.
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 6, 2026 23:37
View session
Copilot started reviewing on behalf of
Akanksha Jain (jainakanksha-msft)
August 7, 2026 06:24
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
tests/upgrade/utils/versionResolver.ts:124
- This has the same upgrade-direction problem as the npm resolver: on an older checkout, the selected MCR tag may be newer than the local image, so the scenario validates a downgrade instead. Filter for tags strictly below the local version.
const filtered = tags
.filter((t) => SEMVER_TAG_PATTERN.test(t) && t !== excludeVersion)
.sort(compareSemver);
.github/workflows/UpgradeCompatibility.yml:12
- The temporary
pull_requesttrigger contradicts both the PR description and this file's own instruction to remove it before merge. As written, all four costly jobs run on every PR update rather than only after merges or on demand.
pull_request:
.github/workflows/UpgradeCompatibility.yml:57
- This job runs
test:upgrade:vsix, whose default mode packages the local tree; it never runs the addedpublished-latestmode. Consequently the dedicated workflow does not install or exercise the latest Marketplace VSIX as claimed in the PR description and changelog. Run both modes (or use a matrix) so local packaging and published-package coverage are both retained.
run: xvfb-run -a npm run test:upgrade:vsix
tests/upgrade/dockerUpgrade.test.ts:180
- The Docker path verifies only the queue's approximate count, so corrupted or changed message values would still pass. This conflicts with the stated value-for-value verification and with the npm queue scenario. Receive and compare the seeded messages here as well.
const queueClient = makeQueueClient(QUEUE_PORT, queueFixture.queueName);
const properties = await queueClient.getProperties();
assert.strictEqual(
properties.approximateMessagesCount,
queueFixture.messages.length,
"Queue message count did not survive the docker image upgrade"
);
tests/upgrade/utils/upgradeTarget.ts:87
- If
docker runfails after creating the container, or any readiness probe rejects,start()exits without removing it. The test callsstart()before itstry/finally, so the named container remains and can keep ports and the bind mount in use. Clean up the container on every startup failure.
runContainer(this.options);
await Promise.all([
tests/upgrade/utils/versionResolver.ts:37
- Excluding only the local version does not guarantee an upgrade. When this suite is run from a stale branch or older commit, the highest remaining published version can be newer than the local build, turning the test into a downgrade. Select the newest stable version strictly below the local baseline.
This issue also appears on line 122 of the same file.
const versions = Object.keys(json.versions ?? {})
.filter((v) => v !== excludeVersion && !v.includes("-"))
.sort(compareSemver);
tests/upgrade/vsixLifecycle/resolveVsixToTest.ts:38
published-latestinvokes a resolver that excludes the local package version. Once that same version is published, this mode deliberately downloads the previous Marketplace release rather than the latest one, despite its name and documented behavior. The Marketplace lifecycle resolver needs a true "latest" mode without local-version exclusion; keep any older-than-local selection separate for upgrade scenarios.
const version =
mode === "published-latest"
? await getLatestPublishedMarketplaceVersion()
: mode;
docs/designs/2026-08-upgrade-compatibility-testing.md:29
- This coverage claim is incorrect. The VSIX lifecycle runner creates a fresh workspace and neither receives the npm suite's data location nor seeds/reads persisted fixtures, so it does not verify that old-version data is readable by a VSIX. Implement the cross-surface persistence scenario or mark this requirement as not covered.
| 2 | Data created by an old version is readable by the latest VSIX | `tests/upgrade/vsixLifecycle/` (installs a VSIX pointed at the same on-disk location seeded by step 1) |
tests/upgrade/vsixLifecycle/runVsixTests.ts:29
- In
@vscode/test-electron2.5.2 this helper adds its own.vscode-test--user-data-dirand--extensions-dirunlessreuseMachineInstallis true. The install command then appends a second pair of those flags, whilerunTestsuses only the custom temp directories. Depending on duplicate-argument handling, the VSIX can be installed into a different profile andgetExtensionwill fail. Suppress the helper defaults so installation and execution unambiguously share the same directories.
const [cli, ...cliArgs] =
resolveCliArgsFromVSCodeExecutablePath(vscodeExecutablePath);
tests/upgrade/utils/processHarness.ts:46
- On a readiness timeout this removes listeners and rejects but leaves the forked Azurite process running. All callers await
start()before entering theirtry/finally, so this failure path leaks the process and can keep ports/data files locked, especially on Windows. Terminate the failed child and wait for its exit before rejecting.
const timer = setTimeout(() => {
cleanup();
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/upgrade/utils/versionResolver.ts:192
- Converting numeric prerelease identifiers to
Numberis not full SemVer ordering for valid identifiers aboveNumber.MAX_SAFE_INTEGER: for example,alpha.9007199254740992andalpha.9007199254740993round to the same value and this comparator returns equality. Compare numeric identifiers as digit strings (length first, then lexicographically) so distinct valid versions remain ordered.
const an = Number(ai);
const bn = Number(bi);
const aIsNum = ai !== "" && !Number.isNaN(an);
const bIsNum = bi !== "" && !Number.isNaN(bn);
if (aIsNum && bIsNum) {
if (an !== bn) return an - bn;
} else if (aIsNum !== bIsNum) {
// Numeric identifiers always have lower precedence than alphanumeric ones.
return aIsNum ? -1 : 1;
tests/upgrade/utils/tableValueCodec.ts:60
- The normalization here can let loss of OData type metadata pass as a successful upgrade. With the current Tables SDK, an annotated Int64 is returned as
bigint, Guid as an EDM wrapper, Binary as bytes, and DateTime asDate; if the@odata.typemetadata disappears, the same wire values arrive as strings, which theseString/unwrap/base64/date fallbacks still accept. Configure the verification clients with type conversion disabled and assert each expected EDMtypeas well as its value, so the claimed typed-property persistence coverage fails when annotations are lost.
String(unwrapTypedValue(fetched.int64Prop)),
entity.int64Prop
);
…en upgradeSuite/index.js Both files were identical apart from which single *.test.js they loaded. Replaced with one index.js that reads AZURITE_VSIX_UPGRADE_PHASE (set by runVsixUpgradeTest.ts per installAndRunVsixSession call) to pick seed.test.js vs. verify.test.js, matching the loader-per-directory pattern already used by suite/index.js.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/upgrade/utils/versionResolver.ts:190
Number()treats valid alphanumeric prerelease identifiers such as1e3as numeric and loses precision for long numeric identifiers, so this comparator does not implement the documented SemVer precedence. For example,alpha.1e3vs.alpha.2e2must be compared lexically, but this code compares 1000 vs. 200. Detect numeric identifiers as digits-only and compare them without floating-point conversion; a regression case for this input would also prevent recurrence.
const an = Number(ai);
const bn = Number(bi);
const aIsNum = ai !== "" && !Number.isNaN(an);
const bIsNum = bi !== "" && !Number.isNaN(bn);
if (aIsNum && bIsNum) {
if (an !== bn) return an - bn;
} else if (aIsNum !== bIsNum) {
…table type-loss detection - versionResolver.ts compareSemver: compare numeric prerelease identifiers as digit strings (length, then lexicographic) instead of via Number() subtraction, which rounds identifiers above Number.MAX_SAFE_INTEGER to the same double and reports distinct valid versions as equal. Added a regression test. - tableValueCodec.ts assertEntityMatchesFixture: query with disableTypeConversion: true and assert each property's @odata.type annotation, not just its value - the previous normalization let a dropped type annotation (e.g. Int64 silently becoming a string) pass as a successful upgrade. Verified against a live Azurite table service: real fixtures pass, and a simulated dropped Int64 annotation is now correctly caught. Updated all three getEntity call sites (tableUpgrade.test.ts, dockerUpgrade.test.ts, verify.test.js).
…iers Latest Copilot suppressed comment (from a review predating f0ecefd) flagged that Number() would coerce a string like "1e3" to 1000, misidentifying an alphanumeric identifier as numeric and comparing it against "2e2" as 1000 vs 200 instead of lexically. f0ecefd's digit-only regex (/^\d+$/) already rejects "1e3" as non-numeric, so this was already fixed - this commit only adds explicit coverage to prevent recurrence.
BlobServerFactory switches to SqlBlobServer whenever process.env.AZURITE_DB is defined at all (isSQL = databaseConnectionString !== undefined), so a developer/CI runner with AZURITE_DB set in their shell would have leaked into the VSIX lifecycle phases via extensionTestsEnv, bypassing the workspace's LokiJS metadata and hitting their external database. Setting it to "" doesn't help, since an empty string is still a defined value. Set it to undefined instead - verified Node's spawn (used internally by @vscode/test-electron's runTests) omits env keys whose value is undefined, so the child process gets no AZURITE_DB at all regardless of what the invoking shell had set.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/upgrade/vsixLifecycle/suite/vsixLifecycle.test.js:97
- The stop assertion negates
probeHttp(), but that helper returnsfalseon a 3-second request timeout as well as on connection refusal. A slow persistence flush or temporarily unresponsive event loop therefore makes this test pass even though the listener is still active, defeating the lifecycle shutdown check. Return a distinct timeout result and keep polling on timeouts; only an explicit connection failure should satisfy the down predicate.
tests/upgrade/utils/httpProbe.ts:38 - A request timeout is treated the same as an explicit connection failure here, so
waitForHttpDown()can return while Azurite is still listening. This matters most in the seed phase, where the published extension'sazurite.closecommand may return before its asynchronous LokiJS flush finishes; if that flush or the event loop takes more than the probe's 3 seconds, the test tears down VS Code early and can either lose the seeded data or falsely claim shutdown completed. Make the probe result distinguish HTTP response, timeout, and connection error, and only consider an explicit connection failure as "down" (continue polling on timeout).
while (Date.now() - start < timeoutMs) {
if (!(await probeOnce(port, path))) {
return;
probeOnce()/probeHttp() collapsed a request timeout and an explicit
connection refusal into the same false/down result, so waitForHttpDown()
(and vsixLifecycle.test.js's equivalent) could report a service as stopped
while it was merely slow to respond - most likely while azurite.close's
async LokiJS flush was still in flight. That races the shutdown check and
can pass the test, or move on to seeding/verifying, before persistence
actually landed.
probeOnce() now returns a tri-state ("up"/"timeout"/"down") instead of
a boolean; waitForHttpDown keeps polling on both "up" and "timeout" and
only stops once a probe gets an explicit connection failure. Verified with a
live server that responds after a 4s delay (longer than the 3s probe
timeout): waitForHttpDown no longer resolves early during the delay and only
resolves once the server is actually closed. waitForHttpUp's behavior is
unchanged (both timeout and down still mean 'not up yet').
Mirrored the same fix in vsixLifecycle.test.js's local probeHttp(), which
can't import the .ts helper (runs inside the VS Code test-electron host).
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/upgrade/utils/tableValueCodec.ts:118
- This conversion maps every raw value except exactly
"true"tofalse, so malformed or corrupted Boolean data such as"0"or"garbage"would incorrectly pass for the fixtures whose expected value is false. Compare the normalized raw value directly withString(entity.boolProp)so invalid values remain detectable.
assert.strictEqual(
String(unwrapAndAssertType(fetched.boolProp, "Boolean", "boolProp")) ===
"true",
entity.boolProp
);
tests/upgrade/utils/processHarness.ts:103
- A child terminated by a signal has
exitCode === nulland a non-nullsignalCode. In that state this guard treats the already-exited process as running, attaches anexitlistener after the event has fired, and eventually rejects after 10 seconds, masking the original test failure. IncludesignalCodein the exited-process check.
if (!child || child.exitCode !== null || child.killed) {
return;
}
- tableValueCodec.ts assertEntityMatchesFixture: comparing String(raw) === "true" against entity.boolProp let any non-"true" raw value (e.g. "0", "garbage") pass as long as the fixture expected false - malformed data was indistinguishable from a correct false. Compare the normalized raw string directly against String(entity.boolProp) so only "true"/"false" match their respective expectation. - processHarness.ts stop(): the already-exited guard only checked exitCode and killed, missing that a process terminated by a signal (e.g. an external SIGTERM, not sent via child.kill()) has exitCode === null, signalCode !== null, and killed === false. Verified with a real child process signaled externally (process.kill(pid, "SIGTERM") rather than child.kill()): the old guard evaluated false (treats it as still running) while the new one correctly evaluates true. Without this, stop() would attach an exit listener after the event already fired and wait out the full 10s giveUpTimer before rejecting, masking the real failure.
VS Code needs publisher to construct the extension's id (publisher.name) when loading a directory via extensionDevelopmentPath, regardless of whether the extension is ever actually published - an invalid manifest would prevent the Extension Host test entry point from loading and fail every VSIX phase before Azurite is even exercised. This is a local-only identifier, not a real marketplace publisher account; private: true already prevents this from being published for real.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 42 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/upgrade/vsixLifecycle/upgradeSuite/seed.test.js:145
- Waiting for the ports to go down does not prove that the published extension finished persisting.
ServerBase.close()stops the HTTP server before awaitingafterClose()(src/common/ServerBase.ts:140-145), and the currently published VSIX uses the old fire-and-forget close command. These probes can therefore succeed while the Lokidb.close()callbacks are still pending; once this test returns, the VS Code process may terminate mid-flush and make the upgrade phase flaky or lose seeded data. Wait for an actual persistence-completion signal/artifact before ending the seed session.
tests/upgrade/utils/versionResolver.ts:171 - The comparator is still incorrect for valid SemVer core components larger than
Number.MAX_SAFE_INTEGER.parseSemver()converts major/minor/patch tonumber, so, for example,9007199254740992.0.0and9007199254740993.0.0round to the same value and compare equal. Since this function is documented as full SemVer ordering (and already handles oversized numeric prerelease identifiers), compare core components as digit strings or big integers too.
if (pa.major !== pb.major) return pa.major - pb.major;
if (pa.minor !== pb.minor) return pa.minor - pb.minor;
if (pa.patch !== pb.patch) return pa.patch - pb.patch;
parseSemver converted major/minor/patch to Number, so core components above Number.MAX_SAFE_INTEGER (e.g. 9007199254740992.0.0 vs 9007199254740993.0.0) rounded to the same double and compared as equal - the same precision-loss bug already fixed for numeric prerelease identifiers. Keep major/minor/patch as digit strings and compare them with the same length-then-lexicographic compareNumericString helper already used for prerelease identifiers (factored out, no behavior change there). Added a regression test covering all three positions.
waitForHttpDown only proves the HTTP listener stopped accepting connections, not that persistence landed on disk: ServerBase.close() calls httpServer.stop() *before* afterClose() closes the metadata/extent stores (src/common/ServerBase.ts), and the published Marketplace vsix this phase runs may not even await its close command's promise before returning. If the seed VS Code session ends - and gets torn down - while that flush is still in flight, the verify phase's brand-new VS Code process (reading the same on-disk workspace) can see lost or corrupted data. Added fileStability.ts's waitForFileStable(), which polls a file's mtime until it stops changing for a settle window - the closest an external process can get to observing an async write actually finishing. Verified it correctly waits out a file under active writes (doesn't resolve early) and throws for a file that's never created. seed.test.js now waits for all three Loki metadata files (__azurite_db_blob__.json, __azurite_db_queue__.json, __azurite_db_table__.json) to stabilize after the ports go down, before ending the seed session. Also corrected httpProbe.ts's waitForHttpDown doc comment, which incorrectly claimed a closed port proves the flush completed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/upgrade/utils/fileStability.ts:16
- The default quiet window is only 1 second, but every Loki metadata store autosaves on a 5-second interval (for example,
src/blob/persistence/LokiBlobMetadataStore.ts:129). A file can therefore remain unchanged for 1 second and be reported as stable even though a pending autosave/close write has not occurred yet, allowing the seed VS Code process to be torn down before persistence completes. Use a quiet period longer than the writer interval (or require observing the post-close write before declaring stability).
stableForMs = 1000,
All Loki metadata stores autosave on a 5000ms interval (e.g. LokiBlobMetadataStore.ts's autosaveInterval: 5000). With the previous 1000ms default, a file could sit unchanged for exactly one second between two periodic autosave ticks and be reported stable even though a pending write is still due at the next tick - letting the seed VS Code process get torn down before that write lands. Default stableForMs is now LOKI_AUTOSAVE_INTERVAL_MS + 1000 (6000ms), a full autosave cycle plus margin, so a genuinely quiet file has to survive past the point another autosave would have fired. Verified with a standalone script simulating a write 2s after the initial one: the default now waits ~8s total (past both the write and the new 6s window) instead of resolving after the old 1s window.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/upgrade/utils/httpProbe.ts:71
- This treats every request error as an explicit port closure, although
waitForHttpDownpromises to return only on connection refusal. A transientECONNRESET(for example while the server is draining sockets) can therefore make the shutdown check pass while the listener is still bound. Only classifyECONNREFUSEDasdown; keep polling for other errors.
req.on("error", () => resolve("down"));
tests/upgrade/vsixLifecycle/suite/vsixLifecycle.test.js:41
- The stop assertion says only connection refusal counts as
down, but this maps any request error—including a transientECONNRESETwhile sockets are draining—todown. That can let the lifecycle test pass before the listener is actually closed. Only classifyECONNREFUSEDasdownand continue polling for other errors.
Both httpProbe.ts's probeOnce and vsixLifecycle.test.js's duplicated probeHttp resolved "down" for *any* request error, including a transient ECONNRESET while the server is draining sockets mid-shutdown. That let waitForHttpDown/waitUntil(...=== "down") pass while the listener was still bound, racing the actual close. Now only err.code === "ECONNREFUSED" resolves "down"; every other error resolves "timeout" (inconclusive - keep polling), matching the existing timeout handling already in place for slow/unresponsive probes. Verified with a standalone script: a real socket that resets the connection (ECONNRESET) is classified as "timeout", not "down", while an actually-refused port is still correctly classified as "down".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/upgrade/utils/npmVersionInstaller.ts:137
- The Windows upgrade job deletes this large npm installation with a single
rmSync, so a transientEPERM/EBUSYfrom antivirus or a just-released file handle can fail an otherwise successful suite during the root hook. The repository’s test cleanup helper explicitly retries this case (tests/testutils.ts:131-140); apply the same Windows retry policy here.
rmSync(installed.installDir, { recursive: true, force: true });
cleanupCachedNpmInstalls() deleted each large installed npm package with a single rmSync, so a transient EPERM/EBUSY from antivirus or a just-released file handle during this root hook could fail an otherwise successful suite on Windows runners. Apply the same retry/swallow policy tests/testutils.ts's rmRecursive already uses: maxRetries on Windows, and swallow the cleanup error so teardown stays non-flaky. Verified npx tsc --noEmit passes and that rmSync with maxRetries set still removes a normal directory correctly.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/upgrade/utils/dockerHarness.ts:85
- If
docker stopfails for any reason other than an already-absent container, this immediate rethrow skips the subsequentdocker rm -f. The failure path can therefore leave a running container holding the test ports and bind mount, and the suite'safterhook does not remove either generated container name. Preserve the stop error, attempt the forced removal, then rethrow so the persistence failure remains visible without leaking the container.
const stderr = (err as { stderr?: Buffer }).stderr?.toString() ?? "";
if (!/no such container/i.test(stderr)) {
throw err;
stopAndRemoveContainer() rethrew a genuine docker stop failure immediately, skipping the subsequent docker rm -f. That could leave a running container holding the test ports and bind mount, and the suite's after hook doesn't separately remove either generated container name - so the failure would also leak the container across runs. Now the stop error is preserved (unless it's the expected 'no such container' from the pre-emptive cleanup call in start()), rm -f is attempted unconditionally, and the original stop error is rethrown afterward so the persistence failure stays visible without leaking the container. Verified with a fake execFileSync: a real stop failure still runs rm -f and rethrows afterward, while the existing no-such-container path still no-ops as before.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/upgrade/vsixLifecycle/upgradeSuite/seed.test.js:41
- The seed phase is running the published VSIX, but it derives the files to await from the local build's constants. If a release changes a persistence filename while adding migration support for the old name, this phase waits for a file the published VSIX never creates and fails before the local VSIX can exercise that migration. That breaks the suite's version-agnostic guarantee; determine the files written by the seed phase independently of the local generation (and include the extent metadata stores), or use a shutdown-completion signal that does not depend on filenames.
seed.test.js derived the files to await stability on from the LOCAL
build's dist/src/{blob,queue,table}/utils/constants, but this phase runs
the published Marketplace VSIX. If a future release renamed a
metadata/extent store filename (even while adding migration support for
the old name), this phase would wait for a file the published VSIX never
creates and fail before the local VSIX ever got to exercise that
migration - breaking the suite's version-agnostic guarantee. It also
never covered the extent metadata stores, only the top-level metadata DB.
Replaced fileStability.ts's single-file waitForFileStable with
waitForDirectoryStable, which recursively tracks the latest mtime across
an entire directory tree instead of specific filenames - agnostic to
whatever gets renamed, added, or removed across versions, and naturally
covers the extent stores alongside the metadata stores. seed.test.js now
calls it once on the whole workspace directory instead of importing
local-build constants and waiting on three hardcoded paths.
Verified with a standalone script that a write in a nested subdirectory
(mirroring __blobstorage__/...) is tracked, not just top-level files, and
that it still throws when the directory never appears.
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
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.
Summary
Adds a new
tests/upgrade/regression suite plus a dedicated CI workflow that validates Azurite's in-place upgrade / persistence compatibility across releases:.vsix, activate, start all three services, stop them.The suite is intentionally version-agnostic — nothing hardcodes an "old" version number;
versionResolver.tsalways resolves the latest published npm/Marketplace/MCR version at run time, so it keeps working release after release with no maintenance.What's included
tests/upgrade/blobUpgrade.test.ts,queueUpgrade.test.ts,tableUpgrade.test.ts— npm-based cross-version upgrade tests.tests/upgrade/dockerUpgrade.test.ts— Docker/MCR image upgrade test (same bind-mounted volume across image tags).tests/upgrade/vsixLifecycle/— real VS Code extension install/activate/start/stop lifecycle test using@vscode/test-electron.tests/upgrade/utils/:upgradeTarget.ts—UpgradeTargetinterface withNpmProcessTarget/DockerContainerTargetadapters, so npm and Docker scenarios share identical start/stop orchestration.blobUploader.ts/tableValueCodec.ts— shared fixture upload/seed/verify logic, used by both npm and Docker tests so Docker gets full fixture parity (all blob types, all typed table properties).httpProbe.ts,dockerHarness.ts,processHarness.ts,versionResolver.ts,npmVersionInstaller.ts,dataFixtures.ts,integrity.ts..github/workflows/UpgradeCompatibility.yml— runs on push tomain(i.e. after a PR merges) and on demand viaworkflow_dispatch, with jobs for npm (Ubuntu + Windows), VSIX lifecycle (Ubuntu + xvfb), and Docker image upgrade (Ubuntu).test:upgrade,test:upgrade:docker,test:upgrade:vsix(chains local lifecycle, published lifecycle, and the published->local upgrade phases in one command).@vscode/test-electron.docs/designs/2026-08-upgrade-compatibility-testing.md.ChangeLog.mdupdated underGeneral:.Testing
npm run build— clean.npm run test:upgrade— blob/queue/table npm-based upgrade suites pass locally.test:upgrade:dockerandtest:upgrade:vsixare implemented and type-check cleanly but haven't been executed in this dev environment (no Docker CLI / no display available); they're expected to run in the CI workflow'sDockerImageUpgrade_UbuntuandVsixLifecycle_Ubuntujobs (Docker preinstalled,xvfb-runused for the display).Why not run on every PR
The CI workflow deliberately does not trigger on
pull_requestto avoid paying the npm-install / Docker-pull / VS Code download cost on every push to an open PR. It runs once per merge tomain, plus on demand viaworkflow_dispatch.