test(documents.js): work toward 100% mutation score - #1272
Draft
Mearman wants to merge 79 commits into
Draft
Conversation
The 200,000-synthetic-row-divider test in lattice.test.ts completes in well under a second uncontended, but under Stryker's per-statement instrumentation combined with heavy concurrent host load it can exceed vitest's default 5000ms wall-clock timeout despite doing the same real work. Raise the timeout to 60 seconds, matching the same wall-clock-dominated-by-scheduling pattern already documented for read-graph.test.ts's docxToPdf conversion and its sibling ODS mergeCells test.
…and header labels
bridges.test.ts's ods<->xlsx round-trip test read only each cell's numeric/typed
`.value`, never the source ODS fixture's own rendered `.displayText`
(buildRichFixturePackage's per-cell text:p run) or the two unchecked header labels
("Amount", "Active") -- so a header cell or a cell's rendered text could silently
go blank without any assertion catching it.
Also remove gridOdsPackage/richOdsPackage/decoratedOdsPackage from test-support/ods.ts:
dead exports with no caller anywhere in the suite (only their *Bytes counterparts are
ever used), each one a whole function body with no test coverage.
embeddedHsqldbMultiIndexOdbPackage (odb.ts), odfFormulaPackage (odf.ts), sheetFormulaOdsPackage (ods-formula.ts), and pdfWithForeignHiddenAnnotationPdf (pdf.ts) had no caller anywhere in the suite -- each one a whole function body (plus, for the first three, a now-unused Package/decodePackage import) that existed purely as NoCoverage mutation surface with nothing exercising it. pdfWithForeignHiddenAnnotationPdf's own comment ties it to a readPageNotes /T-marker check in pdf/read.ts, but no such file or function exists anywhere in this package's current src -- the reader it was meant to exercise appears to have moved or been removed elsewhere, leaving this fixture orphaned.
…pdate/clear The borders getter (top/left/bottom/right resolution, nil/none exclusion, auto-color and missing-sz fallbacks, and the empty-map-to-undefined collapse) was only ever exercised indirectly through a docx-odt bridge round trip that reads back via ooxml.js's own separate reader, never through this getter itself. Add direct tests for the getter's full edge/style/color/width matrix, the nil/none exclusion path, the auto-color and missing-attribute fallbacks, and the all-edges-excluded case. Also cover DocxTableRow.heightPt being updated and cleared on a row that already carries a w:trHeight, which the existing round-trip test never exercised since it only ever set the value once.
…, background, and borders Each of DocxTableCell's colSpan/background/borders setters filters out any prior element of the same kind before inserting the new one, but every existing test only ever set each property once starting from an empty w:tcPr, so that filter's own predicate never ran against a real element -- setting a value while one already exists was untested.
createEmptyDocxPackage's own tests only checked that the right parts and elements existed, never the actual namespace URIs, content-type strings, relationship targets, page/margin dimensions, or style attributes createEmptyDocxPackage hardcodes -- so a corrupted namespace, content type, or page dimension would still pass every existing assertion. Add exact-value checks for every literal: the version/encoding/standalone declaration on every part, both Default extensions and Override content types in [Content_Types].xml, both relationship parts' Id/Type/Target, the US-Letter w:sectPr's pgSz/pgMar values, and the Normal style's type/id/name.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+299
to
+301
| cell.borders = { | ||
| top: { color: { r: 1, g: 0, b: 0 }, widthPt: 2, style: "dashed" }, | ||
| }; |
…ight's no-op branch Both helpers stamp DEFAULT_COLUMN_WIDTH_PT/DEFAULT_ROW_HEIGHT_PT only when the column/row has no width/height style yet, but no existing test ever called cell() (which triggers them) on a column/row that already had an explicit width/height set -- so the "already set, leave it alone" branch never actually ran with a real value to compare against.
…ts gridlines/headers parsing
readSheetPrintSettings/writeSheetPrintSettings had no dedicated test
at all; every existing test only ever set printSettings.gridlines
through a single shallow property check elsewhere, leaving pageSize,
margins, pageOrder, printRange, scalePercent, fitToPages, manualBreaks,
and repeatColumns/repeatRows entirely unexercised. Add direct coverage
for each field's round trip through the real OdsSheet.printSettings
getter/setter, the wrapRepeatRange gap-fill and stale-wrapper
dissolution behaviour, hasManualBreak's own break-detection, and
parsePrintRanges/parseScalePercentage/parseNonNegativeInteger's
malformed-input handling (poking the underlying XML directly for
shapes the writer itself never produces).
Also simplify the gridlines/headers reader: the previous
`new Set(...).filter((token) => token.length > 0)` guarded against
empty tokens from stray whitespace, but the only two things ever read
from that set are `.has("grid")`/`.has("headers")`, which an empty
token can never satisfy either way -- the filter's outcome was
unobservable through any real behaviour. Replaced with a plain
`.split(" ").includes(...)` check, which is equally robust to stray
whitespace without carrying an untestable branch.
| for (let column = 0; column < 6; column++) { | ||
| sheet.cell(0, column).value = { kind: "number", value: column }; | ||
| } | ||
| sheet.printSettings = { ...BASE, repeatColumns: { start: 0, end: 1 } }; |
createEmptyOdsPackage had no dedicated test at all -- every existing test only exercised it indirectly through createOds(), checking structural existence (a table exists, a cell can be written) without ever asserting the actual namespace URIs, the of: namespace LibreOffice needs to recalculate table:formula on open, media type, page-layout geometry, calculation-settings defaults, or office:meta field mapping this scaffold hardcodes. Add exact-value checks for the mimetype and manifest root entry, every part's declaration, content.xml's of: namespace and default sheet/style chain, styles.xml's page-layout/ master-page chain, and every office:meta field buildOfficeMeta writes (and omits) for a given LayoutMetadata.
… the ods scaffold test tsconfig.node.json's stricter typecheck (run separately from the default tsconfig by the pre-push hook) caught what tsconfig.json's own run missed: Array.prototype.find can return undefined, and attr() requires a real XmlElement.
…back in detectPackageManager userAgent ?? "" only mattered for the startsWith checks below it, and none of yarn/pnpm/bun's prefixes match an empty string either, so the fallback literal's own value was unobservable and the branch always returned npm regardless. Replace it with an explicit early return for the undefined case so the mutation-prone fallback literal no longer exists as an AST node.
…ectly No test in the package called applyOdfGeometry directly; its 33 covering tests all went through higher-level editors that never distinguished rotationDeg undefined from rotationDeg 0, so the combined "undefined || === 0" guard survived a mutation to a bare false. Add direct unit coverage for all three branches (undefined, exactly 0, and a real rotation) plus a sanity check on buildTransformAttr's own composition.
UnsupportedFontSourceFormatError, UnsupportedConversionError, and UnsupportedPackageFormatError were only ever checked by instanceof/toThrow(), so each constructor's own this.name assignment had no test observing its value and survived a mutation to an empty string.
…ion guard Neither constructor guard's error message had a test calling it with a wrong-kind ContentDocument, so both throw strings survived being mutated to an empty template literal.
…te string The presentation-slide-shape test only checked the recovered formula, never the locate path built from the slide/shape indices, so that template literal survived being mutated to an empty string.
…g count No test read sheet.rows at all, so table.rows.length + 1 (one sizing entry per data row plus the header row) survived being mutated to - 1.
The "single-length level whose slot is undefined" branch had no covering test at all (not reachable through any real odf.js-decoded report), so its error message survived being mutated to an empty string. Exercise it directly with a hand-built groups array carrying an explicit hole.
The only existing test's container held a single element that always matched regardless of the tag check, so node.tag === tag survived being mutated to true. Add a container whose one element has a different tag.
… option forwarding
Neither the markdown reader's images/signal object nor readDocumentLayout's
signal object had a test observing an effect from either field, so both
ObjectLiteral mutations to {} survived. Assert the images resolver is
actually invoked and that an aborted signal is checked before parsing (the
markdown reader's own signal check, and readPdf's own pre-page-loop check).
Also assert throwIfAborted's DOMException carries the exact name/message
rather than only its type.
…uards xls.content.write's non-spreadsheet refusal message and pdf.layout.write's signal forwarding to writePdf had no test observing either, so a StringLiteral mutation to the error message and an ObjectLiteral mutation of the options object both survived.
inferCellValue("") already returns undefined on its own (its own
text.length === 0 check), so field === "" ? undefined : inferCellValue(field)
restated that in a second place for no observable difference -- an empty
field's own type-inference path was unkillable by any test because both
branches always produced the same undefined. Call inferCellValue(field)
directly, and assert onCellTypeInference fires only for the populated
fields in a row that also carries empty ones.
Mirrors the identical CsvInvalidUtf8Error gap: only instanceof/toThrow(Class) was checked, so the constructor's own name/message string literals survived being mutated to empty strings.
…case Number.prototype.toFixed already normalizes -0 to "0.000000" on its own, so the trailing-zero trim below collapses any zero-valued coordinate (positive or negative) to a bare "0" through the exact same path every other value takes -- the early return produced no output any test could ever distinguish from the general path, for any input.
…no fonts No test called treeEmbeddedFontsOf directly, so its "return undefined rather than an empty array" branch (the whole point of the function per its own comment: a splice site can spread undefined without minting an empty table) had no test observing the difference between the two.
…-placeholder branches The existing table test's nested cell content never needed transforming, so recursing markdownTableCell into it versus not (a BlockStatement mutation skipping the whole branch) produced identical output. Add a case with a pageBreak nested in a cell, which only survives as marker text if the recursion genuinely ran. Also cover formulaParagraph's own placeholder-run branch (no presentation LaTeX), whose ArrayDeclaration mutation to an empty runs array had no test asserting the placeholder text reached the output.
…sting odf.js's own single shared paintOrder counter guarantees a shape and a vector can never collide on the same value, which made the "order < vectorOrder" boundary check unkillable through any real odf.js-decoded package -- the "<=" mutant produces identical grouping for every reachable input. Split the pure grouping algorithm out of collectSlideVectorGroups so it takes plain paintOrder-bearing data directly, and test the boundary (and the paintOrderOf throw guard) against a hand-built collision no real package can produce but the function's own contract still needs to hold for.
…e guard short-circuits Every legacy reader independently rejects non-CFB bytes too (each one's own first step is archive-codec's readCompoundFile), so the outcome alone (undefined either way) could never distinguish the outer isCompoundFile guard existing from it being skipped. Spy on readDocContent to prove it is never even invoked for bytes that fail the guard, rather than merely happening to fall through all three readers to the identical result.
…med-package guards readPptxContent's own upstream flat reader has no slides to map over in either malformed shape (a missing ppt/presentation.xml part, or one with no p:sldIdLst), so nothing reaching slidePathsInOrder through readPptxContent could ever observe which of its two possible empty-array returns actually came back -- both ArrayDeclaration mutations to a placeholder array survived. Export the function so its own two guards are directly testable against hand-built packages.
addCoreProperties' keywords branch was only ever exercised with a non-empty array or a fully-absent field; nothing pinned the empty-array case, which must also omit the element per the function's own contract.
…ot-found paths Only the append-relative-to-last-element and prepend cases were tested; neither the mid-container insertion point nor the reference-sibling-absent fallback (append rather than misplace at the start) had a dedicated test.
… structure Nothing exercised this function in isolation; its p:nvSpPr wrapper (tag names, the cNvPr id/name attributes, the sibling p:cNvSpPr/p:nvPr pair) was only reachable indirectly through a full slide-building test.
…teness, and boundary cases Whitespace-trimming before both regexes, the exponent-overflow finiteness guard, and the split(/\s+/) collapse of consecutive whitespace were never exercised, and the negative-height check had no case distinguishing zero (legal) from negative (rejected) at the boundary itself.
…uceRational Neither function had any test at all despite being the shared exact-rational arithmetic behind decimal-literal lowering and the coherence lint's own comparison normalisation.
…rimitives attrValue, elementChildren, firstChildByLocalName, and textContent had no test at all; each one's own predicate (attribute name equality, local-name equality, the non-element/non-text fallthrough) was only ever exercised indirectly through the layout engine's end-to-end tests.
…p guard tablesInOrder held names looked back up in the schema map to build each table, but schema.set(relation.name, relation) always runs in the same statement that pushes the name, so the lookup could never fail. Track the relation objects themselves in creation order instead, removing the never-true undefined guard entirely rather than leaving it untestable. Also adds direct coverage for readBurpHeader's att_backup_format/compress guards, relationToColumns' computed-field filter, and the two unsupported nested/top-level record error messages.
…st-removal guard Nothing exercised remove() itself (splicing the page out of the drawing, marking the handle removed) or the resulting live()/removed throw on any further use -- removePageAt on the editor manipulates the tree directly rather than going through an OdgPage instance's own remove().
Every existing case had at least one path segment before the file name on both sides; nothing exercised dirSegments' own no-slash-found branch for the source part.
Replaces the if-guarded max = index assignment with Math.max, matching the odf-package/media.ts and opc/media.ts convention added earlier in this same mutation-coverage effort -- every Object N directory carries a distinct index, so the if-comparison's own tie boundary was unobservable, and the function had no test at all until now.
…venodd fill-rule Neither buildSvgText diagnostic nor written attribute had a dedicated test: a shape with no name and no sourcePath still needs a non-empty label, and a path vector's own fill-rule="evenodd" attribute was only ever exercised through the read side, never confirmed on write.
…property access diagnostics[0]?.detail.startsWith(...) only guards the .detail access, not the subsequent .startsWith call on a value TypeScript still sees as string | undefined under tsconfig.node.json's stricter settings -- assert against .detail directly with toMatch instead.
Mirrors the existing markdown/PDF cases: readRtfContent checks
options.signal before tokenizing, so an already-aborted signal must
reach it through the { signal } options object rather than an empty
one.
…mutant
Slicing from indexOf(":") + 1 already returns the whole string when
there is no colon (indexOf yields -1, so the slice starts at 0), so
the colonIndex === -1 ternary guard was redundant: every input was
already handled correctly by the slice alone. That redundancy made a
ConditionalExpression mutation on the guard permanently equivalent
(unkillable by any test), since the branch it toggled produced the
same output either way. Dropping the guard removes the mutation
opportunity outright.
reduceRational(0n, 0n) exercises gcd's own 0/0 special case (defined
as 1 to avoid a BigInt division-by-zero crash) directly, rather than
only through a path decimalToRational can actually reach.
decimalToRational("-5") pins that the ^\d+$ digits check rejects a
literal whose only non-digit character sits before its digits, not
just one with non-digit characters at the end (already covered by the
existing "12a" case).
The existing test only exercised title/author being kept when overrides omits them; subject and keywords had the identical conditional-spread shape with no coverage of their own absent-field case.
…ent mutants compareCellKeys checked equality first, then used a nested less-than ternary for the remainder. Since the equality guard already rules out left === right before that nested comparison runs, a < -to- <= mutation on it produced identical output for every reachable input: the equal case never reaches that branch, and < versus <= only differ on equality. Reordering to check < (then >, then implicit equal) puts the equal-values input back in reach of that comparison, so a relational-operator mutation there is observable again.
The existing "outside Pictures/" case used a path whose Pictures/- prefix-length slice happens not to look like an image filename, so it could not distinguish a real prefix check from one that never actually skipped. Adds a path deliberately crafted so slicing at the Pictures/ prefix length spells a valid image filename by coincidence, pinning that the function only follows the real path prefix.
The early return for an empty vector-groups array only skipped allocating a shapes array that the rebuild loop below would otherwise reconstruct with identical contents (the insertion while-loop never runs when groups is empty, so every shapeIndex iteration just re-pushes the shape already at that index). The guard changed nothing observable, making its ConditionalExpression mutation permanently equivalent. Removing it drops the mutation opportunity.
The presence loop matched on element tag AND attribute value, but no test distinguished that from matching on the attribute value alone. Adds a case where an Override element carries an Extension attribute equal to the value being ensured, confirming ensureDefaultContentType still adds a genuine Default entry rather than mistaking the Override for one.
escapeRegExp's replacement text turning a special character into a literal match, rather than deleting it outright, needs a part whose extension contains one to distinguish -- the existing "p.g" case only proved over-matching was prevented, not that a genuine "p+g" match still works. Also mirrors src/odf-package/media.test.ts's own prefix-length coincidence case for this sibling OOXML-side implementation.
…r guard siblingRank !== -1 was always true by the time siblingRank > childRank runs, since -1 can never be greater than childRank (already known non-negative from the earlier childRank === -1 return above) -- the guard changed nothing observable, an equivalent mutant. Also adds insertBefore's own not-found-append-at-end case (insertAfter already had one) and a same-schema-rank case for insertInSchemaOrder, both previously unexercised.
Indexing a string past its end yields undefined in JavaScript, and undefined === " " is already false, so the i + runLength < text.length check could never change the loop's outcome on its own -- it only ever agreed with what the character comparison already decided, making three separate mutations on it (the bound itself, its operator, and its arithmetic) all permanently equivalent. Relying on the character comparison alone removes the redundant check.
… content None of content.xml/styles.xml/meta.xml's own root element tag was directly asserted -- every existing check reached into a child by tag name, which would still succeed even with an empty root tag. Also pins that CONTENT_NS_PREFIXES is genuinely spread into content.xml's attributes (xmlns:table specifically, distinct from the hand-declared xmlns:of already checked) and that styles.xml carries its own empty office:styles sibling.
…kers MATH_INLINE_SOURCE, MATH_INLINE_FONT_MARKER, and MATH_BLOCK_STYLE_ID had no test exercising formulaParagraph at all: a formula whose provenance source matches the inline marker renders as a \( \) span, and any other source renders as a $$ display block instead.
…utes Neither the XML declaration node nor the four xmlns attributes on cp:coreProperties were directly asserted anywhere -- every existing check reached into a child by tag name or attribute, which would still succeed with an empty declaration/attributes object.
Two independently-ANDed length checks (common < fromDirs.length && common < toDirs.length) meant relaxing either one in isolation never changed the loop's outcome: the sibling, still-correct check kept stopping the loop at the same common, and wherever the two paths' directories genuinely differ, fromDirs[common] === toDirs[common] already fails once one side runs out (a real segment can never equal undefined). That made both length checks, their operators, and the && joining them all permanently equivalent mutation targets. A single combined bound has no sibling clause left to mask a boundary mutation, observable via two identically-deep directory chains. Also drops relsPathFor's own redundant lastSlash === -1 guard for the filename split, the same "slicing from -1 + 1 already returns the whole string" equivalence already fixed in src/mathml/nodes.ts's localName.
…etry passthrough paragraphs() had no test proving it excludes non-paragraph blocks (a table specifically) from the fixture it already round-trips. startList's own task default had no case exercising the unset (false) path, only the explicit task: true one. Neither constructor guard (non-wordprocessing kind, an empty sections array) nor the pageSize/margins/clock passthrough into createMarkdownEditor had any coverage at all.
…line stamping readDocumentMetadata and readNativeDocumentTree had no coverage at all for abort-signal forwarding on either their pdf or non-pdf dispatch branch, nor for the markdown images resolver readNativeDocumentTree also forwards. The stampPdfPackageTables call had no test able to distinguish it running from being skipped either: the existing pdf fixture (docxToPdf of a plain docx) carries no outline or destinations of its own, so stamping is a no-op either way for that input. Building a small PDF directly through pdf-codec's own writePdf with a bare outline entry, bypassing every documents.js writer, gives the call something to actually stamp.
…message shapes The odf -> pdf special case checked source.format === "odf" && targetFormat === "pdf", but nothing exercised the targetFormat half: an odf source to any OTHER target had no test, so a request that should reject as unsupported (odf has no composition-engine route except to pdf) could have silently succeeded through odfToPdf instead. Also adds direct coverage for the char/substituted diagnostic's exact message text, the bold/italic weight suffixes in a font-substitution message, odfToPdf's own onDocument/signal forwarding, and that the odf->pdf route's options object genuinely reaches the call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Working through survived/no-coverage mutants in packages/documents.js toward a genuine 100% mutation score, with no Stryker disable comments anywhere.
Progress tracked in commits as it lands. Will mark ready once the mutation score is 100% with the break threshold raised accordingly.