Add regression coverage for the known correctness defects - #91
Merged
Conversation
Grows the suite from 11 tests to 133. No library code changes — this establishes the safety net that the correctness fixes will be validated against, and pins the current behaviour of every defect first. Known-but-unfixed bugs are expressed with a KnownBug.StillBroken helper rather than committed red or skipped. The test asserts the CORRECT behaviour; while the bug exists the assertion fails and the test passes, and the moment the bug is fixed the test fails and asks to be promoted. CI therefore stays green and a red build always means a new regression, while every defect is committed as executable documentation that cannot rot the way a Skip or a comment would. Setting FASTEXCEL_KNOWNBUG_LOG records how each defect actually fails, so a test passing for the wrong reason is auditable. CI uploads it as an artifact. Every one was checked against its issue report; several reproduce the reported error verbatim. Covered: shared-string positional resolution and rich text (#87, #81, #10), string escaping (#76), culture-dependent numeric output (#55), unsupported write types (#77, #61), cell ordering (#72), empty-cell gaps (#88, #83, #78), non-cell row content (#22), defined-name scoping (#84, #49), stream and update lifecycle (#75, #69, #74), and worksheet part resolution (#82, PR #62). Three defects surfaced that were not previously on the tracker: - The whole defined-name API throws NullReferenceException unless a sheet is read first, because LoadDefinedNames uses Archive without calling PrepareArchive. This likely underlies #89 and #49. - Update silently discards changes to cells that already hold a value. Worksheet.Merge documents that "the parameter takes precedence", but MergeRows is invoked on the incoming worksheet with the existing rows as its argument, so Cell.Merge copies the old value over the new one. Additive updates work, which is why this went unnoticed. - Worksheet.Rows and Row.Cells are both lazy iterators over the open archive, so a Worksheet does not outlive the FastExcel instance that produced it even though nothing in the API signals that. Tests build synthetic packages via an XlsxBuilder helper so the exact spreadsheet XML under test is visible in the test itself, run the culture-sensitive cases under de-DE, ru-RU, fr-FR, tr-TR and ar-SA, and exercise column conversion across Excel's full 16,384-column range. Parallelisation is disabled because culture is thread-local. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HQc2D6EJmEgB7e97G9MGK5
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
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.
Follows #90. Grows the suite from 11 tests to 133. Still no library code changes — this builds the safety net the correctness fixes will be validated against, and pins the current behaviour of every defect before anything is touched.
How known bugs are committed
Most of these tests describe behaviour the library does not have yet. Committing them red would leave CI permanently failing and make a real regression indistinguishable from a backlog item;
Skipwould let them rot silently. So they assert the correct behaviour and wrap it:This is the expected-failure pattern (pytest's
xfailwith strict XPASS):CI stays green, a red build always means a new regression, and every defect is committed as executable documentation that can't drift. It also self-validates: a wrapped assertion that quietly starts passing cannot go unnoticed — that's how I caught myself over-claiming #74, which turns out to already behave correctly on the stream path and is now a plain assertion.
Auditing that tests fail for the right reason
The weakness of
xfailis that a test failing from a typo looks identical to one failing from the bug.FASTEXCEL_KNOWNBUG_LOGrecords how each defect actually fails; CI uploads it as an artifact. I checked all 34 entries against their issue reports, and several reproduce the reported error verbatim:KeyNotFoundException: The given key '3' was not presentYellow, actualYel— rich-text runs truncated<si><t>Hello_x0020_World</t></si>on disk1,2under de/ru/fr/tr,1٫2under ar-SA<v>True</v>and<v>05/06/2024 00:00:00</v>in numeric cells["A", "C", "B"]IOException: Entries cannot be opened multiple times in Update modeArgumentException: Update mode requires a stream with read, write, and seek capabilitiesalpha content, gotbeta contentArgumentNullExceptionfromRegex.Replaceon a null referenceThree defects that were not on the tracker
LoadDefinedNamesusesFastExcel.Archivewithout ever callingPrepareArchive, so on a fresh instance it's null and every lookup throwsNullReferenceException. This likely underlies [Bug] Opening a large XLSX **throws exception** when attempting to getLoadDefinedNames#89 (whose stack trace runs throughGetCellRangesByDefinedName) and Defined Names doesn't seem to work #49 ("Defined Names doesn't seem to work").Updatesilently discards changes to cells that already hold a value.Worksheet.Mergedocuments that "the parameter takes precedence", butMergeRowsis invoked on the incoming worksheet with the existing rows as its argument, soCell.Mergecopies the old value over the new one. Additive updates work fine, which is exactly why this went unnoticed — and it's a plausible cause of several "Update doesn't work" reports including Writing Excel fails to preserve template #71.Worksheet.RowsandRow.Cellsare both lazy iterators over the open archive, so aWorksheetdoes not outlive theFastExcelinstance that produced it. Nothing in the API signals this;.ToList()onRowsis not enough to make it safe.Approach
XlsxBuilderconstructs minimal valid packages in memory, so the exact spreadsheet XML under test is visible in the test itself rather than buried in an opaque binary fixture. That's what makes it possible to test a shared-string table containing duplicates, a row with gaps, or a sheet part not named after its position.FastExcel.Tests/README.mddocuments all of this for contributors.Verification
Next
With this in place the Phase 1 correctness fixes become safe: rewriting
SharedStringsto index positionally, replacingXmlConvert.EncodeNamewith real XML escaping, and making the writer culture-invariant and type-aware. Each fix will flip itsKnownBugtests into failures asking to be promoted, which is the signal that it worked.Generated by Claude Code