diff --git a/scripts/validate-odf.mjs b/scripts/validate-odf.mjs index 7911c99..e02589f 100644 --- a/scripts/validate-odf.mjs +++ b/scripts/validate-odf.mjs @@ -39,7 +39,8 @@ import { mkdtempSync, writeFileSync, mkdirSync } from "node:fs" import { execFileSync } from "node:child_process" import { tmpdir } from "node:os" -import { join, dirname } from "node:path" +import { join, dirname, resolve } from "node:path" +import { pathToFileURL } from "node:url" const args = new Map() for (let i = 2; i < process.argv.length; i += 2) { @@ -48,7 +49,13 @@ for (let i = 2; i < process.argv.length; i += 2) { const schema = args.get("schema") const manifestSchema = args.get("manifest-schema") const jing = args.get("jing") -const dist = args.get("dist") ?? new URL("../dist/index.mjs", import.meta.url).href +// A URL rather than the path as typed: `extract` resolves the ZIP reader +// relative to it, so `--dist` points at one bundle instead of the bundle +// for the library and whatever happens to sit in ../dist for the reader. +const distArg = args.get("dist") +const dist = distArg + ? pathToFileURL(resolve(distArg)).href + : new URL("../dist/index.mjs", import.meta.url).href if (!schema || !jing) { console.error("usage: node scripts/validate-odf.mjs --schema --jing ") @@ -168,9 +175,7 @@ const PARTS = ["content.xml", "styles.xml", "meta.xml", "settings.xml"] /** Pull one entry out of an ODS with hucre's own ZIP reader. */ async function extract(bytes, path) { - const { ZipReader } = await import(new URL("../dist/zip/reader.mjs", import.meta.url).href).catch( - () => ({}), - ) + const { ZipReader } = await import(new URL("zip/reader.mjs", dist).href).catch(() => ({})) if (ZipReader) return new TextDecoder().decode(await new ZipReader(bytes).extract(path)) // The bundle does not export it; read the workbook back through the // public API is not enough, so fall back to `unzip -p`. @@ -194,19 +199,34 @@ function validate(file, rng) { const work = mkdtempSync(join(tmpdir(), "hucre-odf-val-")) let failures = 0 +let skipped = 0 for (const [label, bytes] of await documents()) { console.log(`\n${label} (${bytes.length} bytes)`) - const checks = [...PARTS.map((p) => [p, schema])] - if (manifestSchema) checks.push(["META-INF/manifest.xml", manifestSchema]) + const checks = PARTS.map((p) => [p, schema]) + if (manifestSchema) { + checks.push(["META-INF/manifest.xml", manifestSchema]) + } else { + // A check that did not run is not a check that passed. Saying so is + // the difference between "valid" and "nobody looked" (#566). + skipped++ + console.log(` ${"META-INF/manifest.xml".padEnd(24)} SKIPPED (no manifest schema supplied)`) + } for (const [part, rng] of checks) { let xml try { xml = await extract(bytes, part) } catch { - console.log(` ${part.padEnd(24)} (absent)`) + // Every part in PARTS is written by all three ODS writers, so one + // that will not come out of the package is a defect rather than an + // absence — and a package that cannot be opened at all lands every + // part here. Counting these as checks that passed is what let the + // script print "All parts valid." for a document it never read + // (#566). + failures++ + console.log(` ${part.padEnd(24)} MISSING/UNREADABLE`) continue } @@ -225,5 +245,11 @@ for (const [label, bytes] of await documents()) { } } -console.log(failures === 0 ? "\nAll parts valid." : `\n${failures} invalid part(s).`) +console.log( + failures > 0 + ? `\n${failures} invalid or unreadable part(s).` + : skipped > 0 + ? `\nAll checked parts valid; ${skipped} manifest check(s) skipped (no manifest schema supplied).` + : "\nAll parts valid.", +) process.exit(failures === 0 ? 0 : 1) diff --git a/test/odf-validation.test.ts b/test/odf-validation.test.ts new file mode 100644 index 0000000..19ab691 --- /dev/null +++ b/test/odf-validation.test.ts @@ -0,0 +1,128 @@ +import { afterEach, describe, expect, it } from "vitest" +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs" +import { tmpdir } from "node:os" +import { delimiter, join } from "node:path" +import { fileURLToPath } from "node:url" +import { spawnSync } from "node:child_process" +import type { SpawnSyncReturns } from "node:child_process" + +// The validator is a developer script, not library code: it needs `jing`, +// a 40 MB OASIS grammar and a JVM, so the suite cannot run it for real. +// What it can run is the script's own bookkeeping — which parts counted +// as checked — against stub `unzip` and `java` on PATH and a stub bundle +// behind `--dist`. That is the part #566 got wrong. + +let work: string | undefined + +afterEach(() => { + if (work) rmSync(work, { recursive: true, force: true }) + work = undefined +}) + +/** The line the script prints for one part, in its own column layout. */ +function line(part: string, status: string): string { + return ` ${part.padEnd(24)} ${status}` +} + +function runValidator(unzipMode: "missing-settings" | "unreadable"): SpawnSyncReturns { + work = mkdtempSync(join(tmpdir(), "hucre-odf-validation-test-")) + const bin = join(work, "bin") + mkdirSync(bin) + + // A stub for every export `documents()` reaches for. Add to the script + // and this list grows with it — the alternative is building dist to + // test the counting. + const dist = join(work, "fake-dist.mjs") + writeFileSync( + dist, + `export async function writeOds() { return new Uint8Array([1]) } +export function writeOdsStream() { + let read = false + return { getReader: () => ({ async read() { + if (read) return { done: true } + read = true + return { done: false, value: new Uint8Array([1]) } + } }) } +} +export class OdsStreamWriter { + addRow() {} + async finish() { return new Uint8Array([1]) } +} +export function createCellStore() { return {} } +export function setCell() {} +`, + ) + + // `unzip -p `: one part refuses to come out, or the whole + // package does. + writeFileSync( + join(bin, "unzip"), + `#!/bin/sh +if [ "$FAKE_UNZIP_MODE" = "missing-settings" ] && [ "$3" != "settings.xml" ]; then + printf '' + exit 0 +fi +exit 1 +`, + { mode: 0o755 }, + ) + // jing, accepting everything it is handed. + writeFileSync(join(bin, "java"), "#!/bin/sh\nexit 0\n", { mode: 0o755 }) + + return spawnSync( + process.execPath, + [ + fileURLToPath(new URL("../scripts/validate-odf.mjs", import.meta.url)), + "--schema", + "unused.rng", + "--jing", + "unused.jar", + "--dist", + dist, + ], + { + cwd: work, + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}${delimiter}${process.env["PATH"]}`, + FAKE_UNZIP_MODE: unzipMode, + }, + }, + ) +} + +describe("ODF validator CLI", () => { + it("fails when a required package part is missing (#566)", () => { + const result = runValidator("missing-settings") + + expect(result.status).toBe(1) + expect(result.stdout).toContain(line("settings.xml", "MISSING/UNREADABLE")) + // The parts that did come out are still reported as checked, so the + // failure is the missing one rather than a package-wide collapse. + expect(result.stdout).toContain(line("content.xml", "valid")) + expect(result.stdout).toContain(line("styles.xml", "valid")) + expect(result.stdout).toContain(line("meta.xml", "valid")) + // One part × the three documents the script writes. + expect(result.stdout).toContain("3 invalid or unreadable part(s).") + expect(result.stdout).not.toContain("All parts valid.") + }) + + it("fails when the package cannot be read at all (#566)", () => { + const result = runValidator("unreadable") + + expect(result.status).toBe(1) + expect(result.stdout).toContain(line("content.xml", "MISSING/UNREADABLE")) + expect(result.stdout).not.toContain(line("content.xml", "valid")) + // Four parts × three documents, none of them checked. + expect(result.stdout).toContain("12 invalid or unreadable part(s).") + }) + + it("names the manifest check as skipped when no schema is supplied (#566)", () => { + const result = runValidator("unreadable") + + expect(result.stdout).toContain( + line("META-INF/manifest.xml", "SKIPPED (no manifest schema supplied)"), + ) + }) +}) diff --git a/tsconfig.cli.json b/tsconfig.cli.json index 282ec60..88cedea 100644 --- a/tsconfig.cli.json +++ b/tsconfig.cli.json @@ -22,6 +22,7 @@ "test/cli.test.ts", "test/threaded-comments-write.test.ts", "test/ods-fidelity.test.ts", + "test/odf-validation.test.ts", "test/parity-statement.test.ts", "test/real-files.test.ts", "test/ods-third-party.test.ts", diff --git a/tsconfig.json b/tsconfig.json index c3bd9b4..b8f1d58 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -42,6 +42,9 @@ // reason the file-reading tests above are here. See #474. "test/_reflect.ts", "test/reflect.test.ts", + // Spawns `scripts/validate-odf.mjs` with stub tools on PATH, so it + // needs `node:child_process` and `node:fs`. + "test/odf-validation.test.ts", // Reads the fixture corpus off disk, so it needs `node:fs`. "test/third-party-fixtures.test.ts", "test/ods-third-party.test.ts",