test: add separate snapshot update command and rely on object comparison rather than strings (#1007)

In the previous commit I tried to stabilise the produced snapshots so
that they match the output from the discogs function, however it led to
some hidden problems with oxfmt. The only reason I tried to do it is to
keep the `pnpm test:discogs -u` command so that vitest handles snapshot
files, however fixing the hidden issue mentioned above together with
already a complicated setup I think we shouldn't rely on snapshot
comparison as strings at all. It's much easier to compare them as
objects and just create a separate command to regenerate snapshots, and
this is what the current PR is doing.

Author: arsinclair

package.json

@@ -31,8 +31,10 @@
         "lint-fix": "pnpm run eslint -- --fix",
         "oxlint": "oxlint --type-aware",
         "eslint": "eslint .",
+        "check": "pnpm run format:check && pnpm run type-check && pnpm run lint",
         "test": "vitest run",
         "test:discogs": "vitest run tests/discogs",
+        "test:discogs:update-snapshots": "node --strip-types tests/discogs/update-snapshots.ts && oxfmt tests/discogs/snapshots",
         "test:discogs:update-fixtures": "node --strip-types tests/discogs/fetch-fixtures.ts && pnpm run format tests/discogs/fixtures",
         "test:watch": "vitest",
         "generate-readme": "node --strip-types ./tools/generate-readme.ts > README.md",

tests/discogs/README.md

@@ -20,7 +20,7 @@ A **fixture** is the JSON returned by the Discogs API for a release (e.g. `fixtu
 
 A **snapshot** is the JSON produced by running `parseDiscogsRelease` on that fixture (e.g. `snapshots/1996829.json`). It is the expected MusicBrainz import data: artist credits, labels, discs, tracks, durations, and so on.
 
-Snapshot files are regenerated by the tests with stable key ordering when `pnpm test:discogs -u` is used.
+Snapshot files are formatted by oxfmt. Regeneration is a separate command from the tests because the tests compare parsed JSON rather than serialized text.
 
 When you change the parser, tests re-parse the fixtures and compare the result to the snapshots.
 
@@ -48,8 +48,9 @@ CI runs `pnpm test:discogs` on every pull request. No Discogs API calls are made
     Use the API URL (`https://api.discogs.com/releases/<id>`), not the website URL.
 
 3. Fetch the fixture.
-4. Run all tests. Missing snapshot will be automatically created.
-5. Commit the new fixture, snapshot, and config change.
+4. Regenerate the snapshots.
+5. Run all tests.
+6. Commit the new fixture, snapshot, and config change.
 
 Pick releases that cover distinct edge cases (multi-disc tracklists, unusual side numbering, nested sub-tracks, etc.) rather than many similar ones.
 
@@ -70,9 +71,9 @@ pnpm test:discogs:update-fixtures
 Regenerate snapshots after an intentional parser change (no network request):
 
 ```bash
-pnpm test:discogs -u
+pnpm test:discogs:update-snapshots
 ```
 
-After adding an entry to `config.ts`, run `pnpm test:discogs:update-fixtures` once to create its fixture, then `pnpm test:discogs` to initialize its snapshot and confirm everything passes.
+After adding an entry to `config.ts`, run `pnpm test:discogs:update-fixtures` once to create its fixture, then regenerate the snapshots and run the tests.
 
-If a test fails after a parser change you intended, review the diff, then run `pnpm test:discogs -u` to approve the new snapshots and commit the updated files.
+If a test fails after a parser change you intended, review the failure, regenerate the snapshots, inspect the file diff, and commit the updated files.

tests/discogs/discogs.test.ts

@@ -13,47 +13,16 @@ const SNAPSHOTS_DIR = path.join(DIR, 'snapshots');
 
 const parseDiscogsRelease = loadParseDiscogsRelease();
 
-function sortJsonValue(value: unknown): unknown {
-    if (Array.isArray(value)) {
-        return value.map(sortJsonValue);
-    }
-
-    if (value !== null && typeof value === 'object') {
-        return Object.fromEntries(
-            Object.entries(value)
-                .sort(([left], [right]) => left.localeCompare(right))
-                .map(([key, child]) => [key, sortJsonValue(child)]),
-        );
-    }
-
-    return value;
-}
-
-function stableStringify(value: unknown): string {
-    return `${JSON.stringify(sortJsonValue(JSON.parse(JSON.stringify(value))), null, 4)}\n`;
-}
-
-// These tests compare parsed JSON structurally, so mirror Vitest's snapshot update flags explicitly.
-function shouldUpdateSnapshots(): boolean {
-    return process.argv.some(arg => arg === '-u' || arg === '--update' || arg === '--updateSnapshot');
-}
-
 const releases = RELEASES.map(({ url, description }) => ({
     id: path.basename(new URL(url).pathname),
     description,
 }));
 
 test.each(releases)('$id: $description', ({ id }) => {
     const fixture = JSON.parse(fs.readFileSync(path.join(FIXTURES_DIR, `${id}.json`), 'utf8')) as Record<string, unknown>;
-    const snapshotPath = path.join(SNAPSHOTS_DIR, `${id}.json`);
-    const result = parseDiscogsRelease(fixture);
-    const serializedResult = stableStringify(result);
+    const snapshot = JSON.parse(fs.readFileSync(path.join(SNAPSHOTS_DIR, `${id}.json`), 'utf8')) as Record<string, unknown>;
 
-    if (shouldUpdateSnapshots() || !fs.existsSync(snapshotPath)) {
-        fs.writeFileSync(snapshotPath, serializedResult, 'utf8');
-        return;
-    }
+    const result = JSON.parse(JSON.stringify(parseDiscogsRelease(fixture))) as Record<string, unknown>;
 
-    const snapshot = JSON.parse(fs.readFileSync(snapshotPath, 'utf8')) as Record<string, unknown>;
-    expect(JSON.parse(serializedResult)).toEqual(snapshot);
+    expect(result).toEqual(snapshot);
 });

tests/discogs/update-snapshots.ts

@@ -0,0 +1,22 @@
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+import { RELEASES } from './config.ts';
+import { loadParseDiscogsRelease } from './load-parser.ts';
+
+const DIR = path.dirname(fileURLToPath(import.meta.url));
+const FIXTURES_DIR = path.join(DIR, 'fixtures');
+const SNAPSHOTS_DIR = path.join(DIR, 'snapshots');
+
+const parseDiscogsRelease = loadParseDiscogsRelease();
+
+fs.mkdirSync(SNAPSHOTS_DIR, { recursive: true });
+
+for (const { url } of RELEASES) {
+    const id = path.basename(new URL(url).pathname);
+    const fixture = JSON.parse(fs.readFileSync(path.join(FIXTURES_DIR, `${id}.json`), 'utf8')) as Record<string, unknown>;
+    const result = parseDiscogsRelease(fixture);
+
+    fs.writeFileSync(path.join(SNAPSHOTS_DIR, `${id}.json`), `${JSON.stringify(result, null, 4)}\n`);
+}