From 4e00e2540ca70f97e5589fd893bc93cb0cebabfb Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Wed, 21 Oct 2026 09:27:03 +0000 Subject: [PATCH 1/4] test: make SRV resolver checks reliable on restricted runners Avoid OS-dependent UDP refusal signals, wall-clock deadlines, TCP write timing, and random-ID assumptions. Guard DNS isolation and clean up stub startup failures while preserving loopback transport coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../features/srv-dns-fallback/README.md | 14 +++ .../src/fallbackDnsClient.test.ts | 75 +++++++++---- .../src/resolveSrvForDriver.test.ts | 17 +-- .../src/srvUriRewrite.differential.test.ts | 17 ++- .../src/test/dnsStubServer.test.ts | 98 +++++++++++++++++ .../src/test/dnsStubServer.ts | 104 +++++++++--------- 6 files changed, 239 insertions(+), 86 deletions(-) create mode 100644 packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts diff --git a/docs/ai-and-plans/features/srv-dns-fallback/README.md b/docs/ai-and-plans/features/srv-dns-fallback/README.md index d13ce79e9..cf7eec8dc 100644 --- a/docs/ai-and-plans/features/srv-dns-fallback/README.md +++ b/docs/ai-and-plans/features/srv-dns-fallback/README.md @@ -85,6 +85,20 @@ Increment 1 ([plan](./iterations/01-srv-fallback/plan.md), progress recorded per | `docs/user-manual/troubleshooting-srv-dns.md` | Docs | Symptoms, confirmation, workarounds, timing, trace, switches | | `src/services/localQuickStart/QuickStartService.ts`, `connectToClient.ts` | — | Out of scope (D0007); `connectToClient` is deprecated | +## Test isolation + +The resolver package tests do not query external DNS servers. Transport tests use UDP/TCP +stubs bound to `127.0.0.1`; system DNS server discovery and driver resolver probes are mocked. +Transport tests freeze both the clock and resolver timers, advancing time only for timeout +scenarios, so slow loopback I/O cannot exhaust the resolver's time budget. TCP frame assembly +is tested by splitting client data events into individual bytes, not by delaying server writes. +Retry tests do not assume randomly generated query IDs differ. Stub startup cleans up partial +binds and retries address-in-use conflicts up to three attempts; other bind failures remain errors. +The UDP refusal tests inject `ECONNREFUSED` and `ECONNRESET` without sending a datagram, and +check immediate rejection and timer cleanup without advancing fake timers. They do not rely on +closed-port ICMP errors, which may be suppressed on Windows or restricted runners. +Loopback sockets must still be permitted. + ## Timeline | Date | PR | What changed | Docs | diff --git a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts index 4faea677f..c00cb5721 100644 --- a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts +++ b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts @@ -3,11 +3,12 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as dnsPacket from 'dns-packet'; import dgram from 'node:dgram'; import dns from 'node:dns'; +import net from 'node:net'; import { FallbackDnsError, resolveSrvFallback, resolveTxtFallback } from './fallbackDnsClient.js'; import { DnsStubServer } from './test/dnsStubServer.js'; @@ -72,11 +73,22 @@ function compressedReply(query: dnsPacket.Packet, targets = ['fc-abc-000']): Buf describe('fallback DNS client', (): void => { const stubs: DnsStubServer[] = []; + beforeEach((): void => { + vi.useFakeTimers({ toFake: ['Date', 'setTimeout', 'clearTimeout'] }); + vi.spyOn(dns, 'getServers').mockImplementation((): string[] => { + throw new Error('Tests must explicitly supply loopback DNS servers'); + }); + }); + afterEach(async (): Promise => { - vi.useRealTimers(); - await Promise.all(stubs.map((stub): Promise => stub.close())); - stubs.length = 0; - vi.restoreAllMocks(); + try { + await Promise.all(stubs.map((stub): Promise => stub.close())); + expect(vi.getTimerCount()).toBe(0); + } finally { + stubs.length = 0; + vi.restoreAllMocks(); + vi.useRealTimers(); + } }); async function stub(handler: (request: StubRequest) => void): Promise { @@ -119,7 +131,7 @@ describe('fallback DNS client', (): void => { ); }); - it('ignores replies that do not match the query or come from another source', async (): Promise => { + it('ignores malformed replies and replies that do not match the query', async (): Promise => { const server = await stub((request): void => { const wrongId = reply(request); wrongId.writeUInt16BE(((request.query.id ?? 0) + 1) % 65536, 0); @@ -133,7 +145,7 @@ describe('fallback DNS client', (): void => { }), ); request.send(Buffer.from([0xff, 0xff, 0xff])); - server.later((): void => request.send(reply(request)), 10); + request.send(reply(request)); }); const result = await resolveSrvFallback(srvName, options([server])); expect(result.records.map((record): string => record.name)).toEqual([targetName]); @@ -144,13 +156,32 @@ describe('fallback DNS client', (): void => { if (request.transport === 'udp') { request.send(reply(request, [], dnsPacket.TRUNCATED_RESPONSE)); } else { - request.send(compressedReply(request.query), true); + request.send(compressedReply(request.query)); } }); + const emit = net.Socket.prototype.emit; + let fragments = 0; + // TCP can coalesce writes, so split the client's data events rather than timing server writes. + vi.spyOn(net.Socket.prototype, 'emit').mockImplementation(function ( + this: net.Socket, + event: string | symbol, + ...args: unknown[] + ): boolean { + if (event === 'data' && `127.0.0.1:${this.remotePort}` === server.address && Buffer.isBuffer(args[0])) { + const bytes = args[0]; + for (let offset = 0; offset < bytes.length; offset++) { + fragments++; + emit.call(this, event, bytes.subarray(offset, offset + 1)); + } + return true; + } + return emit.call(this, event, ...args); + }); const result = await resolveSrvFallback(srvName, options([server])); expect(result).toMatchObject({ transport: 'tcp', serversTried: 1 }); expect(result.records[0].name).toBe(targetName); expect(server.queries.map((request): string => request.transport)).toEqual(['udp', 'tcp']); + expect(fragments).toBeGreaterThan(2); }); it.each([ @@ -180,7 +211,6 @@ describe('fallback DNS client', (): void => { }); it('tries the next server after a silent server times out', async (): Promise => { - vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); const first = await stub((): void => { vi.advanceTimersByTime(25); }); @@ -190,7 +220,6 @@ describe('fallback DNS client', (): void => { }); it('retries a server once after a UDP timeout', async (): Promise => { - vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); const server = await stub((request): void => { if (server.queries.length === 1) { vi.advanceTimersByTime(25); @@ -202,7 +231,6 @@ describe('fallback DNS client', (): void => { expect(result.serversTried).toBe(1); expect(server.queries).toHaveLength(2); - expect(server.queries[0].query.id).not.toBe(server.queries[1].query.id); }); it('does not retry a server after SERVFAIL', async (): Promise => { @@ -226,7 +254,6 @@ describe('fallback DNS client', (): void => { }); it('bounds all servers by one shared deadline', async (): Promise => { - vi.useFakeTimers({ toFake: ['Date', 'setTimeout', 'clearTimeout'] }); const first = await stub((): void => { vi.advanceTimersByTime(200); }); @@ -272,23 +299,25 @@ describe('fallback DNS client', (): void => { await expect(outcome).rejects.toMatchObject({ code: dns.CONNREFUSED, syscall: 'queryTxt', serversTried: 0 }); }); - it('fails fast when a UDP server refuses the query', async (): Promise => { - const port = await new Promise((resolve, reject): void => { - const socket = dgram.createSocket('udp4'); - socket.once('error', reject); - socket.bind(0, '127.0.0.1', (): void => { - const address = socket.address(); - socket.close((): void => resolve(address.port)); + it.each(['ECONNREFUSED', 'ECONNRESET'])('fails fast when a UDP socket reports %s', async (code): Promise => { + const connect = vi.spyOn(dgram.Socket.prototype, 'connect'); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + // Closed-port ICMP errors are not reliably delivered on every OS or restricted runner. + const send = vi.spyOn(dgram.Socket.prototype, 'send').mockImplementation(function (this: dgram.Socket): void { + queueMicrotask((): void => { + this.emit('error', Object.assign(new Error(code), { code })); }); }); - const started = Date.now(); const outcome = resolveSrvFallback(srvName, { - servers: [`127.0.0.1:${port}`], - deadlineAt: started + 3000, + servers: ['127.0.0.1:53'], + deadlineAt: Date.now() + 3000, udpTimeoutMs: 2000, }); await expect(outcome).rejects.toMatchObject({ code: dns.CONNREFUSED, serversTried: 1 }); - expect(Date.now() - started).toBeLessThan(1000); + expect(connect).toHaveBeenCalledExactlyOnceWith(53, '127.0.0.1', expect.any(Function)); + expect(send).toHaveBeenCalledTimes(1); + expect(close).toHaveBeenCalledTimes(1); + expect(vi.getTimerCount()).toBe(0); }); }); diff --git a/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts b/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts index 925131be0..b2243d2c8 100644 --- a/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts +++ b/packages/documentdb-js-srv-resolver/src/resolveSrvForDriver.test.ts @@ -164,17 +164,20 @@ describe('resolveSrvForDriver', (): void => { it('cancels a pending probe and rejects with the abort reason', async (): Promise => { const controller = new AbortController(); let rejectProbe!: (error: unknown) => void; - probe.mockImplementation( - (): Promise => - new Promise((_resolve, reject): void => { - rejectProbe = reject; - }), - ); + const probeStarted = new Promise((resolve): void => { + probe.mockImplementation( + (): Promise => + new Promise((_resolve, reject): void => { + rejectProbe = reject; + resolve(); + }), + ); + }); const cancel = vi .spyOn(dns.promises.Resolver.prototype, 'cancel') .mockImplementation((): void => rejectProbe(probeError('ECANCELLED'))); const result = run({ signal: controller.signal }); - await Promise.resolve(); + await probeStarted; const reason = new Error('stop'); controller.abort(reason); await expect(result).rejects.toBe(reason); diff --git a/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts b/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts index 2ed98ea67..1edd33084 100644 --- a/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts +++ b/packages/documentdb-js-srv-resolver/src/srvUriRewrite.differential.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { MongoClient } from 'mongodb'; import * as dns from 'node:dns'; @@ -167,6 +167,12 @@ function rewriteOutcome(fixture: Fixture): Outcome { } describe('SRV rewrite differential against the installed MongoDB API driver', (): void => { + beforeEach((): void => { + const error = new Error('Driver DNS queries must use the mocked resolve function'); + vi.spyOn(dns.promises, 'resolveSrv').mockRejectedValue(error); + vi.spyOn(dns.promises, 'resolveTxt').mockRejectedValue(error); + }); + afterEach((): void => { vi.restoreAllMocks(); }); @@ -178,10 +184,13 @@ describe('SRV rewrite differential against the installed MongoDB API driver', () if (rrtype === 'SRV') { return srvRecords.map((record): SrvRecord => ({ ...record })); } - if (fixture.txtRecords === undefined) { - throw Object.assign(new Error('queryTxt ENODATA'), { code: 'ENODATA' }); + if (rrtype === 'TXT') { + if (fixture.txtRecords === undefined) { + throw Object.assign(new Error('queryTxt ENODATA'), { code: 'ENODATA' }); + } + return fixture.txtRecords.map((record): string[] => [...record]); } - return fixture.txtRecords.map((record): string[] => [...record]); + throw new Error(`Unexpected DNS record type: ${rrtype}`); }, ); const expected = await driverOutcome(fixture); diff --git a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts new file mode 100644 index 000000000..3a9ac7ae9 --- /dev/null +++ b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.test.ts @@ -0,0 +1,98 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import dgram from 'node:dgram'; +import net from 'node:net'; + +import { DnsStubServer } from './dnsStubServer.js'; + +describe('DNS stub server lifecycle', (): void => { + const stubs: DnsStubServer[] = []; + + afterEach(async (): Promise => { + try { + await Promise.all(stubs.map((stub): Promise => stub.close())); + } finally { + stubs.length = 0; + vi.restoreAllMocks(); + } + }); + + it('retries a TCP port conflict after closing the partially started stub', async (): Promise => { + const error = Object.assign(new Error('port in use'), { code: 'EADDRINUSE' }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen').mockImplementationOnce(function ( + this: net.Server, + ): net.Server { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + + const server = await DnsStubServer.start((): void => {}); + stubs.push(server); + + expect(server.address).toMatch(/^127\.0\.0\.1:\d+$/); + expect(listen).toHaveBeenCalledTimes(2); + expect(close).toHaveBeenCalledTimes(1); + }); + + it.each([ + ['EACCES', 1], + ['EADDRINUSE', 3], + ])('cleans up and reports %s after %i attempts', async (code, attempts): Promise => { + const error = Object.assign(new Error(code), { code }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen').mockImplementation(function ( + this: net.Server, + ): net.Server { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + + await expect(DnsStubServer.start((): void => {})).rejects.toBe(error); + + expect(listen).toHaveBeenCalledTimes(attempts); + expect(close).toHaveBeenCalledTimes(attempts); + }); + + it('preserves UDP bind errors without trying to close unbound sockets', async (): Promise => { + const error = Object.assign(new Error('bind denied'), { code: 'EACCES' }); + const bind = vi.spyOn(dgram.Socket.prototype, 'bind').mockImplementationOnce(function ( + this: dgram.Socket, + ): dgram.Socket { + queueMicrotask((): void => { + this.emit('error', error); + }); + return this; + }); + const close = vi.spyOn(dgram.Socket.prototype, 'close'); + const listen = vi.spyOn(net.Server.prototype, 'listen'); + + await expect(DnsStubServer.start((): void => {})).rejects.toBe(error); + + expect(bind).toHaveBeenCalledTimes(1); + expect(close).not.toHaveBeenCalled(); + expect(listen).not.toHaveBeenCalled(); + }); + + it('can close an already closed stub', async (): Promise => { + const server = await DnsStubServer.start((): void => {}); + stubs.push(server); + const closeUdp = vi.spyOn(dgram.Socket.prototype, 'close'); + const closeTcp = vi.spyOn(net.Server.prototype, 'close'); + + await server.close(); + await server.close(); + + expect(closeUdp).toHaveBeenCalledTimes(1); + expect(closeTcp).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts index b7a16c291..4f9b6bb8f 100644 --- a/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts +++ b/packages/documentdb-js-srv-resolver/src/test/dnsStubServer.ts @@ -12,7 +12,7 @@ import { type DnsTransport } from '../fallbackDnsClient.js'; export interface StubRequest { readonly query: dnsPacket.DecodedPacket; readonly transport: DnsTransport; - send(bytes: Buffer, split?: boolean): void; + send(bytes: Buffer): void; } export class DnsStubServer { @@ -21,7 +21,7 @@ export class DnsStubServer { public address = ''; private readonly udp = dgram.createSocket('udp4'); private readonly tcp = net.createServer(); - private readonly timers = new Set(); + private udpBound = false; private closed = false; private constructor(private readonly handler: (request: StubRequest) => void) { @@ -50,21 +50,10 @@ export class DnsStubServer { } const bytes = received.subarray(2, length + 2); received = received.subarray(length + 2); - this.dispatch(bytes, 'tcp', (response, split): void => { + this.dispatch(bytes, 'tcp', (response): void => { const prefix = Buffer.alloc(2); prefix.writeUInt16BE(response.length); - const framed = Buffer.concat([prefix, response]); - if (!split) { - socket.write(framed); - return; - } - socket.write(framed.subarray(0, 1)); - this.later((): void => { - socket.write(framed.subarray(1, 5)); - }, 5); - this.later((): void => { - socket.write(framed.subarray(5)); - }, 10); + socket.write(Buffer.concat([prefix, response])); }); } }); @@ -72,55 +61,66 @@ export class DnsStubServer { } public static async start(handler: (request: StubRequest) => void): Promise { - const stub = new DnsStubServer(handler); - await new Promise((resolve, reject): void => { - stub.udp.once('error', reject); - stub.udp.bind(0, '127.0.0.1', (): void => { - stub.udp.removeListener('error', reject); - resolve(); - }); - }); - const port = stub.udp.address().port; - await new Promise((resolve, reject): void => { - stub.tcp.once('error', reject); - stub.tcp.listen(port, '127.0.0.1', (): void => { - stub.tcp.removeListener('error', reject); - resolve(); - }); - }); - stub.address = `127.0.0.1:${port}`; - return stub; - } - - public later(callback: () => void, delayMs: number): void { - const timer = setTimeout((): void => { - this.timers.delete(timer); - if (!this.closed) { - callback(); + for (let attempt = 0; ; attempt++) { + const stub = new DnsStubServer(handler); + try { + await new Promise((resolve, reject): void => { + stub.udp.once('error', reject); + stub.udp.bind(0, '127.0.0.1', (): void => { + stub.udpBound = true; + stub.udp.removeListener('error', reject); + resolve(); + }); + }); + const port = stub.udp.address().port; + await new Promise((resolve, reject): void => { + stub.tcp.once('error', reject); + stub.tcp.listen(port, '127.0.0.1', (): void => { + stub.tcp.removeListener('error', reject); + resolve(); + }); + }); + stub.address = `127.0.0.1:${port}`; + return stub; + } catch (error) { + await stub.close(); + // A UDP-assigned port can already be occupied by an unrelated TCP listener. + if (!(error instanceof Error && 'code' in error && error.code === 'EADDRINUSE' && attempt < 2)) { + throw error; + } } - }, delayMs); - this.timers.add(timer); + } } public async close(): Promise { - this.closed = true; - for (const timer of this.timers) { - clearTimeout(timer); + if (this.closed) { + return; } + this.closed = true; for (const socket of this.tcpSockets) { socket.destroy(); } await Promise.all([ - new Promise((resolve): void => { - this.udp.close((): void => resolve()); - }), - new Promise((resolve): void => { - this.tcp.close((): void => resolve()); - }), + this.udpBound + ? new Promise((resolve): void => { + this.udp.close((): void => resolve()); + }) + : Promise.resolve(), + this.tcp.listening + ? new Promise((resolve, reject): void => { + this.tcp.close((error): void => { + if (error) { + reject(error); + } else { + resolve(); + } + }); + }) + : Promise.resolve(), ]); } - private dispatch(bytes: Buffer, transport: DnsTransport, send: (response: Buffer, split?: boolean) => void): void { + private dispatch(bytes: Buffer, transport: DnsTransport, send: (response: Buffer) => void): void { const request: StubRequest = { query: dnsPacket.decode(bytes), transport, send }; this.queries.push(request); this.handler(request); From cc381cc31f7510433ec185c1b4810aebeede7968 Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Wed, 21 Oct 2026 09:41:03 +0000 Subject: [PATCH 2/4] test: keep TCP event mock return type safe for lint Normalize the forwarded emit result to a boolean because the root non-strict TypeScript context gives Function.call an unchecked return type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts index c00cb5721..f092a15e9 100644 --- a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts +++ b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts @@ -175,7 +175,7 @@ describe('fallback DNS client', (): void => { } return true; } - return emit.call(this, event, ...args); + return emit.call(this, event, ...args) === true; }); const result = await resolveSrvFallback(srvName, options([server])); expect(result).toMatchObject({ transport: 'tcp', serversTried: 1 }); From 9477396f5160dbc2ef3c7bb37afb0cfd0507f1a1 Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Wed, 21 Oct 2026 09:53:53 +0000 Subject: [PATCH 3/4] test: type TCP emitter forwarding for strict package checks CI typechecks package tests with strictBindCallApply, which resolves the overloaded Socket.emit call signature differently from the root build. Give the saved emitter an explicit rest-argument signature so forwarding is valid under both configurations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts index f092a15e9..f12044b74 100644 --- a/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts +++ b/packages/documentdb-js-srv-resolver/src/fallbackDnsClient.test.ts @@ -159,7 +159,8 @@ describe('fallback DNS client', (): void => { request.send(compressedReply(request.query)); } }); - const emit = net.Socket.prototype.emit; + const emit: (this: net.Socket, event: string | symbol, ...args: unknown[]) => boolean = + net.Socket.prototype.emit; let fragments = 0; // TCP can coalesce writes, so split the client's data events rather than timing server writes. vi.spyOn(net.Socket.prototype, 'emit').mockImplementation(function ( From 70867505815c0ff62774644d9ea391b50a9032ed Mon Sep 17 00:00:00 2001 From: Tomasz Naumowicz Date: Wed, 21 Oct 2026 09:56:24 +0000 Subject: [PATCH 4/4] docs: align Case 2 checks with CI strict test validation Require the exact workspace test typecheck instead of inferring it from build or Vitest success. Always run CI's non-mutating localization check, and build workspace outputs before strict test validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/copilot-instructions.md | 14 +++++++++++--- CONTRIBUTING.md | 16 +++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index adbfa0a26..befbb0f4f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -50,14 +50,22 @@ is missing. Report all blockers; do not mark the PR ready until the checks pass and the other requirements are satisfied. If there is no PR, stay on Case 1. ```bash -npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed +npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed npm run prettier-fix +npm run l10n:check # always; CI validates localization even when generation is skipped npm run lint -npx vitest run # full suite npm run build -npm run package # catches bundling and missing-asset failures +npm run typecheck:tests # strict package-test checks across all workspaces, as in CI +npx vitest run # full runtime suite; does not replace typecheck:tests +npm run package # catches bundling and missing-asset failures ``` +`npm run build` checks the root project and package source, but does not check package +tests under their stricter `tsconfig.test.json` settings. Vitest runtime success (or +an ad hoc `vitest --typecheck` invocation) is not a substitute for the exact +`npm run typecheck:tests` script. Keep this list aligned with the +[CI static-check job](workflows/main.yml), and report that script's result separately. + Also confirm the AI pre-review (CONTRIBUTING.md §6) has run and its review file is committed under `docs/ai-and-plans/features//iterations/`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e8f9bec79..ee68af67c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -248,14 +248,22 @@ Nothing else. `l10n`, `prettier-fix`, `lint`, and `package` each cost minutes an All of these must pass locally. The same checks run in CI, so catching failures here saves time. ```bash -npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed +npm run l10n # only if a vscode.l10n.t() string was added, changed, or removed npm run prettier-fix +npm run l10n:check # always; CI validates localization even when generation is skipped npm run lint -npx vitest run # full suite npm run build -npm run package # catches bundling and missing-asset failures +npm run typecheck:tests # strict package-test checks across all workspaces, as in CI +npx vitest run # full runtime suite; does not replace typecheck:tests +npm run package # catches bundling and missing-asset failures ``` +`npm run build` checks the root project and package source, but does not check package +tests under their stricter `tsconfig.test.json` settings. Vitest runtime success (or +an ad hoc `vitest --typecheck` invocation) is not a substitute for the exact +`npm run typecheck:tests` script. Keep this list aligned with the +[CI static-check job](.github/workflows/main.yml), and report that script's result separately. + Commit whatever these change, including regenerated files under `l10n/` and anything Prettier reformats. Case 2 is also where the AI pre-review in [§6](#6-ai-assisted-review-workflow) is due. Do not mark a PR ready for human review until that pass has run and its review file is committed under the feature's `iterations/` folder. @@ -263,7 +271,9 @@ Case 2 is also where the AI pre-review in [§6](#6-ai-assisted-review-workflow) ### 4.3. Notes on individual steps - **`npm run l10n`** regenerates the localization bundle from the strings passed to `vscode.l10n.t()`. **Never resolve a conflict in `l10n/bundle.l10n.json` by hand** — the file is generated. Take either side, or delete it, then re-run `npm run l10n` and commit the result. +- **`npm run l10n:check`** validates localization against the source without regenerating files. Run it even when no strings changed, matching CI. - **`npm run prettier-fix`** covers Markdown as well as source, so documentation formatting does not drift whenever a file is touched. Fenced code blocks are left exactly as authored: they are often partial or illustrative, and reformatting them to the Markdown tab width would leave every example disagreeing with the code it documents. +- **`npm run typecheck:tests`** checks every participating workspace's tests with its own TypeScript configuration, including strict checks that the root build and Vitest runtime do not cover. - **`npm run package`** catches Vite host and webview bundling issues and missing assets that unit tests alone will not surface. ## 5. Documenting Work with AI