From d50400001242450f9496ed64bd5e903117f5d213 Mon Sep 17 00:00:00 2001 From: calixteman Date: Sun, 19 Jul 2026 15:34:18 +0200 Subject: [PATCH 1/3] Download the per-test coverage index `coverage_search` now fetches `per-test-index.json` from the pdf.js.refs gh-pages branch, caches it locally, and only re-downloads it (via ETag) when it changed, so querying which ref tests cover a line/function no longer needs a local `--coverage-per-test` build. `--no-download` reuses the cached index offline and `--index` points at a local one. `browsertest` and `makeref` accept the same `--code` filter to run (or regenerate refs for) only the covering tests, dropping any covered IDs that aren't in this branch's manifest so the run isn't rejected. --- README.md | 25 +++- external/ccov/coverage_search.mjs | 164 ++++++++++++++++++++--- gulpfile.mjs | 213 +++++++++++++++++++++++------- 3 files changed, 334 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index eb18e2eae..4a61378fb 100644 --- a/README.md +++ b/README.md @@ -136,14 +136,31 @@ browsable HTML report instead, or pass several formats at once, e.g. ### Finding which tests cover a given line -Run a browser test task with `--coverage-per-test` to build an index -(`per-test-index.json`) in the coverage directory, then query it to list the -tests that exercised a specific source line or function: +`coverage_search` lists the ref tests that exercised a specific source line or +function. It uses the per-test index (`per-test-index.json`) that is rebuilt on +every push to `master` and published to the +[`pdf.js.refs`](https://github.com/mozilla/pdf.js.refs/tree/gh-pages) +repository. The index is downloaded on demand, cached locally, and only +re-downloaded when it has changed, so no local coverage build is required: - $ npx gulp botbrowsertest --coverage-per-test $ npx gulp coverage_search --code="canvas.js::205" $ npx gulp coverage_search --code="canvas.js::drawImageAtIntegerCoords" +To run — or regenerate the reference images for — only the ref tests that touch +a given line or function, pass the same `--code` option to a browser test or +`makeref` task: + + $ npx gulp browsertest --code="canvas.js::205" + $ npx gulp makeref --code="canvas.js::205" + +Pass `--no-download` to reuse the locally cached index without contacting the +network. The index can also be built and queried locally (the CI job that +publishes it builds it the same way): + + $ npx gulp botbrowsertest --coverage-per-test + $ npx gulp coverage_search --code="canvas.js::205" \ + --index=build/coverage/per-test-index.json --no-download + ### Continuous integration On every push and pull request three GitHub Actions workflows collect coverage diff --git a/external/ccov/coverage_search.mjs b/external/ccov/coverage_search.mjs index 6055c0855..ba4e3547b 100644 --- a/external/ccov/coverage_search.mjs +++ b/external/ccov/coverage_search.mjs @@ -20,26 +20,46 @@ import path from "path"; const __dirname = import.meta.dirname; const PROJECT_ROOT = path.join(__dirname, "../.."); -const { values } = parseArgs({ - args: process.argv.slice(2), - options: { - code: { type: "string" }, - "coverage-dir": { type: "string", default: "build/coverage" }, - help: { type: "boolean", short: "h", default: false }, - }, -}); +// The per-test coverage index (which ref test exercises which source +// line/function) is rebuilt on every push to master and published to the +// gh-pages branch of the pdf.js.refs repository. +const PER_TEST_INDEX_URL = + "https://raw.githubusercontent.com/mozilla/pdf.js.refs/gh-pages/per-test-index.json"; + +let values; +try { + ({ values } = parseArgs({ + args: process.argv.slice(2), + options: { + code: { type: "string" }, + index: { type: "string", default: "build/per-test-index.json" }, + "no-download": { type: "boolean", default: false }, + help: { type: "boolean", short: "h", default: false }, + }, + })); +} catch (error) { + // parseArgs is strict, so an unknown/renamed option (e.g. the removed + // --coverage-dir) would otherwise abort with an uncaught stack trace. + console.error(`Error: ${error.message}`); + console.error("Run with --help to see the available options."); + process.exit(1); +} if (values.help || !values.code) { console.log( - "Usage: coverage_search.mjs --code=:: [--coverage-dir=]\n\n" + + "Usage: coverage_search.mjs --code=:: [--index=] [--no-download]\n\n" + " --code Source file and line number or function name to search for.\n" + " Examples:\n" + " --code=canvas.js::205\n" + " --code=canvas.js::drawImageAtIntegerCoords\n" + - " --coverage-dir Coverage directory containing per-test-index.json [build/coverage]\n\n" + + " --index Where to cache or read the per-test index.\n" + + " [build/per-test-index.json]\n" + + " --no-download Don't contact the network; use the cached index as-is.\n\n" + "Prints to stdout the IDs of tests whose coverage includes the given line or\n" + - "function (one ID per line).\n" + - "Run browsertest with --coverage-per-test first to generate the index." + "function (one ID per line).\n\n" + + "The index is downloaded from the pdf.js.refs repository and cached locally;\n" + + "it is only re-downloaded when the published file has changed.\n" + + `Source: ${PER_TEST_INDEX_URL}` ); process.exit(values.help ? 0 : 1); } @@ -58,18 +78,124 @@ const isLine = /^\d+$/.test(location); const lineNum = isLine ? parseInt(location, 10) : null; const funcName = isLine ? null : location; -const coverageDir = path.isAbsolute(values["coverage-dir"]) - ? values["coverage-dir"] - : path.join(PROJECT_ROOT, values["coverage-dir"]); +const indexPath = path.isAbsolute(values.index) + ? values.index + : path.join(PROJECT_ROOT, values.index); +// The ETag of the cached copy is stored alongside it, so the next run can ask +// the server (via If-None-Match) to only re-send the file when it has changed. +const etagPath = `${indexPath}.etag`; + +// Refreshes the locally cached index from the published copy, downloading it +// only when it has changed since the last run. When the network is unavailable +// a previously cached copy is reused if present. +async function refreshIndex() { + const hasCached = fs.existsSync(indexPath); + + if (values["no-download"]) { + return; // Freshness check disabled; the read below validates existence. + } + + // On any download failure, fall back to a previously cached copy when one + // exists; otherwise there's nothing to search, so fail. + const fallbackOrFail = reason => { + if (hasCached) { + console.error( + `Warning: couldn't refresh per-test index (${reason}); using the cached copy.` + ); + return; + } + console.error(`Error: couldn't download per-test index (${reason}).`); + process.exit(1); + }; + + const headers = {}; + if (hasCached && fs.existsSync(etagPath)) { + const etag = fs.readFileSync(etagPath, "utf8").trim(); + if (etag) { + headers["If-None-Match"] = etag; + } + } + + let response; + try { + console.error(`Fetching per-test index from ${PER_TEST_INDEX_URL} ...`); + response = await fetch(PER_TEST_INDEX_URL, { headers }); + } catch (error) { + fallbackOrFail(error.message); + return; + } + + if (response.status === 304) { + console.error("Per-test index is up to date."); + return; + } + if (!response.ok) { + fallbackOrFail(`HTTP ${response.status}`); + return; + } + + let data; + try { + data = Buffer.from(await response.arrayBuffer()); + } catch (error) { + fallbackOrFail(error.message); + return; + } + + // Validate the payload before caching it. + try { + JSON.parse(data.toString("utf8")); + } catch { + fallbackOrFail("the downloaded index is not valid JSON"); + return; + } + + // Write to a temporary file and rename it into place. + try { + fs.mkdirSync(path.dirname(indexPath), { recursive: true }); + const tmpPath = `${indexPath}.${process.pid}.tmp`; + fs.writeFileSync(tmpPath, data); + fs.renameSync(tmpPath, indexPath); + + const etag = response.headers.get("etag"); + if (etag) { + fs.writeFileSync(etagPath, etag); + } else { + fs.rmSync(etagPath, { force: true }); + } + } catch (error) { + // A write failure (disk full, read-only dir, ...) shouldn't be fatal when + // a usable cached copy already exists. + fallbackOrFail(error.message); + return; + } + console.error(`Per-test index updated (${data.length} bytes).`); +} + +await refreshIndex(); -const indexPath = path.join(coverageDir, "per-test-index.json"); if (!fs.existsSync(indexPath)) { - console.error(`Error: index file not found: ${indexPath}`); - console.error("Run browsertest with --coverage-per-test first."); + console.error(`Error: per-test index not found: ${indexPath}`); + console.error( + "Build it locally (gulp botbrowsertest --coverage-per-test) or omit " + + "--no-download to fetch it from the pdf.js.refs repository." + ); process.exit(1); } -const { ids, files } = JSON.parse(fs.readFileSync(indexPath, "utf8")); +let ids, files; +try { + ({ ids, files } = JSON.parse(fs.readFileSync(indexPath, "utf8"))); +} catch (error) { + console.error( + `Error: couldn't read per-test index at ${indexPath}: ${error.message}` + ); + console.error( + "The cached index may be corrupt; delete it and re-run without " + + "--no-download to refetch it." + ); + process.exit(1); +} // Find the file entry whose path matches fileName. let fileEntry = null; diff --git a/gulpfile.mjs b/gulpfile.mjs index 03325d4e0..8712d1016 100644 --- a/gulpfile.mjs +++ b/gulpfile.mjs @@ -793,36 +793,18 @@ function runTests(testsName, { bot = false } = {}) { args.push("--coveragePerTest"); } - const codeArg = testsName === "browser" ? getArgValue("--code") : null; - if (codeArg) { - const coverageDir = - getArgValue("--coverage-output") || BUILD_DIR + "coverage"; - const result = spawnSync( - "node", - [ - path.join(__dirname, "external/ccov/coverage_search.mjs"), - `--code=${codeArg}`, - `--coverage-dir=${coverageDir}`, - ], - { encoding: "utf8" } - ); - if (result.status !== 0) { - reject(new Error(result.stderr?.trim() || "coverage_search failed")); + if (testsName === "browser") { + let shouldRun; + try { + shouldRun = applyCodeTestFilter(args); + } catch (error) { + reject(error); return; } - const testIds = result.stdout.trim().split("\n").filter(Boolean); - if (testIds.length === 0) { - console.log(`\n### No tests found covering "${codeArg}"`); + if (!shouldRun) { resolve(); return; } - console.log( - `\n### Found ${testIds.length} test(s) covering "${codeArg}":\n` + - testIds.map(id => ` ${id}`).join("\n") - ); - for (const id of testIds) { - args.push(`-t=${id}`); - } } const testProcess = startNode(args, { cwd: TEST_DIR, stdio: "inherit" }); @@ -888,6 +870,136 @@ function collectArgs(options, args) { } } +// Builds the coverage_search command line. By default the published index is +// downloaded; an explicit --index= selects a local index instead, used +// read-only so it's never overwritten by the published copy. +function getCoverageSearchArgs(codeArg) { + const searchArgs = [ + path.join(__dirname, "external/ccov/coverage_search.mjs"), + `--code=${codeArg}`, + ]; + const indexArg = getArgValue("--index"); + if (indexArg === "") { + // An explicit but empty value (e.g. an unset shell variable expanding to + // `--index=`) almost certainly isn't intended; fail loudly rather than + // silently falling back to the downloaded index. + throw new Error("--index was given without a value"); + } + if (indexArg) { + searchArgs.push(`--index=${indexArg}`, "--no-download"); + } else if (process.argv.includes("--no-download")) { + searchArgs.push("--no-download"); + } + return searchArgs; +} + +// Returns the set of test IDs defined in the local ref-test manifest, or null +// when it can't be read. Used to drop coverage-derived IDs that don't exist on +// this branch (e.g. a test renamed since the published index was built), which +// would otherwise make test.mjs reject the entire run. +function readManifestTestIds() { + try { + const manifestFile = process.env.PDF_TEST || "test_manifest.json"; + const manifest = JSON.parse( + fs.readFileSync(path.join(__dirname, TEST_DIR, manifestFile), "utf8") + ); + return new Set(manifest.map(entry => entry.id)); + } catch { + return null; + } +} + +// For a --code=:: argument, runs coverage_search to find +// the ref tests that exercise that location and returns their IDs (an empty +// array when none match locally). Returns null when --code wasn't given; throws +// when the search itself fails. +function resolveCodeTestIds() { + const codeArg = getArgValue("--code"); + if (!codeArg) { + return null; + } + // Inherit stderr so the index download progress is visible; stdout is + // captured because it carries the matching test IDs. + const result = spawnSync("node", getCoverageSearchArgs(codeArg), { + encoding: "utf8", + stdio: ["ignore", "pipe", "inherit"], + }); + if (result.status !== 0) { + // status is null when the child couldn't be spawned or was killed by a + // signal; surface the real cause instead of a generic message. + throw new Error( + result.error + ? `coverage_search failed: ${result.error.message}` + : `coverage_search failed (exit code ${result.status})` + ); + } + let testIds = result.stdout.trim().split("\n").filter(Boolean); + + // The published index is built from master, so some covering tests may not + // exist on this branch. Drop them (with a note) rather than letting test.mjs + // reject the whole run — and silently exit 0 — on the first unknown ID. + const knownIds = readManifestTestIds(); + if (knownIds) { + const missing = testIds.filter(id => !knownIds.has(id)); + if (missing.length) { + console.log( + `\n### Ignoring ${missing.length} covered test(s) not in the manifest:\n` + + missing.map(id => ` ${id}`).join("\n") + ); + testIds = testIds.filter(id => knownIds.has(id)); + } + } + + if (testIds.length === 0) { + console.log(`\n### No tests found covering "${codeArg}"`); + } else { + console.log( + `\n### Found ${testIds.length} test(s) covering "${codeArg}":\n` + + testIds.map(id => ` ${id}`).join("\n") + ); + } + return testIds; +} + +function appendTestFilters(args, testIds) { + const existingIds = new Set(); + + // collectArgs always normalizes filters to the space-separated + // `-t ` / `--testfilter ` form, so that's the only shape to read. + for (let i = 0; i < args.length; i++) { + if (args[i] === "-t" || args[i] === "--testfilter") { + if (i + 1 < args.length) { + existingIds.add(args[++i]); + } + } + } + for (const id of testIds) { + if (!existingIds.has(id)) { + // Pass the id as a separate argument: node's parseArgs parses the short + // form `-t=` as the literal value "=", which matches no test and + // aborts the run with "Unrecognized test IDs". + args.push("-t", id); + existingIds.add(id); + } + } +} + +// Applies the --code coverage filter to `args`. Returns false when the run +// should be skipped (--code was given but resolved to no runnable tests), and +// true otherwise (no --code, or matching tests were appended). Throws when the +// coverage search itself fails. +function applyCodeTestFilter(args) { + const codeTestIds = resolveCodeTestIds(); + if (codeTestIds === null) { + return true; // No --code argument; run normally. + } + if (codeTestIds.length === 0) { + return false; // Nothing (runnable) covers the requested location. + } + appendTestFilters(args, codeTestIds); + return true; +} + function makeRef(done, bot) { console.log("\n### Creating reference images"); @@ -936,6 +1048,18 @@ function makeRef(done, bot) { args ); + let shouldRun; + try { + shouldRun = applyCodeTestFilter(args); + } catch (error) { + done(error); + return; + } + if (!shouldRun) { + done(); + return; + } + const testProcess = startNode(args, { cwd: TEST_DIR, stdio: "inherit" }); testProcess.on("close", function (code) { if (code !== 0) { @@ -946,34 +1070,33 @@ function makeRef(done, bot) { }); } -// Queries the per-test coverage index built by --coverage-per-test and prints -// the IDs of tests that exercised a given source file location. Run with -// --code=::, e.g. --code=canvas.js::205 +// Prints the IDs of tests that exercised a given source file location, using +// the per-test coverage index published to the pdf.js.refs repository. The +// index is downloaded on demand, cached locally, and only re-downloaded when +// it has changed. Run with --code=::, e.g. +// --code=canvas.js::205 (add --no-download to reuse the cached index offline). gulp.task("coverage_search", function (done) { const codeArg = getArgValue("--code"); if (!codeArg) { done(new Error('Missing --code argument, e.g. --code="canvas.js::205"')); return; } - const coverageDir = - getArgValue("--coverage-output") || BUILD_DIR + "coverage"; - const result = spawnSync( - "node", - [ - path.join(__dirname, "external/ccov/coverage_search.mjs"), - `--code=${codeArg}`, - `--coverage-dir=${coverageDir}`, - ], - { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] } - ); - if (result.stderr) { - process.stderr.write(result.stderr); - } - if (result.stdout) { - process.stdout.write(result.stdout); + let searchArgs; + try { + searchArgs = getCoverageSearchArgs(codeArg); + } catch (error) { + done(error); + return; } + const result = spawnSync("node", searchArgs, { stdio: "inherit" }); if (result.status !== 0) { - done(new Error("coverage_search failed")); + done( + new Error( + result.error + ? `coverage_search failed: ${result.error.message}` + : `coverage_search failed (exit code ${result.status})` + ) + ); return; } done(); From c7dc7d7dd02dfa7413a53d6abb38e58477375ed6 Mon Sep 17 00:00:00 2001 From: calixteman Date: Sun, 19 Jul 2026 16:13:20 +0200 Subject: [PATCH 2/3] Sanitize the cached ETag and index to satisfy CodeQL Address two CodeQL findings on the per-test index downloader: - "File data in outbound network request": validate the cached ETag against the RFC 7232 grammar before sending it as If-None-Match, so the cache file's contents can't be injected into the request header. - "Network data written to file": cache the re-serialized JSON (JSON.stringify(JSON.parse(...))) instead of the raw response body, so only well-formed JSON produced by our own serializer is written. --- external/ccov/coverage_search.mjs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/external/ccov/coverage_search.mjs b/external/ccov/coverage_search.mjs index ba4e3547b..ed3519e81 100644 --- a/external/ccov/coverage_search.mjs +++ b/external/ccov/coverage_search.mjs @@ -111,7 +111,9 @@ async function refreshIndex() { const headers = {}; if (hasCached && fs.existsSync(etagPath)) { const etag = fs.readFileSync(etagPath, "utf8").trim(); - if (etag) { + // Only forward a syntactically valid HTTP ETag (RFC 7232), so the cached + // file's contents can't be used to inject arbitrary data into the request. + if (/^(?:W\/)?"[\x21\x23-\x7e]*"$/.test(etag)) { headers["If-None-Match"] = etag; } } @@ -134,17 +136,20 @@ async function refreshIndex() { return; } - let data; + let text; try { - data = Buffer.from(await response.arrayBuffer()); + text = await response.text(); } catch (error) { fallbackOrFail(error.message); return; } - // Validate the payload before caching it. + // Parse the payload before caching it, and cache the re-serialized result + // rather than the raw response body: only well-formed JSON produced by our + // own JSON.stringify is ever written to disk. + let serialized; try { - JSON.parse(data.toString("utf8")); + serialized = JSON.stringify(JSON.parse(text)); } catch { fallbackOrFail("the downloaded index is not valid JSON"); return; @@ -154,7 +159,7 @@ async function refreshIndex() { try { fs.mkdirSync(path.dirname(indexPath), { recursive: true }); const tmpPath = `${indexPath}.${process.pid}.tmp`; - fs.writeFileSync(tmpPath, data); + fs.writeFileSync(tmpPath, serialized); fs.renameSync(tmpPath, indexPath); const etag = response.headers.get("etag"); @@ -169,7 +174,7 @@ async function refreshIndex() { fallbackOrFail(error.message); return; } - console.error(`Per-test index updated (${data.length} bytes).`); + console.error(`Per-test index updated (${serialized.length} bytes).`); } await refreshIndex(); From e6c7ab5425c6836250d478f77b12b9ae1403b245 Mon Sep 17 00:00:00 2001 From: calixteman Date: Sun, 19 Jul 2026 16:42:05 +0200 Subject: [PATCH 3/3] Skip the cache existence check when --no-download is set Move the `fs.existsSync(indexPath)` check below the `--no-download` early return: in that mode the check is unused (the read after refreshIndex already validates existence), so it's wasted disk I/O. --- external/ccov/coverage_search.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/external/ccov/coverage_search.mjs b/external/ccov/coverage_search.mjs index ed3519e81..e4f4536ec 100644 --- a/external/ccov/coverage_search.mjs +++ b/external/ccov/coverage_search.mjs @@ -89,12 +89,12 @@ const etagPath = `${indexPath}.etag`; // only when it has changed since the last run. When the network is unavailable // a previously cached copy is reused if present. async function refreshIndex() { - const hasCached = fs.existsSync(indexPath); - if (values["no-download"]) { return; // Freshness check disabled; the read below validates existence. } + const hasCached = fs.existsSync(indexPath); + // On any download failure, fall back to a previously cached copy when one // exists; otherwise there's nothing to search, so fail. const fallbackOrFail = reason => {