Skip to content

Commit 0089fb1

Browse files
thymikeethymikee
committed
test(host-kit): one double answers every group write in these tests
`guardGroupWrites` had been introduced as the seam every group write is answered at, and then two tests beside it went on answering the same question with hand-written `vi.spyOn(process, 'kill')` doubles. Two doubles for one seam is a slow disagreement waiting to happen: the spy said delivery and the guard said delivery differently, and only one of them was checked against what `process.kill` really answers. Both are built on the guard now, and the three answers it can give — `true` for a delivered write, `ESRCH`, `EPERM` — are one table with the report each must produce, so the delivered case and the two refusals cannot drift apart or be edited separately. The invalid-pid checks moved into the same shape rather than staying beside it, and their proof got stronger: the guard records every write it is asked about, so "nothing was signalled" is now an empty list rather than a spy call count, which is a claim about what the seam did rather than about how the spy was wired. The comment clause claiming only one of the three answers was tested is gone, since it was written before the third arrived. Mutations, one at a time: making the catch in `signalProcessGroupBestEffort` rethrow `EPERM` reddens the table test and nothing else; replacing its pid refusal with a NaN check reddens the invalid-pid test through that empty list. Running the second is safe precisely because the guard answers a negative pid itself and never forwards one to the kernel.
1 parent 725a225 commit 0089fb1

1 file changed

Lines changed: 28 additions & 41 deletions

File tree

‎packages/host-kit/src/internal/exec-kill-settle.test.ts‎

Lines changed: 28 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import assert from 'node:assert/strict';
22
import fs from 'node:fs';
33
import path from 'node:path';
4-
import { test, vi } from 'vitest';
4+
import { test } from 'vitest';
55
import {
66
isCommandTimeoutError,
77
runCmd,
@@ -199,53 +199,40 @@ function guardGroupWrites(answer: GroupWriteAnswer = 'delivered'): {
199199
return { writes, restore: () => (process.kill = original) };
200200
}
201201

202-
test('group signaling addresses the negative pid and reports delivery', () => {
203-
const calls: GroupWrite[] = [];
204-
const killSpy = vi.spyOn(process, 'kill').mockImplementation((pid, signal) => {
205-
calls.push({ pid: Number(pid), signal: signal ?? '' });
206-
return true;
207-
});
208-
209-
try {
210-
assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), true);
211-
assert.deepEqual(calls, [{ pid: -101, signal: 'SIGKILL' }]);
212-
} finally {
213-
killSpy.mockRestore();
202+
// One seam, one double. Every group write in this file — the direct calls below included — is answered
203+
// by `guardGroupWrites`, so a hand-written spy beside it would be a second answer to the same question,
204+
// and the two are free to drift from each other.
205+
test('the group signal seam answers each way process.kill answers a negative pid', () => {
206+
// `true` once the kernel accepted the write; `ESRCH` when no member is left and `EPERM` when a
207+
// member belongs to another user. The two throws are the same answer to this seam — nothing was
208+
// reached, so the caller must not keep waiting on a pipe holder it just asked to be killed.
209+
const cases = [
210+
['delivered', true],
211+
['no-such-process', false],
212+
['not-permitted', false],
213+
] as const;
214+
for (const [answer, reported] of cases) {
215+
const groupWrites = guardGroupWrites(answer);
216+
try {
217+
assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), reported, answer);
218+
assert.deepEqual(groupWrites.writes, [{ pid: -101, signal: 'SIGKILL' }], answer);
219+
} finally {
220+
groupWrites.restore();
221+
}
214222
}
215223
});
216224

217-
test('group signaling reports a vanished group and never signals an invalid pid', () => {
218-
const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => {
219-
const error = new Error('not found') as NodeJS.ErrnoException;
220-
error.code = 'ESRCH';
221-
throw error;
222-
});
223-
225+
test('an invalid pid is refused before anything is signalled', () => {
226+
// A zero or negative pid would address this worker's own group, or every process the user owns. The
227+
// guard records every write it is asked about, so an empty list is the proof none was attempted.
228+
const groupWrites = guardGroupWrites();
224229
try {
225-
assert.equal(signalProcessGroupBestEffort(101, 'SIGTERM'), false);
226230
assert.equal(signalProcessGroupBestEffort(0, 'SIGTERM'), false);
227231
assert.equal(signalProcessGroupBestEffort(-1, 'SIGTERM'), false);
228-
// A zero or negative pid would address the caller's own group, or every
229-
// process the user owns, so it must not reach process.kill at all.
230-
assert.equal(killSpy.mock.calls.length, 1);
232+
assert.equal(signalProcessGroupBestEffort(1.5, 'SIGTERM'), false);
233+
assert.deepEqual(groupWrites.writes, []);
231234
} finally {
232-
killSpy.mockRestore();
233-
}
234-
});
235-
236-
test('a group that is gone and a group that is not ours to signal both report nothing reached', () => {
237-
// `process.kill` answers a negative pid in exactly three ways: `true` once the kernel accepted the
238-
// write, `ESRCH` when no member is left, and `EPERM` when a member belongs to another user. The
239-
// second and third are the same answer to this seam — nothing was reached, so the caller must not
240-
// keep waiting on a pipe holder it just asked to be killed — and only the first of them was tested.
241-
for (const answer of ['no-such-process', 'not-permitted'] as const) {
242-
const groupWrites = guardGroupWrites(answer);
243-
try {
244-
assert.equal(signalProcessGroupBestEffort(101, 'SIGKILL'), false);
245-
assert.deepEqual(groupWrites.writes, [{ pid: -101, signal: 'SIGKILL' }]);
246-
} finally {
247-
groupWrites.restore();
248-
}
235+
groupWrites.restore();
249236
}
250237
});
251238

0 commit comments

Comments
 (0)