pack.test.js cannot run on any clean clone (public/magi-audio is gitignored) and the suite still prints '674 passed'

[dancockrell]

Fresh clone from GitHub, npm install && npx vitest run:

 FAIL  src/books/magi/pack.test.js [ src/books/magi/pack.test.js ]
Error: ENOENT: no such file or directory, scandir '...\magi-reader-engine\public\magi-audio'
 ❯ src/books/magi/pack.test.js:53:5
     52|   const clips = new Set(
     53|     readdirSync('public/magi-audio')
       |     ^

 Test Files  1 failed | 39 passed (40)
      Tests  674 passed (674)

Exit code 1. Zero failed assertions, and the line most people read says 674 passed.

Why it can never pass on a clone

.gitignore:26

public/magi-audio/

The directory is deliberately not in the repository — correctly, it is a large media folder. So readdirSync('public/magi-audio') is guaranteed to ENOENT on every clean checkout, every CI runner, and every new machine. This is not a flake or a missing npm install step; it is unreachable by construction.

readdirSync is at test-body level, so it throws before a single assertion in the file runs. Whatever pack.test.js was going to check, none of it was checked.

The part that makes it worth fixing rather than ignoring

README.md:86, on why this test exists:

book keeps its audio. The rule only survives if something checks it.

That is exactly right, and the test written to be that check is the one test that cannot execute anywhere the audio is absent. The guarantee holds only on the one machine that already has the files — which is the machine least in need of the check.

And the failure mode is the bad one. A run that skipped its most important file reports:

Tests  674 passed (674)

A reader scanning that line concludes the suite is clean. The exit code disagrees, but the summary is what gets read, and in CI the file-level failure will look like an infrastructure flake rather than a coverage hole. A negative result and an absent result are printing identically, and the absent one looks like success.

The file already imports the tool for this:

import { readFileSync, existsSync, readdirSync } from 'node:fs';

existsSync is imported and used elsewhere in the file, just not in front of this call.

Suggested fix

Make the missing-audio case a deliberate, named skip rather than a crash — and make sure it cannot be mistaken for a pass:

const AUDIO_DIR = process.env.MAGI_AUDIO_DIR || 'public/magi-audio';
const HAVE_AUDIO = existsSync(AUDIO_DIR);

describe.skipIf(!HAVE_AUDIO)('audio pack', () => { ... });

it('reports what was not checked', () => {
  if (!HAVE_AUDIO) console.warn(
    `NOT CHECKED: audio-pack integrity — ${AUDIO_DIR} is absent (it is gitignored). ` +
    `Point MAGI_AUDIO_DIR at a copy, or fetch the pack, to run it.`);
  expect(true).toBe(true);
});

Three things that matter more than the exact shape:

1. Three states, not two. "passed", "failed" and "could not be checked" are all real here. Folding the third into either of the first two is where this went wrong. The run should be able to end on "no failures, but 1 not checked: audio-pack integrity".
2. Put the path behind an env var. Without a seam, the skip branch cannot be exercised deliberately, so nobody can demonstrate it works — and on the machine that does have the audio, the skip path is never taken, so it would rot unnoticed. MAGI_AUDIO_DIR=/nonexistent npx vitest run should exercise it in one command.
3. Say where to get the pack. README.md documents npm test # 480 unit tests with no mention that one file needs media that is not in the repo. A cloner currently discovers this as a stack trace. One line in the README naming the directory, why it is ignored, and where a copy lives is the difference between a five-minute confusion and a fifteen-second one.

Small, related

README.md:52 says npm test # 480 unit tests. The actual figure is 674 passing across 40 files. Not important on its own, but it is a checkable number in a document, and it is wrong — worth deriving it from the run rather than restating it, or dropping the count.

[dancockrell]

Diagnosis was exactly right, including the part that makes it worth fixing: the test written to be the check was the one test that could not run anywhere the audio was absent.

Fixed locally in 6a94880 — not yet pushed, see below.

readdirSync and readFileSync are guarded, and the presence of the media is now its own assertion rather than an implicit precondition. Demonstrated by moving public/magi-audio aside and running the suite, then moving it back.

Before, on a clean clone:

Test Files  1 failed | 39 passed (40)
Tests       674 passed (674)

After:

Test Files  2 failed | 40 passed (42)
Tests       4 failed | 707 passed (711)

AssertionError: public/magi-audio is missing — run: npm run assets

The summary line now disagrees with a clean bill of health, which was the actual damage you named.

The second failing file is src/books/registry.test.js, added earlier today when The Raven was ported in. It makes the same assertion for every registered book, so a new pack that declares media nobody has fails the same way rather than crashing its own file. src/books/raven/pack.test.js was written with the guard from the start.

Why it is not pushed yet: the pre-push hook runs the full e2e suite, and the @serial cross-origin iframe test times out under machine load — currently 14 node and 15 chrome processes from parallel sessions, and a GPU at 96%. That test passes in 3.6s when run alone and times out at 90s inside the chain, and the test that fails moves between runs. 711 unit tests pass on every attempt. Three commits are queued behind it. I am not bypassing the hook to clear them.

[dancockrell]

Your fix shape was better than mine and I have replaced mine with it. e3d1493.

I fixed this earlier today by asserting the media's presence. That was wrong in a way I did not think through: public/magi-audio is gitignored and npm run assets sources it from folders that exist only on this machine, so a CI runner can never satisfy it. My fix turned a clean clone red and CI red forever — which is the same failure as a permanently green suite wearing the other face. People stop reading both.

Skipped now, per your suggestion, with the paths behind MAGI_AUDIO_DIR / RAVEN_AUDIO_DIR so the branch can be exercised on a machine that has the media. Your point that it is otherwise a branch only clean clones ever take, and they are the ones least able to report on it, is the part I would not have got to.

A clean clone, media moved aside:

Tests  706 passed | 9 skipped (715)     exit 0

against the original

Test Files  1 failed | 39 passed (40)
Tests       674 passed (674)            exit 1

Demonstrated both directions rather than assumed:

npx vitest run src/books/magi/pack.test.js        8 passed
MAGI_AUDIO_DIR=/nonexistent  ... same file        4 passed | 4 skipped

What stays unconditional is the pack declaring where its media lives — a fact about the pack rather than about this checkout. Only "is it on disk here" is environment, and only that is skipped. src/books/registry.test.js enforces the declaration for every registered book.

README:52 is fixed too, and I removed the number rather than correcting it: a count in a README is stale on the commit after it is written. It now says the suite prints its own.

Not pushed yet — the pre-push hook runs the full e2e suite and the @serial iframe test times out under load. Six commits queued.