Reject a Harper node where a test context is expected

README.md

@@ -82,6 +82,8 @@ Edit the `HARPER_INTEGRATION_TEST_LOOPBACK_POOL_COUNT` value in the installed pl
 
 The lifecycle and utility APIs below are framework-agnostic. They manage Harper child processes and a cross-process loopback address pool. Use them in the setup/teardown hooks of whichever test framework you prefer.
 
+`startHarper`, `setupHarperWithFixture`, `killHarper` and `teardownHarper` all take the **context** — the object with a `harper` property — not the node held in `ctx.harper`. Passing the node throws a `TypeError`: the teardown pair tells you to wrap it (`teardownHarper({ harper: node })`), while the start pair tells you to pass the context the node came from, since starting from the node would publish a fresh one over it and abandon the instance already running. It used to be accepted silently, which left the node running until the runner exited. A context whose `harper` was never populated is still a no-op, so teardown after a `before` hook that threw stays safe.
+
 ### `startHarper(ctx, options?)`
 
 Allocates a loopback address from the pool, creates a temporary install directory, starts a Harper process, and waits for it to be ready. Populates `ctx.harper` with the instance details. Call in a setup/`before()` hook.

src/harperLifecycle.ts

@@ -530,13 +530,14 @@ export async function setupHarperWithFixture(
 	fixturePath: string,
 	options?: StartHarperOptions
 ): Promise<StartedHarperTestContext> {
+	assertHarperTestContext(ctx, 'setupHarperWithFixture', START_FROM_THE_CONTEXT);
 	const dataRootDirPrefix = join(
 		process.env.HARPER_INTEGRATION_TEST_INSTALL_PARENT_DIR || tmpdir(),
 		'harper-integration-test-'
 	);
 	const dataRootDir = await mkdtemp(dataRootDirPrefix);
 	await cp(fixturePath, join(dataRootDir, 'components', basename(fixturePath)), { recursive: true, dereference: true });
-	ctx.harper = { dataRootDir };
+	publishHarperNode(ctx, { dataRootDir });
 	return startHarper(ctx, options);
 }
 
@@ -566,6 +567,7 @@ export async function setupHarperWithFixture(
  * ```
  */
 export async function startHarper(ctx: HarperTestContext, options?: StartHarperOptions): Promise<StartedHarperTestContext> {
+	assertHarperTestContext(ctx, 'startHarper', START_FROM_THE_CONTEXT);
 	const dataRootDirPrefix = join(
 		process.env.HARPER_INTEGRATION_TEST_INSTALL_PARENT_DIR || tmpdir(),
 		`harper-integration-test-`
@@ -629,7 +631,7 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO
 		maxMs: options?.startupMaxMs,
 	});
 
-	ctx.harper = {
+	publishHarperNode(ctx, {
 		dataRootDir,
 		admin: {
 			username: DEFAULT_ADMIN_USERNAME,
@@ -641,7 +643,7 @@ export async function startHarper(ctx: HarperTestContext, options?: StartHarperO
 		process: result.process,
 		logDir,
 		startupOutput: { stdout: result.stdout, stderr: result.stderr },
-	};
+	});
 
 	return ctx as StartedHarperTestContext;
 }
@@ -721,6 +723,50 @@ function trackHarperProcess(proc: ChildProcess): void {
 	}
 }
 
+/** Identifies the objects published as `ctx.harper`; a shallow copy of a node does not carry it. */
+const HARPER_NODE = Symbol('harperNode');
+
+/**
+ * The one place a node becomes `ctx.harper`, so every published node carries the brand.
+ * Exported for tests, not from `index.ts`.
+ */
+export function publishHarperNode(ctx: HarperTestContext, node: Partial<HarperContext>): void {
+	ctx.harper = markHarperNode(node);
+}
+
+/** Exported for tests, not from `index.ts`. */
+export function markHarperNode<T extends object>(node: T): T {
+	return Object.defineProperty(node, HARPER_NODE, { value: true, enumerable: false });
+}
+
+/**
+ * Rejects the node where the context belongs. Brand rather than field names: this runs when
+ * `ctx.harper` is falsy, the same path a failed `before` hook takes, so rejecting on a name a
+ * caller's own context might use (`hostname`, `httpURL`, `process`) would bury the real error.
+ */
+function assertHarperTestContext(ctx: unknown, fnName: string, remedy: string): void {
+	if (ctx === null || typeof ctx !== 'object' || Array.isArray(ctx)) {
+		// An array reaches the no-op the same way a node does: `[a, b].harper` is undefined.
+		const isArray = Array.isArray(ctx);
+		const received = ctx === null ? 'null' : isArray ? 'an array' : typeof ctx;
+		const advice = isArray ? ' Pass each context separately.' : '';
+		throw new TypeError(`${fnName}(ctx) requires a test context object, received ${received}.${advice}`);
+	}
+	if (HARPER_NODE in ctx) {
+		throw new TypeError(`${fnName}(ctx) expects the test context, but received the Harper node it holds. ${remedy}`);
+	}
+}
+
+/**
+ * Remedies are per-direction, not generated from the function name: telling a caller who reached a
+ * *start* function with a live node to wrap it would have them overwrite `ctx.harper` and abandon the
+ * instance already running on it.
+ */
+const WRAP_THE_NODE = (fnName: string) =>
+	`Wrap it: ${fnName}({ harper: node }) — otherwise the node keeps running until the runner exits.`;
+const START_FROM_THE_CONTEXT =
+	'Pass the context the node came from. Starting from the node itself would publish a fresh one over it and abandon the instance already running.';
+
 /**
  * Kill harper process (can be used for teardown, or killing it before a restart).
  *
@@ -734,6 +780,7 @@ function trackHarperProcess(proc: ChildProcess): void {
  *   {@link DEFAULT_TEARDOWN_GRACE_MS}.
  */
 export async function killHarper(ctx: StartedHarperTestContext, options?: { graceMs?: number }): Promise<void> {
+	assertHarperTestContext(ctx, 'killHarper', WRAP_THE_NODE('killHarper'));
 	const proc = ctx.harper?.process;
 	if (!proc) return;
 	// Already exited — nothing to do.
@@ -790,6 +837,7 @@ export async function killHarper(ctx: StartedHarperTestContext, options?: { grac
  * ```
  */
 export async function teardownHarper(ctx: StartedHarperTestContext): Promise<void> {
+	assertHarperTestContext(ctx, 'teardownHarper', WRAP_THE_NODE('teardownHarper'));
 	if (!ctx.harper) return;
 	await killHarper(ctx);
 

test/harperLifecycle.test.ts

@@ -2,12 +2,17 @@ import { test, before, after } from 'node:test';
 import { ok, strictEqual, match, rejects } from 'node:assert';
 import { spawn, type ChildProcess } from 'node:child_process';
 import { once } from 'node:events';
-import { mkdtempSync, writeFileSync, rmSync } from 'node:fs';
+import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { setTimeout as sleep } from 'node:timers/promises';
 import {
 	killHarper,
+	teardownHarper,
+	markHarperNode,
+	publishHarperNode,
+	setupHarperWithFixture,
+	startHarper,
 	runHarperCommand,
 	HarperStartupError,
 	buildHarperChildEnv,
@@ -240,6 +245,106 @@ test('killHarper returns immediately when there is no process', async () => {
 	await killHarper({} as unknown as StartedHarperTestContext);
 });
 
+test('teardownHarper rejects the node it published, and killHarper does too', async () => {
+	const node = markHarperNode({ hostname: '127.0.0.2', operationsAPIURL: 'http://127.0.0.2:9925' });
+	await rejects(
+		() => teardownHarper(node as unknown as StartedHarperTestContext),
+		(error: Error) => {
+			ok(error instanceof TypeError, `expected a TypeError, got ${error.constructor.name}`);
+			match(error.message, /expects the test context, but received the Harper node it holds/);
+			match(error.message, /teardownHarper\(\{ harper: node \}\)/);
+			return true;
+		}
+	);
+	await rejects(() => killHarper(node as unknown as StartedHarperTestContext), /killHarper\(ctx\) expects the test context/);
+});
+
+// `startHarper(ctx.harper)` type-checks; unguarded it claimed a second install and loopback address.
+test('the entry points reject the node too, before allocating anything', async () => {
+	const node = markHarperNode({ hostname: '127.0.0.2', dataRootDir: '/tmp/already-installed' });
+	// The remedy must NOT be "wrap it" here: doing that publishes a fresh node over the live one.
+	for (const start of [
+		() => startHarper(node as unknown as StartedHarperTestContext),
+		() => setupHarperWithFixture(node as unknown as StartedHarperTestContext, fixtureDir),
+	]) {
+		await rejects(start, (error: Error) => {
+			match(error.message, /expects the test context, but received the Harper node it holds/);
+			match(error.message, /Pass the context the node came from/);
+			ok(!/Wrap it:/.test(error.message), 'a start function must not advise wrapping a live node');
+			return true;
+		});
+	}
+});
+
+test('teardownHarper still tears down a correctly wrapped context', async () => {
+	const dataRootDir = mkdtempSync(join(tmpdir(), 'guard-teardown-'));
+	await teardownHarper({ harper: markHarperNode({ dataRootDir }) } as unknown as StartedHarperTestContext);
+	strictEqual(existsSync(dataRootDir), false, 'teardown should have removed the install directory');
+});
+
+test('the brand is not enumerable', () => {
+	const node = markHarperNode({ hostname: '127.0.0.2' });
+	strictEqual(Object.keys(node).length, 1);
+	strictEqual(JSON.stringify(node), '{"hostname":"127.0.0.2"}');
+});
+
+test('killHarper rejects a live node unwrapped before signaling it, and reaps it once wrapped', async () => {
+	const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)']);
+	const node = markHarperNode({ hostname: '127.0.0.2', process: child });
+	try {
+		await rejects(() => killHarper(node as unknown as StartedHarperTestContext), TypeError);
+		strictEqual(child.exitCode, null, 'the guard must reject before any signal is sent');
+
+		await killHarper({ harper: node } as unknown as StartedHarperTestContext, { graceMs: 200 });
+		ok(child.exitCode !== null || child.signalCode !== null, 'the wrapped call must reap the child');
+	} finally {
+		child.kill('SIGKILL');
+	}
+});
+
+test('an unbranded context is a no-op even when it carries node-like field names', async () => {
+	await teardownHarper({} as unknown as StartedHarperTestContext);
+	await teardownHarper({ name: 'suite-that-threw-in-before' } as unknown as StartedHarperTestContext);
+	await teardownHarper({
+		httpURL: 'http://127.0.0.2:3000',
+		operationsAPIURL: 'http://127.0.0.2:9925',
+		dataRootDir: '/tmp/a-caller-owned-path',
+		admin: { username: 'admin' },
+	} as unknown as StartedHarperTestContext);
+});
+
+// Every node reaches `ctx.harper` through publishHarperNode, so branding it there is what makes the
+// guard apply to real nodes. Exercised directly: reaching the two call sites needs a loopback slot
+// and a real install, which this suite deliberately never takes.
+test('publishHarperNode brands what it assigns', async () => {
+	const dataRootDir = mkdtempSync(join(tmpdir(), 'guard-publish-'));
+	const ctx: { harper?: Partial<{ dataRootDir: string }> } = {};
+	publishHarperNode(ctx, { dataRootDir });
+	strictEqual(ctx.harper?.dataRootDir, dataRootDir);
+	await rejects(
+		() => teardownHarper(ctx.harper as unknown as StartedHarperTestContext),
+		/received the Harper node it holds/
+	);
+	await teardownHarper(ctx as unknown as StartedHarperTestContext);
+	strictEqual(existsSync(dataRootDir), false, 'the wrapped context must still tear down');
+});
+
+test('the shape guard rejects a non-object argument', async () => {
+	await rejects(
+		() => teardownHarper(undefined as unknown as StartedHarperTestContext),
+		/requires a test context object, received undefined/
+	);
+	await rejects(
+		() => teardownHarper(null as unknown as StartedHarperTestContext),
+		/requires a test context object, received null/
+	);
+	// A suite that keeps its nodes in an array reaches the no-op this way: `[a, b].harper` is undefined.
+	await rejects(
+		() => teardownHarper([{ harper: markHarperNode({}) }] as unknown as StartedHarperTestContext),
+		/requires a test context object, received an array\. Pass each context separately\./
+	);
+});
+
 test('killHarper returns immediately for an already-exited process', async () => {
 	const child = spawn(process.execPath, ['-e', 'process.exit(0)']);
 	await once(child, 'exit');