Skip to content

check-test-wiring drops an unreadable package with existsSync, the defect its sibling was hardened against #3347

Description

@louistrue

scripts/check-test-wiring.mjs discovers packages with existsSync(pkgJsonPath), which answers false for every failure, not just "the file is not there". A package whose manifest cannot be read for any other reason is silently dropped from the audit instead of failing it.

Its sibling scripts/check-test-glob-coverage.mjs was already hardened against exactly this. check-test-wiring.mjs never got the same treatment.

Evidence

scripts/check-test-wiring.mjs, two discovery loops, lines 158 and 310:

const pkgJsonPath = join(pkgDir, 'package.json');
if (!existsSync(pkgJsonPath)) continue;

scripts/check-test-glob-coverage.mjs:316-328, the hardened form:

function existsOrThrow(path, what) {
  try {
    statSync(path);
    return true;
  } catch (err) {
    if (err.code === 'ENOENT') return false;
    fail(
      `cannot read ${what} ${path}: ${err.code || err.message}. ` +
        'Refusing to treat an unreadable path as an absent one -- that is how a ' +
        'package drops out of the audit without anyone noticing.',
    );
  }
}

existsSync returns false on ENOTDIR, EACCES and every other error. existsOrThrow returns false only on ENOENT and fails loudly otherwise.

The failing case

check-test-wiring is the gate that fails when a package with test files has no test script, so turbo test would silently skip it. If that package's package.json becomes unreadable rather than absent (a permissions change, a path that is not a directory, a broken symlink into a partial checkout), existsSync says false, the loop continues, and the package is not audited. The gate reports OK. A package with tests that never run then reads exactly the same as a package with no problem, which is the failure mode the sibling's comment names in so many words.

What this is not

It does not crash on a stray .DS_Store the way check-test-glob-coverage did before #3345's sibling fix. existsSync tolerates it quietly. That quiet tolerance is the finding, not a separate bug.

Proposed fix, and why it is not one line

Adopt existsOrThrow in check-test-wiring.mjs. That immediately reintroduces the .DS_Store crash in a second gate, so the dotfile skip has to land in the same change. Two edits, one new failure mode, and it wants the same both-directions proof: a dotfile is skipped, and a genuinely unreadable package still fails the gate.

The same weaker existsSync shape appears in scripts/check-api-surface.mjs:74, scripts/check-generated.mjs:213 and scripts/verify-esm-entrypoints.mjs:84. scripts/release-version-changed.mjs:106 and scripts/typecheck-tests.mjs:269 use entry.isDirectory() / statSync(dir).isDirectory() instead, which is structurally correct rather than accidentally quiet.

How this was found

While fixing a related flake in check-test-glob-coverage.mjs. No code change for this has been made.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions