fix: snapshot failures (#15)

* fix: snapshots not properly loading between workers Signed-off-by: Chapman Pendery <[email protected]> * fix: add snapshot removal to summary of base reporter Signed-off-by: Chapman Pendery <[email protected]> * fix: flaky tests should still have a result icon Signed-off-by: Chapman Pendery <[email protected]> --------- Signed-off-by: Chapman Pendery <[email protected]>
microsoft · Mar 6, 2024 · 1292070 · 1292070
1 parent f759fcc
commit 1292070
Show file tree

Hide file tree

Showing 7 changed files with 111 additions and 33 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -41,6 +41,7 @@
     "glob": "^10.3.10",
     "jest-diff": "^29.7.0",
     "pretty-ms": "^8.0.0",
+    "proper-lockfile": "^4.1.2",
     "which": "^4.0.0",
     "workerpool": "^9.1.0",
     "xterm-headless": "^5.3.0"
@@ -51,6 +52,7 @@
     "@microsoft/tui-test": "file:.",
     "@tsconfig/node18": "^18.2.2",
     "@types/color-convert": "^2.0.3",
+    "@types/proper-lockfile": "^4.1.4",
     "@types/uuid": "^9.0.7",
     "@types/which": "^3.0.3",
     "@typescript-eslint/eslint-plugin": "^6.7.4",

diff --git a/src/reporter/base.ts b/src/reporter/base.ts
@@ -22,9 +22,15 @@ type TestSummary = {
     written: number;
     updated: number;
     obsolete: number;
+    removed: number;
   };
 };
 
+export type StaleSnapshotSummary = {
+  obsolete: number;
+  removed: number;
+};
+
 export class BaseReporter {
   protected currentTest: number;
   protected isTTY: boolean;
@@ -72,8 +78,8 @@ export class BaseReporter {
   endTest(test: TestCase, result: TestResult): void {
     if (!this.isTTY) this.currentTest += 1;
   }
-  end(rootSuite: Suite, obsoleteSnapshots: number): number {
-    const summary = this._generateSummary(rootSuite, obsoleteSnapshots);
+  end(rootSuite: Suite, staleSnapshotSummary: StaleSnapshotSummary): number {
+    const summary = this._generateSummary(rootSuite, staleSnapshotSummary);
 
     this._printFailures(summary);
     this._printSummary(summary);
@@ -84,7 +90,7 @@ export class BaseReporter {
 
   private _generateSummary(
     rootSuite: Suite,
-    obsoleteSnapshots: number
+    staleSnapshotSummary: StaleSnapshotSummary
   ): TestSummary {
     let didNotRun = 0;
     let skipped = 0;
@@ -96,7 +102,7 @@ export class BaseReporter {
       updated: 0,
       failed: 0,
       passed: 0,
-      obsolete: obsoleteSnapshots,
+      ...staleSnapshotSummary,
     };
 
     rootSuite.allTests().forEach((test) => {
@@ -198,13 +204,18 @@ export class BaseReporter {
     if (snapshots.obsolete > 0) {
       snapshotTokens.push(chalk.yellow(`${snapshots.obsolete} obsolete`));
     }
+    if (snapshots.removed > 0) {
+      snapshotTokens.push(chalk.green(`${snapshots.removed} removed`));
+    }
 
     const snapshotTotal =
       snapshots.passed +
       snapshots.failed +
       snapshots.written +
       snapshots.updated +
+      snapshots.removed +
       snapshots.obsolete;
+
     const snapshotPostfix =
       snapshots.failed > 0 || snapshots.obsolete > 0
         ? chalk.dim(

diff --git a/src/reporter/list.ts b/src/reporter/list.ts
@@ -7,7 +7,7 @@ import chalk from "chalk";
 import { Shell } from "../terminal/shell.js";
 import { fitToWidth, ansi } from "./utils.js";
 import { TestCase, TestResult, TestStatus } from "../test/testcase.js";
-import { BaseReporter } from "./base.js";
+import { BaseReporter, StaleSnapshotSummary } from "./base.js";
 import { Suite } from "../test/suite.js";
 
 export class ListReporter extends BaseReporter {
@@ -48,13 +48,17 @@ export class ListReporter extends BaseReporter {
     this._updateOrAppendLine(row, line, prefix);
   }
 
-  override end(rootSuite: Suite, obsoleteSnapshots: number): number {
-    return super.end(rootSuite, obsoleteSnapshots);
+  override end(
+    rootSuite: Suite,
+    staleSnapshotSummary: StaleSnapshotSummary
+  ): number {
+    return super.end(rootSuite, staleSnapshotSummary);
   }
 
   private _resultIcon(status: TestStatus): string {
     const color = this._resultColor(status);
     switch (status) {
+      case "flaky":
       case "expected":
         return color("✔");
       case "unexpected":

diff --git a/src/runner/runner.ts b/src/runner/runner.ts
@@ -239,8 +239,11 @@ export const run = async (options: ExecutionOptions) => {
   } catch {
     /* empty */
   }
-  const obsoleteSnapshots = await cleanSnapshots(allTests, options);
-  const failures = reporter.end(rootSuite, obsoleteSnapshots);
+  const staleSnapshots = await cleanSnapshots(allTests, options);
+  const failures = reporter.end(rootSuite, {
+    obsolete: options.updateSnapshot ? 0 : staleSnapshots,
+    removed: options.updateSnapshot ? staleSnapshots : 0,
+  });
 
   process.exit(failures);
 };
diff --git a/src/runner/worker.ts b/src/runner/worker.ts
@@ -55,14 +55,17 @@ const runTest = async (
 
   const allTests = Object.values(globalThis.tests);
   const testPath = test.filePath();
+  const testSignature = test.titlePath().slice(1).join(" › ");
   const signatureIdenticalTests = allTests.filter(
-    (t) => t.filePath() === testPath && t.title === test.title
+    (t) =>
+      t.filePath() === testPath &&
+      t.titlePath().slice(1).join(" › ") === testSignature
   );
   const signatureIdx = signatureIdenticalTests.findIndex(
     (t) => t.id == test.id
   );
   const currentConcurrentTestName = () =>
-    `${test.titlePath().slice(1).join(" › ")} | ${signatureIdx + 1}`;
+    `${testSignature} | ${signatureIdx + 1}`;
 
   expect.setState({
     ...expect.getState(),

diff --git a/src/test/matchers/toMatchSnapshot.ts b/src/test/matchers/toMatchSnapshot.ts
@@ -9,6 +9,7 @@ import fs from "node:fs";
 import fsAsync from "node:fs/promises";
 import module from "node:module";
 import workpool from "workerpool";
+import lockfile from "proper-lockfile";
 
 import { Terminal } from "../../terminal/term.js";
 
@@ -55,11 +56,21 @@ const updateSnapshot = async (
     await fsAsync.mkdir(path.dirname(snapPath), { recursive: true });
   }
 
-  const fh = await fsAsync.open(snapPath, "w+");
+  const unlock = await lockfile.lock(snapPath, {
+    stale: 5_000,
+    retries: {
+      retries: 5,
+      minTimeout: 50,
+      maxTimeout: 1_000,
+      randomize: true,
+    },
+  });
+
+  delete require.cache[require.resolve(snapPath)];
   const snapshots = require(snapPath);
   snapshots[testName] = snapshot;
-
-  await fh.writeFile(
+  await fsAsync.writeFile(
+    snapPath,
     "// TUI Test Snapshot v1\n\n" +
       Object.keys(snapshots)
         .sort()
@@ -69,7 +80,7 @@ const updateSnapshot = async (
         )
         .join("")
   );
-  await fh.close();
+  await unlock();
 };
 
 export const cleanSnapshot = async (
@@ -139,36 +150,40 @@ export async function toMatchSnapshot(
     globalThis.__expectState.updateSnapshot && snapshotsDifferent;
   const snapshotEmpty = existingSnapshot == null;
 
-  if (!workpool.isMainThread) {
-    const snapshotResult: SnapshotStatus = snapshotEmpty
-      ? "written"
-      : snapshotShouldUpdate
-        ? "updated"
-        : snapshotsDifferent
-          ? "failed"
-          : "passed";
-    workpool.workerEmit({
-      snapshotResult,
-      snapshotName: snapshotPostfixTestName,
-    });
-  }
+  const emitResult = () => {
+    if (!workpool.isMainThread) {
+      const snapshotResult: SnapshotStatus = snapshotEmpty
+        ? "written"
+        : snapshotShouldUpdate
+          ? "updated"
+          : snapshotsDifferent
+            ? "failed"
+            : "passed";
+      workpool.workerEmit({
+        snapshotResult,
+        snapshotName: snapshotPostfixTestName,
+      });
+    }
+  };
 
   if (snapshotEmpty || snapshotShouldUpdate) {
     await updateSnapshot(
       this.testPath ?? "",
       snapshotPostfixTestName,
       newSnapshot
     );
-    return Promise.resolve({
+    emitResult();
+    return {
       pass: true,
       message: () => "",
-    });
+    };
+  } else {
+    emitResult();
   }
-
-  return Promise.resolve({
+  return {
     pass: !snapshotsDifferent,
     message: !snapshotsDifferent
       ? () => ""
       : () => diffStringsUnified(existingSnapshot, newSnapshot ?? ""),
-  });
+  };
 }