Skip to content

feat(ext/node): support t.assert.snapshot in node:test#35476

Open
divybot wants to merge 4 commits into
denoland:mainfrom
divybot:orch/divybot-589
Open

feat(ext/node): support t.assert.snapshot in node:test#35476
divybot wants to merge 4 commits into
denoland:mainfrom
divybot:orch/divybot-589

Conversation

@divybot

@divybot divybot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #35402. node:test's test context exposed t.assert but none of the snapshot assertions, so the documented t.assert.snapshot(value) API threw TypeError: t.assert.snapshot is not a function.

This implements Node's snapshot testing in the node:test polyfill:

  • t.assert.snapshot(value, options?)
  • t.assert.fileSnapshot(value, path, options?)
  • The snapshot module export: snapshot.setResolveSnapshotPath(fn) and snapshot.setDefaultSnapshotSerializers(serializers)

The reproduction from the issue now works:

import { test, suite } from "node:test";

suite("suite of snapshot tests", () => {
  test("snapshot test", (t) => {
    t.assert.snapshot(5);
  });
});
$ deno test --update-snapshots test.test.js   # writes test.test.js.snapshot
$ deno test test.test.js                       # compares, passes

Details

  • Snapshots are stored next to the test file as <testfile>.snapshot in the same exports[`name N`] = `...`; format Node uses, so snapshot files are interchangeable between the two runtimes. The default serializer matches Node (JSON.stringify(value, null, 2)).
  • deno test --update-snapshots plays the role of Node's --test-update-snapshots. In update mode the file is rewritten from the snapshots asserted in the run (stale entries dropped); otherwise the serialized value is compared with assert.strictEqual, producing a diff on mismatch.
  • The default snapshot location reuses the test runner's existing snapshot ops (op_test_snapshot_read/write/in_update_mode) via a new nodeLayout option, so it is resolved from the trusted test origin and stays exempt from permission checks — matching Node's permission-free behavior. fileSnapshot targets a user-supplied path and is subject to normal --allow-read/--allow-write.
  • The TypeScript declarations for these APIs already shipped (from @types/node); this change provides the runtime implementation behind them.

Tests

  • New tests/specs/test/node_snapshot/: create→verify roundtrip (incl. asserting the exact on-disk file format), missing-snapshot error, and mismatch diff.
  • Existing node::node_test_* (24) and Deno-native test::snapshot (8) specs still pass (the nodeLayout op change is backward compatible).

`node:test`'s test context exposed `t.assert` but not the snapshot
assertions, so `t.assert.snapshot(value)` (and `t.assert.fileSnapshot`)
threw "is not a function" (denoland#35402).

This implements Node's snapshot testing in the `node:test` polyfill:

- `t.assert.snapshot(value, options?)` and
  `t.assert.fileSnapshot(value, path, options?)`.
- The `snapshot` module export with `setResolveSnapshotPath()` and
  `setDefaultSnapshotSerializers()`.

Snapshots are stored next to the test file as `<testfile>.snapshot` in
the same `exports[`name N`] = `...`;` format Node uses, so snapshot
files are interchangeable between the two runtimes. `deno test
--update-snapshots` takes the place of Node's `--test-update-snapshots`.

The default snapshot location reuses the test runner's snapshot ops
(via a new `nodeLayout` option) so it is resolved from the trusted test
origin and stays exempt from permission checks, matching Node's
permission-free behavior. `fileSnapshot` writes a user-supplied path and
is subject to normal read/write permissions.

Closes denoland#35402
@littledivy

Copy link
Copy Markdown
Member

@divybot merge main

# Conflicts:
#	ext/node/polyfills/testing.ts
@divybot

divybot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Merged main ✅ — resolved the overlap with the fileSnapshot work from #35478: t.assert.snapshot is layered on top, and both now share the update-mode detection and setDefaultSnapshotSerializers default (so fileSnapshot honors custom default serializers too). fileSnapshot stays on the shared assert object; only the per-test snapshot is added per-context. Specs + ./x lint/./x fmt are green locally.

@bartlomieju

Copy link
Copy Markdown
Member

Nice work — this layers cleanly on top of #35139 (reuses the same op_test_snapshot_read/write/in_update_mode ops and the --update-snapshots flag; the only Rust change is the additive, backward-compatible node_layout field). A single deno test --update-snapshots run correctly drives both the Deno-native assertSnapshot and the new t.assert.snapshot, and I confirmed the permission-exempt path is still derived from the trusted ModuleSpecifier in OpState (the JS-side originUrl only keys the cache in the default case), so a compromised origin string can't redirect a permission-free write.

A few things I'd like to discuss before merging:

File format divergence — do we want two formats?

This writes Node's <testfile>.snapshot (exports[\name N`] = `...`;), while #35139 writes Deno's snapshots/.snap. So a project that snapshots via both Deno.test+assertSnapshotandnode:test+t.assert.snapshot` ends up with two separate files in two formats that never reconcile.

I'm not sure we want to ship two on-disk formats. The Node format is desirable here for interchange with Node, but I'd like us to consciously decide whether we (a) accept the split as intentional (node:test interop wins), (b) converge both APIs on the Node format, or (c) converge on the Deno format and give up Node file interchange. Can we discuss before this lands? At minimum this should be called out explicitly in the PR description so it's a deliberate choice rather than an accident of two independent PRs.

Correctness notes:

  • Origin mismatch between the two contexts. NodeTestContext resolves the origin from ctx.#denoContext?.origin (the actual test file), but TapContext uses globalThis.Deno?.mainModule. Under deno test with the default layout this is benign (the real path comes from OpState; the origin only keys the JS cache). But under node:test's own run() streaming path (TapContext) combined with a custom setResolveSnapshotPath, the resolver receives mainModule instead of the running test's file — wrong file if tests span multiple files. Worth a comment and/or a follow-up fix.

  • Whole-file rewrite per assertion in update mode. Acknowledged in the comment already, just flagging: with no end-of-run flush hook, N snapshots in a file means N full writes (O(n^2)). Fine for realistic counts.

  • vm.runInNewContext on the snapshot file mirrors Node exactly and the runner-managed default file is trusted, but note node:vm is not a security boundary — a custom-path snapshot pointing at attacker-controlled content would execute arbitrary JS (same as Node, so not a regression, just noting).

Test gaps (non-blocking): no coverage for setResolveSnapshotPath (the custom, permission-checked branch — the one that actually behaves differently in the op) or setDefaultSnapshotSerializers, including its stated effect on fileSnapshot now that defaultFileSnapshotSerializer was removed.

@bartlomieju

Copy link
Copy Markdown
Member

Nice work — the escaping roundtrip and the permission story both check out (explicit path/dir still win over nodeLayout in the match, so it can't be combined with a user path to bypass checks). A few findings:

1. NODE_OPTIONS=--test-update-snapshots breaks t.assert.snapshot()

isSnapshotUpdateMode() is now shared and still accepts NODE_OPTIONS=--test-update-snapshots. That made sense for fileSnapshot (writes via fs), but snapshot() writes through op_test_snapshot_write, which rejects unless the Rust-side SnapshotUpdateMode marker was set by the actual --update-snapshots flag. So:

NODE_OPTIONS=--test-update-snapshots deno test main.js

enters the update branch, sets loaded = true (so the existing file is never read and comparison is silently skipped), and then every snapshot test fails with the generic op error. For snapshot() I'd honor only the op-based mode, or detect the mismatch up front and point the user at --update-snapshots. A spec test for this path would have caught it.

2. TAP-mode wiring is effectively dead

TapContext.assert gets a snapshot method, but under plain deno run the snapshot ops don't exist, so requireSnapshotOps() always throws. That's a reasonable limitation, but it's a gap vs Node (node file.js supports snapshots natively) — worth a note in the PR description or a follow-up issue. Also TapContext uses Deno.mainModule as origin while NodeTestContext uses #denoContext?.origin; if TapContext ever runs under deno test (e.g. run() children) those would disagree.

Minor

  • The no-this-alias suppressions in #snapshotMethods() / TapContext.get assert are avoidable — the arrows can capture this lexically (() => this.fullName).
  • Snapshot '${id}' not found in '${this.path}.' / Cannot read snapshot file '${path}.' — period inside the quotes. Fine if copied verbatim from Node for message parity, otherwise move it out.
  • A custom setResolveSnapshotPath returning a relative path gets test_dir.join(path) in Rust (relative to the test file), whereas Node resolves relative to CWD. Edge case; a path.resolve on the JS side would remove the ambiguity.
  • The permission prompt says Deno.TestContext.assertSnapshot even when triggered from node:test's custom resolver — cosmetic but confusing.
  • Consider enabling Node's own test/parallel/test-runner-snapshot-tests.js from the node_compat suite if it's there — strongest parity signal.

Coverage gaps worth adding: setResolveSnapshotPath (the permission-checked path), setDefaultSnapshotSerializers, and the NODE_OPTIONS case from point 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:test does not implement test.TestContextAssert.snapshot

3 participants