Fix flaky tests and add typechecking

.github/workflows/ci.yml

@@ -19,6 +19,7 @@ jobs:
           path: appmap-node-${{ env.SHORT_SHA }}.tgz
       - run: yarn pkg-pr-new publish --commentWithDev --yarn
       - run: yarn lint
+      - run: yarn typecheck
 
   test:
     needs: build-and-lint

package.json

@@ -45,6 +45,7 @@
   "scripts": {
     "prepack": "tsc -p tsconfig.build.json",
     "lint": "eslint src test",
+    "typecheck": "tsc --noEmit",
     "test": "jest",
     "watch": "tsc --watch -p tsconfig.build.json"
   },

src/__tests__/config.test.ts

@@ -154,7 +154,9 @@ describe(Config, () => {
   let dir: string;
   beforeEach(() => {
     chdir((dir = tmp.dirSync().name));
-    jest.replaceProperty(process, "env", {});
+    // The cast is needed because next's types (pulled in through the test apps)
+    // augment ProcessEnv to make NODE_ENV required.
+    jest.replaceProperty(process, "env", {} as NodeJS.ProcessEnv);
   });
 
   const origCwd = cwd();

src/__tests__/helpers.ts

@@ -8,7 +8,6 @@ export function createTestFn(name: string, ...args: string[]): FunctionInfo {
       generator: false,
       id: identifier(name),
       params: args.map(identifier),
-      type: "FunctionDeclaration",
     },
     { path: "test.js", lineno: 42 },
   );

src/__tests__/jsxInstrument.test.ts

@@ -19,7 +19,8 @@ function isInstrumented(program: ReturnType<typeof transformJsx>): boolean {
   return program.body.some(
     (n) =>
       n.type === "VariableDeclaration" &&
-      n.declarations?.[0]?.id?.name === "__appmapFunctionRegistry",
+      n.declarations[0]?.id.type === "Identifier" &&
+      n.declarations[0].id.name === "__appmapFunctionRegistry",
   );
 }
 

src/__tests__/parameter.test.ts

@@ -1,4 +1,4 @@
-import AppMap from "../AppMap";
+import type * as AppMap from "../AppMap";
 import { objectId, optParameter, parameter } from "../parameter";
 
 class Klass {

src/hooks/next.ts

@@ -20,6 +20,11 @@ interface NextConfig {
   webpack?: (config: WebpackConfiguration, context: WebpackContext) => WebpackConfiguration;
 }
 
+// The third argument of next's loadConfig(phase, dir, opts).
+interface LoadConfigOptions {
+  rawConfig?: boolean;
+}
+
 import { call_, identifier, literal, member, ret } from "../generate";
 import { warn } from "../message";
 
@@ -47,16 +52,36 @@ export function transform(program: ESTree.Program): ESTree.Program {
       };
       const thisFile = call_(identifier("require"), literal(__filename));
       const injectConfig = member(thisFile, "injectConfig");
-      fun.body.body = [ret(call_(injectConfig, orig))];
+      // Forward the arguments rather than the declared parameters, so that we
+      // don't depend on how a given version of next happens to name them.
+      fun.body.body = [ret(call_(injectConfig, orig, identifier("arguments")))];
     },
   });
   return program;
 }
 
 const jsExtensions = ["tsx", "ts", "js", "cjs", "mjs", "jsx"];
 
-export async function injectConfig(loadConfig: () => Promise<NextConfig>): Promise<NextConfig> {
+export async function injectConfig(
+  loadConfig: () => Promise<NextConfig>,
+  args?: IArguments,
+): Promise<NextConfig> {
   const result = await loadConfig();
+
+  // With rawConfig next asks for the config module's exports rather than the
+  // resolved configuration, to validate what the user configured. That object
+  // is a module namespace and hence not extensible, and injecting a webpack
+  // config into it would in any case make next believe the user has one.
+  const opts = args?.[2] as LoadConfigOptions | undefined;
+  if (opts?.rawConfig) return result;
+
+  // Any other config we can't extend is unexpected. Don't throw, the caller
+  // handles it poorly, but do say so: nothing would be instrumented otherwise.
+  if (!Object.isExtensible(result)) {
+    warn("The Next.js configuration cannot be modified, AppMap will not be injected into it.");
+    return result;
+  }
+
   const loaderPath = resolve(__dirname, "../webpack.js");
 
   // Webpack loader injection (Next.js with --webpack or older Next.js)

test/helpers.ts

@@ -50,6 +50,23 @@ export function spawnAppmapNodeWithOptions(
   return result;
 }
 
+/**
+ * Stop handling a spawned process' output, removing every stdout and stderr
+ * data listener: the logging ones installed by spawnAppmapNode as well as any
+ * the caller has added itself. The process is expected to be gone by now, so
+ * there should be nothing left to listen for.
+ *
+ * Call this once a spawned process is no longer needed: on Windows appmap-node
+ * spawns the target with shell: true, so the actual process is a grandchild
+ * which keeps the stdio pipes open even after the shell has exited. Anything it
+ * writes on the way out (eg. terminal escapes) would otherwise be logged after
+ * the test has finished, which makes Jest fail the whole run.
+ */
+export function detachOutput(child: ChildProcessWithoutNullStreams) {
+  child.stdout.removeAllListeners("data");
+  child.stderr.removeAllListeners("data");
+}
+
 let target = fwdSlashPath(cwd());
 
 export function testDir(path: string) {

test/httpServer.test.ts

@@ -3,6 +3,7 @@ import { setTimeout as sleep } from "node:timers/promises";
 
 import {
   SpawnAppmapNodeOptions,
+  detachOutput,
   fixAppmap,
   integrationTest,
   readAppmaps,
@@ -141,7 +142,8 @@ async function makeRequest(
   return Buffer.concat(chunks).toString();
 }
 
-function killServer(server: ChildProcessWithoutNullStreams) {
+async function killServer(server: ChildProcessWithoutNullStreams) {
   server.kill("SIGINT");
-  return new Promise<void>((r) => server.on("close", () => r()));
+  await new Promise<void>((r) => server.on("close", () => r()));
+  detachOutput(server);
 }

test/next.test.ts

@@ -1,73 +1,4 @@
-import { IncomingMessage, request } from "node:http";
-import { resolve } from "node:path";
+import { integrationTest } from "./helpers";
+import testNextApp from "./nextApp";
 
-import { getFreePort, integrationTest, readAppmaps, spawnAppmapNode } from "./helpers";
-
-const nextBin = require.resolve("next/dist/bin/next", {
-  paths: [resolve(__dirname, "next")],
-});
-
-async function spawnNextJsApp(port: number) {
-  // On Windows, we give "node" argument explicitly because next is a js file with
-  // shebang (#!/usr/bin/env node) which does not work on Windows.
-  const app =
-    process.platform == "win32"
-      ? spawnAppmapNode("node", nextBin, "dev", "-p", String(port))
-      : spawnAppmapNode(nextBin, "dev", "-p", String(port));
-
-  await new Promise<void>((r) => {
-    const onData = (chunk: Buffer) => {
-      console.log("CHUNK", chunk.toString());
-      if (chunk.toString().includes("Ready")) {
-        app.stdout.removeListener("data", onData);
-        r();
-      }
-    };
-    app.stdout.on("data", onData);
-  });
-  return app;
-}
-
-integrationTest(
-  "mapping a Next.js appmap",
-  async () => {
-    const port = await getFreePort();
-    const app = await spawnNextJsApp(port);
-    const response = await makeRequest(port, "/hello");
-    console.log("Response", response);
-    const pid = parseInt((JSON.parse(response) as unknown as { pid: string }).pid);
-
-    await makeRequest(port, "/about");
-
-    app.kill("SIGINT");
-    await new Promise((r) => app.once("exit", r));
-    const appMaps = readAppmaps();
-    // Delete response body captures because they will be different in every run
-    Object.values(appMaps).forEach(
-      (a) =>
-        a.events?.forEach((e) => {
-          if ("http_server_response" in e) delete e.http_server_response.return_value;
-          if ("http_client_response" in e) delete e.http_client_response.return_value;
-        }),
-    );
-
-    expect(appMaps).toMatchSnapshot();
-
-    // We need to kill the next process explicitly on Windows
-    // because it's spawn-ed with "shell: true" and app is the shell process.
-    if (process.platform == "win32") process.kill(pid, "SIGINT");
-  },
-  20000,
-);
-
-async function makeRequest(port: number, path: string, method = "GET") {
-  const url = new URL(path, `http://localhost:${port}`);
-  const response = new Promise<IncomingMessage>((resolve, reject) => {
-    const req = request(url, { method }, resolve).once("error", reject);
-    req.end();
-  });
-
-  const chunks: Buffer[] = [];
-  for await (const chunk of await response) chunks.push(chunk as Buffer);
-  return Buffer.concat(chunks).toString();
-}
+integrationTest("mapping a Next.js appmap", testNextApp, 60000);

test/next16.test.ts

@@ -1,81 +1,12 @@
-import { IncomingMessage, request } from "node:http";
-import { resolve } from "node:path";
-
-import { getFreePort, integrationTest, readAppmaps, spawnAppmapNode } from "./helpers";
-
-const nextBin = require.resolve("next/dist/bin/next", {
-  paths: [resolve(__dirname, "next16")],
-});
-
-async function spawnNextJsApp(port: number) {
-  // On Windows, we give "node" argument explicitly because next is a js file with
-  // shebang (#!/usr/bin/env node) which does not work on Windows.
-  // Turbopack (the default bundler in Next.js 16) respects webpack loaders configured
-  // via next.config turbopack.rules, which we inject in src/hooks/next.ts.
-  const app =
-    process.platform == "win32"
-      ? spawnAppmapNode("node", nextBin, "dev", "-p", String(port))
-      : spawnAppmapNode(nextBin, "dev", "-p", String(port));
-
-  await new Promise<void>((r) => {
-    const onData = (chunk: Buffer) => {
-      console.log("CHUNK", chunk.toString());
-      if (chunk.toString().includes("Ready")) {
-        app.stdout.removeListener("data", onData);
-        r();
-      }
-    };
-    app.stdout.on("data", onData);
-  });
-  return app;
-}
+import { integrationTest } from "./helpers";
+import testNextApp from "./nextApp";
 
 // Next.js 16 requires Node.js >= 20.9.0
 const nodeSupported = (() => {
   const [major, minor] = process.versions.node.split(".").map(Number);
   return major > 20 || (major === 20 && minor >= 9);
 })();
 
-integrationTest.if(nodeSupported)(
-  "mapping a Next.js 16 appmap",
-  async () => {
-    const port = await getFreePort();
-    const app = await spawnNextJsApp(port);
-    const response = await makeRequest(port, "/hello");
-    console.log("Response", response);
-    const pid = parseInt((JSON.parse(response) as unknown as { pid: string }).pid);
-
-    await makeRequest(port, "/about");
-
-    app.kill("SIGINT");
-    await new Promise((r) => app.once("exit", r));
-    const appMaps = readAppmaps();
-    // Delete response body captures because they will be different in every run
-    Object.values(appMaps).forEach(
-      (a) =>
-        a.events?.forEach((e) => {
-          if ("http_server_response" in e) delete e.http_server_response.return_value;
-          if ("http_client_response" in e) delete e.http_client_response.return_value;
-        }),
-    );
-
-    expect(appMaps).toMatchSnapshot();
-
-    // We need to kill the next process explicitly on Windows
-    // because it's spawn-ed with "shell: true" and app is the shell process.
-    if (process.platform == "win32") process.kill(pid, "SIGINT");
-  },
-  60000,
-);
-
-async function makeRequest(port: number, path: string, method = "GET") {
-  const url = new URL(path, `http://localhost:${port}`);
-  const response = new Promise<IncomingMessage>((resolve, reject) => {
-    const req = request(url, { method }, resolve).once("error", reject);
-    req.end();
-  });
-
-  const chunks: Buffer[] = [];
-  for await (const chunk of await response) chunks.push(chunk as Buffer);
-  return Buffer.concat(chunks).toString();
-}
+// Turbopack (the default bundler in Next.js 16) respects webpack loaders configured
+// via next.config turbopack.rules, which we inject in src/hooks/next.ts.
+integrationTest.if(nodeSupported)("mapping a Next.js 16 appmap", testNextApp, 60000);