From 156303fc86585dbccf339c9c52ad0e3288601064 Mon Sep 17 00:00:00 2001 From: Raman Sinclair Date: Mon, 20 Jul 2026 16:34:36 +0200 Subject: [PATCH] test: add separate snapshot update command and rely on object comparison rather than strings In the previous commit I tried to stabilise the produced snapshots so that they match the output from the discogs function, however it lead to some hidden problems. 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. --- package.json | 2 ++ tests/discogs/README.md | 13 ++++++----- tests/discogs/discogs.test.ts | 37 +++---------------------------- tests/discogs/update-snapshots.ts | 22 ++++++++++++++++++ 4 files changed, 34 insertions(+), 40 deletions(-) create mode 100644 tests/discogs/update-snapshots.ts diff --git a/package.json b/package.json index a2f1a8f..e515e82 100644 --- a/package.json +++ b/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", diff --git a/tests/discogs/README.md b/tests/discogs/README.md index 8a2afbe..97fbb80 100644 --- a/tests/discogs/README.md +++ b/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/`), 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. diff --git a/tests/discogs/discogs.test.ts b/tests/discogs/discogs.test.ts index 30173cc..c1f1fde 100644 --- a/tests/discogs/discogs.test.ts +++ b/tests/discogs/discogs.test.ts @@ -13,31 +13,6 @@ 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, @@ -45,15 +20,9 @@ const releases = RELEASES.map(({ url, description }) => ({ test.each(releases)('$id: $description', ({ id }) => { const fixture = JSON.parse(fs.readFileSync(path.join(FIXTURES_DIR, `${id}.json`), 'utf8')) as Record; - 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; - if (shouldUpdateSnapshots() || !fs.existsSync(snapshotPath)) { - fs.writeFileSync(snapshotPath, serializedResult, 'utf8'); - return; - } + const result = JSON.parse(JSON.stringify(parseDiscogsRelease(fixture))) as Record; - const snapshot = JSON.parse(fs.readFileSync(snapshotPath, 'utf8')) as Record; - expect(JSON.parse(serializedResult)).toEqual(snapshot); + expect(result).toEqual(snapshot); }); diff --git a/tests/discogs/update-snapshots.ts b/tests/discogs/update-snapshots.ts new file mode 100644 index 0000000..1657676 --- /dev/null +++ b/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; + const result = parseDiscogsRelease(fixture); + + fs.writeFileSync(path.join(SNAPSHOTS_DIR, `${id}.json`), `${JSON.stringify(result, null, 4)}\n`); +}