adopt vitest by Fil · Pull Request #2373 · observablehq/plot

Fil · 2026-03-04T20:30:27Z

This:

replaces mocha by vitest as the preferred test framework
~~reinstates the offset logic for DPI=1 — but tests with offset=0 (high-res screen by default)~~
~~fixes an offset inconsistency in the hex marks~~ (moved to fix axis offset in snapshots #2375)

should be merged together with #2375, but there are two PRs to help review the many snapshots changes (in #2375) and at the same time ensure that changing the test framework does not modify the snapshot (this PR).

AI disclosure: I used @claude to generate a first draft this PR.

test/setup.js

mbostock · 2026-03-04T20:51:22Z

test/style-test.js

@@ -1,3 +1,4 @@
+import {it} from "vitest";
 import assert from "assert";


I think assert should be imported from vitest now?

There are still many cases of import assert from "assert"; so either we should discuss further what the desired import should be or we should make the corresponding changes. Not sure why this comment was resolved.

Sorry I misunderstood your comment. It's probably more consistent to import assert from vitest rather than from the node built-in? I'll change that.

EDIT: it means I also need to change the regexp in some tests, because vitest's version of assert (reexported from chai) doesn't return the leading Error: in its messages.

mbostock · 2026-03-04T20:55:36Z

test/plot.js

@@ -56,7 +63,7 @@ for (const [name, plot] of Object.entries(plots)) {
      await fs.writeFile(diffile, actual, "utf8");
    }

-    assert.ok(equal, `${name} must match snapshot`);
+    expect(equal, `${name} must match snapshot`).toBe(true);


Don’t we want to use Vitest’s file snapshots here? In my mind that was one of the primary goals of adopting Vitest. See (internal) example: https://github.com/observablehq/charts/blob/main/test/charts/test.ts

The issue here is that snapshots have embedded images and that the file snapshots need to discard them (with something like <replaced>) because of inconsistencies between platforms. We can adopt a system where the images are compared separately, but it's not so great when you review changes visually.

I think we can do it with a custom snapshot serializer.

New strategy: extract the images to the side as PNGs, apply the image comparison algo to them, and link them by name instead of the <replaced> string.

As I understand it a custom serializer would create the output as a .snap file containing the SVG/HTML structure and the images. But the comparison would still want an exact equality, which can't be guaranteed across platforms. We need the comparison to say "ok" even when files are slightly different, but I think I found a way to do this by doing the substitution early if the image discrepancy is low.

Didn’t we have the same problem with Charts, and we previously implemented a solution for this in the example I linked? https://github.com/observablehq/charts/blob/main/test/charts/test.ts I guess the difference is in Charts we only worried about a canvas element, but I think rewriting the SVG to redact images (that will be compared separately) makes sense. We don’t need the SVG comparison to test images if we’re separately testing the image snapshots. And we can manually test image equality if we want a fuzzy match that won’t work with toMatchFileSnapshot, but the logic of when to regenerate snapshots (shouldUpdateSnapshot) should match Vitest’s logic for consistency.

Yes. But let me argue why what I have implemented here feels like an improvement over what we did there.

To recap, the original issue: we want to use vitest's toMatchFileSnapshot for the best dev experience (vitest -u to regenerate snapshots, or typing 'u' in live testing). However, the binary images generated vary slightly by platform, which can make tests fail strict equality in CI even though "it's all good". To get around that, we would need a custom check for "fuzzy equal", but toMatchFileSnapshot does not support custom checks.

Some of our charts are an HTML combination of a <svg> and an <img>, which we disassemble and test separately. A frustration for me with this approach is that since we save them and test them as separate pieces, we can't open the snapshots in a browser as a whole, for example to check that they are well aligned, or are in the correct z order. In Plot, there are more cases — images embedded in svg, or a single plot that contains dozens of images.

The approach I've implemented here keeps the plot "whole", exactly as it was generated, in one file. So you can open the snapshot in a browser and everything is there. To prevents platform-variations errors, we do an early comparison: if the generated image is different from that in the reference snapshot, we fuzzy check it, and if that fuzzy test succeeds, we consider it to be the same image and substitute the reference image into the "fresh" chart, before we call toMatchFileSnapshot. This will not error on fuzzy image differences, but will still error if anything else has changed.

I also just realized that with this approach we don't even need shouldUpdateSnapshot (which checks the internal state of vitest), so the code feels cleaner. The actual usefulness of regenerating snapshots (vitest -u) is for when we update the code and want to see the diff in snapshots. The only thing the approach I've implemented here does not support is if we actually need to regenerate the binary images for some reason (maybe a new version of node-canvas, or we all start developing on different machines), then vitest -u will not update the files; in that event, we'd only have to rm the snapshots manually.

I see the value in having our snapshots include the images inline so that they are directly inspectable in the browser.

I see that this would mean not being able to us toMatchFileSnapshot because that requires an exact match. So we need our own implementation, but it should be as close to toMatchFileSnapshot as we can make it.

Even if we can’t use toMatchFileSnapshot, I still want to conform to Vitest’s snapshot workflow; that’s why I implemented shouldUpdateSnapshot. I think it’s nice that you can hit u or whatever to update the snapshots rather than having to manually delete them from your file system and rerun the tests. We shouldn’t delete that useful logic for the sake of making the code cleaner.

mbostock · 2026-03-05T20:58:51Z

test/style-test.js

+  expect(maybeClassName("foo")).toBe("foo");
+  expect(maybeClassName("foo2")).toBe("foo2");
+  expect(maybeClassName("foo-bar")).toBe("foo-bar");
+  expect(maybeClassName("-bar")).toBe("-bar");


I don’t think we should rewrite these to use expect; assert.strictEqual should continue to work fine with Vitest, and we want to minimize the diff.

mbostock

I bet we could make these change without having to regenerate all the snapshots… we could do some preprocessing to address the precision issue. That would make it easier to review and ensure that this isn’t introducing any unexpected changes.

Fil · 2026-03-06T13:33:09Z

I bet we could make these change without having to regenerate all the snapshots

Our snapshots in main, using mocha, are in fact wrong because the offset is computed inconsistently in the test section. (When we import d3-axis, the "fake" global.window object is not set up yet, so D3 axes have an offset of 0, whereas other elements in Plot use an offset of 0.5 because JSDOM's default DPI is 1.)

I created a fix-axis-offset branch off of main that only fixes the offset, and then rebased this branch on top of fix-axis-offset.

This should make it easier to review. All the changes to snapshots are in #2375.

…ted images

…leading Error: in its messages. Drop the caret ^ so that if we ever went back to Node's assert, we won't have to add 'Error: ' back.

Rationale: if we need to regenerate the snapshots (vitest -u aor interactively), it's not to overcome the platform issues. For that you just have to rm all the files and start fresh…

Fil requested a review from mbostock March 4, 2026 20:30

mbostock reviewed Mar 4, 2026

View reviewed changes

test/setup.js Outdated Show resolved Hide resolved

mbostock reviewed Mar 4, 2026

View reviewed changes

test/setup.js Outdated Show resolved Hide resolved

mbostock reviewed Mar 4, 2026

View reviewed changes

Fil force-pushed the fil/vitest branch 2 times, most recently from 5ea6a23 to 635d2aa Compare March 5, 2026 11:47

mbostock reviewed Mar 5, 2026

View reviewed changes

Fil mentioned this pull request Mar 6, 2026

fix axis offset in snapshots #2375

Open

Fil changed the base branch from main to fil/fix-axis-offset March 6, 2026 12:20

This comment was marked as outdated.

Sign in to view

Fil marked this pull request as draft March 6, 2026 13:08

Fil force-pushed the fil/vitest branch 2 times, most recently from 64ea8b9 to c2e0667 Compare March 6, 2026 13:29

Fil changed the title ~~adopt vitest; fix hex bug~~ adopt vitest Mar 7, 2026

Fil force-pushed the fil/vitest branch from c2e0667 to d43eb8d Compare March 7, 2026 15:30

Fil force-pushed the fil/fix-axis-offset branch from e4658e0 to 8229321 Compare March 7, 2026 16:20

Fil added 7 commits March 7, 2026 17:31

update github action

9d03912

adopt vitest; substitute images if they approximately match the expec…

e9ce249

…ted images

restore coverage

2b2f89f

minimize diff

0828a49

import assert from vitest instead of the Node built-in

e516323

vitest's version of assert (reexported from chai) doesn't return the …

bc6d497

…leading Error: in its messages. Drop the caret ^ so that if we ever went back to Node's assert, we won't have to add 'Error: ' back.

we don't even need to check the internal state of vitest

0664444

Rationale: if we need to regenerate the snapshots (vitest -u aor interactively), it's not to overcome the platform issues. For that you just have to rm all the files and start fresh…

Fil force-pushed the fil/vitest branch from d43eb8d to 0664444 Compare March 7, 2026 16:33

Fil marked this pull request as ready for review March 7, 2026 16:34

		@@ -1,3 +1,4 @@
		import {it} from "vitest";
		import assert from "assert";

Conversation

Fil commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fil Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbostock left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Fil commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fil commented Mar 4, 2026 •

edited

Loading

Fil Mar 6, 2026 •

edited

Loading

Fil commented Mar 6, 2026 •

edited

Loading