bun test: fail on a .snap entry it cannot read instead of adding a duplicate

src/runtime/test_runner/snapshot.rs

@@ -227,17 +227,6 @@
             return Ok(());
         }
 
-        // SAFETY: VM is thread-local singleton installed before any test runs; lives for the
-        // duration of the runner. Per `VirtualMachine::get` doc, callers form a short-lived borrow.
-        let vm = VirtualMachine::get().as_mut();
-        let opts = js_parser::ParserOptions::init(
-            vm.transpiler.options.jsx.clone(),
-            bun_ast::Loader::Js,
-        );
-        // Thread a per-call arena — js_parser is bump-allocated.
-        let arena = bun_alloc::Arena::new();
-        let mut temp_log = bun_ast::Log::init();
-
         // do NOT call `Jest::runner()` here — it hands out an exclusive ref to the global TestRunner,
         // and `self: &mut Snapshots` is a live borrow of that same TestRunner's `.snapshots`
         // field. Retagging the whole TestRunner would invalidate `self` under Stacked Borrows.
@@ -268,83 +257,123 @@
         // SAFETY: buf[pos] == 0 written above
         let snapshot_file_path = ZStr::from_buf(&buf[..], pos);
 
+        // `source` aliases `file_buf` (lifetime erased); `load_entries` writes to `values` only.
         let source = bun_ast::Source::init_path_string(
             snapshot_file_path.as_bytes(),
             self.file_buf.as_slice(),
         );
 
-        let parser = js_parser::Parser::init(
-            opts,
-            &mut temp_log,
-            &source,
-            &vm.transpiler.options.define,
-            &arena,
-        )?;
+        let mut temp_log = bun_ast::Log::init();
+        let result = Self::load_entries(&mut self.values, &source, &mut temp_log);
+        if temp_log.errors > 0 {
+            let _ = temp_log.print(std::ptr::from_mut::<bun_core::io::Writer>(
+                bun_output::error_writer(),
+            ));
+            bun_output::flush();
+        }
+        result
+    }
+
+    /// Loads the entries of a `.snap` file into `values`. Any other statement is an error.
+    fn load_entries(
+        values: &mut HashMap<u64, Box<[u8]>>,
+        source: &bun_ast::Source,
+        temp_log: &mut bun_ast::Log,
+    ) -> Result<(), Error> {
+        // SAFETY: VM is thread-local singleton installed before any test runs; lives for the
+        // duration of the runner. Per `VirtualMachine::get` doc, callers form a short-lived borrow.
+        let vm = VirtualMachine::get().as_mut();
+        let opts = js_parser::ParserOptions::init(
+            vm.transpiler.options.jsx.clone(),
+            bun_ast::Loader::Js,
+        );
+        // Thread a per-call arena — js_parser is bump-allocated.
+        let arena = bun_alloc::Arena::new();
 
-        let parse_result = parser.parse()?;
-        let mut ast = match parse_result {
+        let parser =
+            js_parser::Parser::init(opts, temp_log, source, &vm.transpiler.options.define, &arena)
+                .map_err(|_| crate::Error::ParseError)?;
+        let mut ast = match parser.parse().map_err(|_| crate::Error::ParseError)? {
             bun_js_parser::Result::Ast(ast) => ast,
             _ => return Err(crate::Error::ParseError),
         };
-
-        if ast.exports_ref.is_empty() {
-            return Ok(());
-        }
         let exports_ref = ast.exports_ref;
 
-        // TODO: when common js transform changes, keep this updated or add flag to support this version
-
         for part in ast.parts.as_mut_slice() {
             // `part.stmts` is an arena-owned `StoreSlice<Stmt>`; arena outlives this
             // loop and `ast` is owned here, so unique access is upheld.
             for stmt in part.stmts.slice_mut() {
-                match &mut stmt.data {
-                    bun_ast::StmtData::SExpr(expr) => {
-                        if let bun_ast::ExprData::EBinary(e_binary) = &mut expr.value.data {
-                            // deref `StoreRef` once to a plain `&mut E::Binary`
-                            // so the borrow checker can see `.left`/`.right` as disjoint
-                            // field projections (custom `DerefMut` blocks split-borrows
-                            // otherwise).
-                            let e_binary = &mut **e_binary;
-                            if e_binary.op == bun_ast::Op::Code::BinAssign {
-                                let (left, right) = (&mut e_binary.left, &mut e_binary.right);
-                                if let bun_ast::ExprData::EIndex(e_index) = &mut left.data {
-                                    // split-borrow `index`/`target` so we can take
-                                    // `&mut` on `index` (EString::slice needs &mut) while reading
-                                    // `target` immutably.
-                                    let target_is_exports = matches!(
-                                        &e_index.target.data,
-                                        bun_ast::ExprData::EIdentifier(target) if target.ref_.eql(exports_ref)
-                                    );
-                                    if target_is_exports {
-                                        if let bun_ast::ExprData::EString(index) =
-                                            &mut e_index.index.data
-                                        {
-                                            if let bun_ast::ExprData::EString(value_string) =
-                                                &mut right.data
-                                            {
-                                                let key = index.slice(&arena);
-                                                let value = value_string.slice(&arena);
-                                                let value_clone: Box<[u8]> =
-                                                    Box::<[u8]>::from(value);
-                                                let name_hash: u64 = hash(key);
-                                                self.values.insert(name_hash, value_clone);
-                                            }
-                                        }
-                                    }
-                                }
-                            }
-                        }
-                    }
-                    _ => {}
+                if matches!(
+                    stmt.data,
+                    bun_ast::StmtData::SComment(_)
+                        | bun_ast::StmtData::SDirective(_)
+                        | bun_ast::StmtData::SEmpty(_)
+                ) {
+                    continue;
                 }
+                let Some((name, value)) = Self::exports_assignment(&mut stmt.data, exports_ref)
+                else {
+                    temp_log.add_error(
+                        Some(source),
+                        stmt.loc,
+                        "Expected a snapshot entry: exports[`name`] = `value`;",
+                    );
+                    continue;
+                };
+                let bun_ast::ExprData::EString(name_string) = &mut name.data else {
+                    temp_log.add_error(
+                        Some(source),
+                        name.loc,
+                        "The snapshot name must be a template literal without substitutions",
+                    );
+                    continue;
+                };
+                let bun_ast::ExprData::EString(value_string) = &mut value.data else {
+                    temp_log.add_error(
+                        Some(source),
+                        value.loc,
+                        "The snapshot value must be a template literal without substitutions",
+                    );
+                    continue;
+                };
+                let name_hash: u64 = hash(name_string.slice(&arena));
+                values.insert(name_hash, Box::<[u8]>::from(value_string.slice(&arena)));
             }
         }
 
-        let _ = &mut ast;
+        if temp_log.errors > 0 {
+            return Err(crate::Error::ParseError);
+        }
         Ok(())
     }
 
+    /// The `name` and `value` expressions of a `exports[name] = value;` statement.
+    fn exports_assignment(
+        stmt: &mut bun_ast::StmtData,
+        exports_ref: bun_ast::Ref,
+    ) -> Option<(&mut bun_ast::Expr, &mut bun_ast::Expr)> {
+        let bun_ast::StmtData::SExpr(s_expr) = stmt else {
+            return None;
+        };
+        let bun_ast::ExprData::EBinary(e_binary) = &mut s_expr.value.data else {
+            return None;
+        };
+        // Plain `&mut` structs, so `.left`/`.right` and `.target`/`.index` split-borrow.
+        let e_binary = &mut **e_binary;
+        if e_binary.op != bun_ast::Op::Code::BinAssign {
+            return None;
+        }
+        let bun_ast::ExprData::EIndex(e_index) = &mut e_binary.left.data else {
+            return None;
+        };
+        let e_index = &mut **e_index;
+        match &e_index.target.data {
+            bun_ast::ExprData::EIdentifier(target) if target.ref_.eql(exports_ref) => {}
+            _ => return None,
+        }
+        Some((&mut e_index.index, &mut e_binary.right))
+    }
+
     pub(crate) fn write_snapshot_file(&mut self) -> Result<(), Error> {
         if let Some(file) = self._current_file.take() {
             file.file
@@ -918,7 +947,12 @@
                 }
             }
 
-            self.parse_file(&file)?;
+            if let Err(err) = self.parse_file(&file) {
+                // Or the next `.snap` file is appended to this one and parsed with it.
+                self.file_buf = Vec::new();
+                self.values.clear();
+                return Err(err);
+            }
             self._current_file = Some(file);
         }
 

test/js/bun/test/snapshot-tests/new-snapshot.test.ts

@@ -1,6 +1,7 @@
-import { expect, test } from "bun:test";
+import { describe, expect, test } from "bun:test";
 import fs from "fs";
-import { bunEnv, bunExe, tmpdirSync } from "harness";
+import { bunEnv, bunExe, tempDir, tmpdirSync } from "harness";
+import { join } from "path";
 
 test("it will create a snapshot file and directory if they don't exist", () => {
   const tempDir = tmpdirSync();
@@ -28,3 +29,133 @@ test("it will create a snapshot file and directory if they don't exist", () => {
   expect(exitCode2).toBe(0);
   expect(fs.existsSync(tempDir + "/__snapshots__/new-snapshot.test.ts.snap")).toBe(true);
 });
+
+const HEADER = "// Bun Snapshot v1, https://bun.sh/docs/test/snapshots\n";
+const A_TEST = `
+  import { test, expect } from "bun:test";
+  test("a", () => expect("hello").toMatchSnapshot());
+`;
+const B_TEST = `
+  import { test, expect } from "bun:test";
+  test("b", () => expect("world").toMatchSnapshot());
+`;
+const VALUE_ERROR = "The snapshot value must be a template literal without substitutions";
+const NAME_ERROR = "The snapshot name must be a template literal without substitutions";
+
+// CI=false: only a run that may add snapshots can append to the file.
+async function runBunTest(dir: string, ...args: string[]) {
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "test", ...args],
+    cwd: dir,
+    env: { ...bunEnv, CI: "false" },
+    stdin: "ignore",
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  return { stdout, stderr, exitCode };
+}
+
+describe("a .snap entry bun test cannot read", () => {
+  // Each entry below is line 3 of its .snap file. The reader used to skip it, so the snapshot
+  // counted as new: the test passed, and a second `a 1` entry was appended under the bad one.
+  test.concurrent.each([
+    ["a substitution in the value", "exports[`a 1`] = `${x}`;", "3:18", VALUE_ERROR],
+    ["a substitution in the name", 'exports[`a ${1}`] = `"hello"`;', "3:9", NAME_ERROR],
+    ["a regular expression as the value", "exports[`a 1`] = /hello/;", "3:18", VALUE_ERROR],
+    ["a number as the value", "exports[`a 1`] = 1;", "3:18", VALUE_ERROR],
+    ["a concatenation as the value", 'exports[`a 1`] = `"hel"` + `"lo"`;', "3:18", VALUE_ERROR],
+    [
+      "a statement that is not an exports[...] assignment",
+      'module.exports[`a 1`] = `"hello"`;',
+      "3:1",
+      "Expected a snapshot entry: exports[`name`] = `value`;",
+    ],
+  ])("%s fails the test and leaves the file alone", async (_, entry, position, message) => {
+    const snap = `${HEADER}\n${entry}\n`;
+    using dir = tempDir("snap-bad-entry", { "a.test.ts": A_TEST, "__snapshots__/a.test.ts.snap": snap });
+    const snapPath = join(String(dir), "__snapshots__", "a.test.ts.snap");
+
+    const { stderr, exitCode } = await runBunTest(String(dir));
+    expect(stderr).toContain(`error: ${message}\n    at ${snapPath}:${position}\n`);
+    expect(stderr).toContain("Failed to parse snapshot file");
+    expect(stderr).toContain(" 1 fail");
+    expect(stderr).not.toContain("added");
+    expect(exitCode).toBe(1);
+    expect(fs.readFileSync(snapPath, "utf8")).toBe(snap);
+  });
+
+  test.concurrent("every bad entry of the file is reported at once", async () => {
+    const snap = `${HEADER}\nexports[\`a 1\`] = \`\${x}\`;\n\nexports[\`a \${2}\`] = \`"hello"\`;\n\nexports[\`b 1\`] = \`"ok"\`;\n`;
+    using dir = tempDir("snap-bad-entries", { "a.test.ts": A_TEST, "__snapshots__/a.test.ts.snap": snap });
+    const snapPath = join(String(dir), "__snapshots__", "a.test.ts.snap");
+
+    const { stderr, exitCode } = await runBunTest(String(dir));
+    expect(stderr).toContain(`error: ${VALUE_ERROR}\n    at ${snapPath}:3:18\n`);
+    expect(stderr).toContain(`error: ${NAME_ERROR}\n    at ${snapPath}:5:9\n`);
+    expect(stderr.split(`    at ${snapPath}:`)).toHaveLength(3);
+    expect(exitCode).toBe(1);
+    expect(fs.readFileSync(snapPath, "utf8")).toBe(snap);
+  });
+
+  test.concurrent("a file that does not parse names the line and does not break the next test file", async () => {
+    const torn = `${HEADER}\nexports[\`a 1\`] = \`"hello;\n`;
+    const intact = `${HEADER}\nexports[\`b 1\`] = \`"world"\`;\n`;
+    using dir = tempDir("snap-torn", {
+      "a.test.ts": A_TEST,
+      "b.test.ts": B_TEST,
+      "__snapshots__/a.test.ts.snap": torn,
+      "__snapshots__/b.test.ts.snap": intact,
+    });
+    const snapPath = join(String(dir), "__snapshots__", "a.test.ts.snap");
+
+    const { stderr, exitCode } = await runBunTest(String(dir));
+    expect(stderr).toContain(`\n    at ${snapPath}:3:18\n`);
+    expect(stderr).toContain("Failed to parse snapshot file");
+    expect(stderr).not.toContain("Failed to snapshot value");
+    // a.test.ts must run first. The bytes of its unreadable .snap file used to stay in the buffer
+    // that b.test.ts.snap was then read into, so b's valid snapshot failed to parse as well.
+    expect(stderr.match(/^\w+\.test\.ts:$/gm)).toEqual(["a.test.ts:", "b.test.ts:"]);
+    expect(stderr).toContain("(pass) b");
+    expect(stderr).toContain(" 1 pass\n 1 fail\n");
+    expect(exitCode).toBe(1);
+    expect(fs.readFileSync(snapPath, "utf8")).toBe(torn);
+    expect(fs.readFileSync(join(String(dir), "__snapshots__", "b.test.ts.snap"), "utf8")).toBe(intact);
+  });
+
+  test.concurrent("--update-snapshots rewrites the file", async () => {
+    using dir = tempDir("snap-bad-entry-update", {
+      "a.test.ts": A_TEST,
+      "__snapshots__/a.test.ts.snap": `${HEADER}\nexports[\`a 1\`] = \`\${x}\`;\n`,
+    });
+    const snapPath = join(String(dir), "__snapshots__", "a.test.ts.snap");
+
+    const { stderr, exitCode } = await runBunTest(String(dir), "--update-snapshots");
+    expect(stderr).not.toContain("Failed to parse snapshot file");
+    expect(stderr).toContain("snapshots: +1 added");
+    expect(exitCode).toBe(0);
+    expect(fs.readFileSync(snapPath, "utf8")).toBe(`${HEADER}\nexports[\`a 1\`] = \`"hello"\`;\n`);
+  });
+
+  test.concurrent("comments, directives, empty statements and escaped ${} are still read", async () => {
+    using dir = tempDir("snap-trivia", {
+      "a.test.ts": `
+        import { test, expect } from "bun:test";
+        test("a", () => expect("\${x} and \`ticks\`").toMatchSnapshot());
+      `,
+    });
+    const snapPath = join(String(dir), "__snapshots__", "a.test.ts.snap");
+    expect((await runBunTest(String(dir))).exitCode).toBe(0);
+
+    const written = fs.readFileSync(snapPath, "utf8");
+    expect(written).toContain("\\${x} and \\`ticks\\`");
+    const edited = written.replace(HEADER, `${HEADER}"use strict";\n/*! kept */\n;\n`);
+    fs.writeFileSync(snapPath, edited);
+
+    const { stderr, exitCode } = await runBunTest(String(dir));
+    expect(stderr).not.toContain("error:");
+    expect(stderr).toContain(" 1 snapshots, ");
+    expect(exitCode).toBe(0);
+    expect(fs.readFileSync(snapPath, "utf8")).toBe(edited);
+  });
+});