Skip to content

One unloadable test file no longer takes down the whole run #184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DmitrySharabin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

One unloadable test file no longer takes down the whole run #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 48 additions & 33 deletions src/env/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,40 @@ const filenamePatterns = {
exclude: /^index/,
};

/**
* A test that reports why its file could not be loaded, so one unloadable file
* fails on its own instead of taking down the whole run.
* @param {Error} err
* @param {string} name
*/
function failedTest (err, name) {
return {
name,
run: () => {
throw err;
},
};
}

/**
* Import a test file, or produce a failing test in its place if it cannot be loaded.
* @param {URL} url
* @param {string} base - Directory to label the file against, the same one loaded files are labeled against.
*/
async function importTests (url, base) {
// Before the import, not after: a file that fails to load must still get its directory watched,
// or fixing it is never noticed. Explicit rather than via the resolve hook, which needs Node ≥ 22.15.
loadedFiles.add(url.href);

try {
let module = await import(url);
return module.default ?? Object.values(module);
}
catch (err) {
return failedTest(err, path.relative(base, fileURLToPath(url)));
}
}

async function getTestsIn (dir) {
let filenames = fs
.readdirSync(dir)
Expand All @@ -34,20 +68,7 @@ async function getTestsIn (dir) {
let cwd = process.cwd();
let paths = filenames.map(name => path.resolve(cwd, dir, name));

return (
await Promise.all(
paths.map(path => {
path = pathToFileURL(path);
loadedFiles.add(path.href);
return import(path).then(
module => module.default ?? Object.values(module),
err => {
console.error(`Error importing tests from ${path}:`, err);
},
);
}),
)
).flat();
return (await Promise.all(paths.map(path => importTests(pathToFileURL(path), dir)))).flat();
}

// AbortSignal (not a plain boolean) because runAll() shallow-copies options per child —
Expand Down Expand Up @@ -183,17 +204,18 @@ async function rerun (options, urls) {
continue;
}

// Import before mutating stats — on error, keep the old subtree intact
let uncached = new URL(url);
uncached.searchParams.set("htest", version);

let module;
let test;
try {
module = await import(uncached.href);
let module = await import(uncached.href);
test = module.default ?? Object.values(module);
}
catch (err) {
console.error(`Error importing ${url}:`, err);
continue;
// Swap in a failing test rather than keeping the stale passing one,
// which would leave a file that just broke showing green.
test = failedTest(err, old.test.file.label);
}

// Subtract old stats, swap in the new subtree
Expand All @@ -206,8 +228,6 @@ async function rerun (options, urls) {
(currentRoot.timeTakenAsync ?? 0) - old.timeTakenAsync;
}

let test = module.default ?? Object.values(module);

if (Object.isExtensible(test)) {
test.file = old.test.file;
}
Expand Down Expand Up @@ -319,6 +339,7 @@ export default {

let tests;
let isDirectory = fs.statSync(location, { throwIfNoEntry: false })?.isDirectory();
let base = isDirectory ? location : path.dirname(location);
if (isDirectory) {
// Directory provided, fetch all files
tests = await getTestsIn(location);
Expand All @@ -329,12 +350,7 @@ export default {
let modules = globSync(location).flatMap(paths => {
// Convert paths to imported modules
paths = getType(paths) === "string" ? [paths] : paths;
return paths.map(p => {
p = path.resolve(process.cwd(), p);
p = pathToFileURL(p);
loadedFiles.add(p.href);
return import(p).then(m => m.default ?? Object.values(m));
});
return paths.map(p => importTests(pathToFileURL(path.resolve(process.cwd(), p)), base));
});
tests = (await Promise.all(modules)).flat();
}
Expand All @@ -344,16 +360,15 @@ export default {
// Tag each module's default with its source file. Re-imports return the cached namespace — no I/O.
await Promise.all(
[...loadedFiles].map(async url => {
let module = await import(url);
let test = module.default ?? module;
// loadedFiles holds transitive imports too, so a failure here was already surfaced —
// by this file's own failing test, or by the test file that imported it.
let module = await import(url).catch(() => null);
let test = module?.default ?? module;
if (test && typeof test === "object" && Object.isExtensible(test) && !test.file) {
let fileUrl = new URL(url);
fileUrl.search = "";
test.file = {
label: path.relative(
isDirectory ? location : path.dirname(location),
fileURLToPath(url),
),
label: path.relative(base, fileURLToPath(url)),
path: fileUrl.href,
};
}
Expand Down
51 changes: 51 additions & 0 deletions tests/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,33 @@ import Test from "../src/classes/Test.js";
import TestResult from "../src/classes/TestResult.js";
import BubblingEventTarget from "../src/classes/BubblingEventTarget.js";

// In run() rather than beforeAll(): a throwing beforeAll skips its tests, and a skipped test
// cannot fail CI. Memoized because both assertions read one child run.
let unloadable;
function runUnloadable () {
return (unloadable ??= (async () => {
let { spawnSync } = await import("node:child_process");
let { mkdtempSync, writeFileSync, rmSync } = await import("node:fs");
let { tmpdir } = await import("node:os");
let { join } = await import("node:path");

// ESM, because a CJS syntax error escapes as an unhandled rejection Node never hands us
let dir = mkdtempSync(join(tmpdir(), "htest-unloadable-"));
writeFileSync(join(dir, "package.json"), `{ "type": "module" }`);
writeFileSync(join(dir, "good.js"), `export default { tests: [{ run: () => 1, expect: 1 }] };`);
writeFileSync(join(dir, "broken.js"), `import missing from "./nope.js";\nexport default { tests: [] };`);

let result = spawnSync(
process.execPath,
[join(process.cwd(), "bin/htest.js"), dir, "--ci"],
{ encoding: "utf8" },
);
rmSync(dir, { recursive: true });

return result;
})());
}

export default {
name: "Run tests",
tests: [
Expand Down Expand Up @@ -80,6 +107,30 @@ export default {
},
expect: true,
},
{
name: "A test file that cannot be imported",
description: "It fails on its own instead of taking down the run, so healthy siblings still report.",
skip: typeof globalThis.process === "undefined",
async run () {
let { stdout, stderr } = await runUnloadable();

// A failing run prints its tree to stderr, a crashing one its stack — assert across both.
return stdout + stderr;
},
check: (actual, expect) => actual.includes(expect),
tests: [
{
name: "Still runs the healthy files, and counts the broken one",
description: "The denominator matters: dropping the broken file would report 1/1 and look fine.",
expect: "1/2 PASS",
},
{
name: "Names the file that failed to load",
description: "Matching the row, not the output: the error text below it mentions the path either way.",
expect: "FAIL broken.js",
},
],
},
{
name: "Aborting",
tests: [
Expand Down