RS-21803: Row Spans test - #68
Open
SurreyHughesDisplayr wants to merge 5 commits into
Open
Conversation
…class sharing Add test_that blocks for CreateCustomTable's row.spans argument: default no-op, per-span rowspan cells with correct heights/labels/row offsets, per-span class appending, styling arguments reaching the generated CSS, the extra header cell, and unvalidated mismatched-height behaviour. Two production behaviours diverge from the plan's assumptions and are pinned with comments rather than fixed: addCSSclass only creates a single shared "rowspandefault1" class (not one per span) when sticky positioning is inactive because the CSS string passed in is scalar, not vectorised to row.spans length; and adding row.spans increases the sticky-position count by 1 rather than reducing it, since the rm.index pruning only trims the row-span column's own top.position vector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add num.header.rows = 2 case that actually exercises the rm.index
pruning branch (front-loaded vs back-loaded span heights), since the
existing nhr = 1 case is a no-op for that branch.
- Correct the "single shared CSS class" test: drop the false
production-bug claim (it's addCSSclass() only expanding class.css
into a matrix when `position` is non-NULL) and add a sticky-path
counterpart asserting each span gets its own class.
- Pin the single-span over-cover case's actual output (rowspan="10",
4 body rows) and add a multi-span over-cover case pinning the real
cbind() misalignment defect under expect_warning.
- Make the extra-header-cell corner assertions discriminating via
corner = "RH", and anchor the styling-passthrough assertions to the
.rowspandefault1{...} declaration body instead of the whole document.
- Fix a stale comment that didn't match its assertion's count.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Corrects several factually-wrong explanations in the row.spans test block: the shared-CSS-class test's row-header parenthetical (it does not always get a non-NULL position), the multi-span over-cover test's title/mechanism (last span is discarded, not misaligned; the warning fires because 4 is not a multiple of 12, not the reverse), and the single-span over-cover test's "single assignment" reasoning. Also strengthens assertions (per-span CSS class checks 1/2/4, no stray rowspan in later over-cover rows) and expresses the num.header.rows=2 sticky counts relative to noSpans instead of hardcoded 15/16/17. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CreateCustomTable throws "invalid argument to unary operator" when row.spans is supplied with every span height equal to 1, together with sticky positioning (row.height set AND num.header.rows set). rm.index is only populated for spans taller than one row, so it stays NULL, and the pruning expression top.position[-rm.index] becomes top.position[-NULL], which is an invalid unary operator argument in R. This is a genuine, unticketed production defect. The new test pins the crash and also asserts that the same all-height-1 spans without sticky args, and mixed heights (2,1,1) with sticky args, both succeed - so the test proves the crash is specific to the all-heights-1 + sticky combination, not row.spans in general. No production code is changed; the defect remains unfixed and merely documented. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Aug 21, 2026
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.
Summary
Adds unit test coverage for the
row.spansargument ofCreateCustomTable(R/createcustomtable.R).<td rowspan="N" class="rowspandefaultN">tags per span, plus span labels, heights and row offsets, as fully-anchored exact strings.position = NULLbypasses thenrow*ncolmatrix expansion inaddCSSclass), and with it on each span gets its own class —.rowspandefault1/2/3each once, no fourth.rm.indexpruning branch atnum.header.rows = 2, discriminating a front-loaded span (prunes,noSpans + 1) from a back-loaded one (does not prune,noSpans + 2). Verified by mutation: deleting the pruning line from production flips the front-loaded count and fails the test.Pinned production defects
Two tests deliberately pin current buggy behaviour and will fail loudly when fixed. Neither has a defect ticket.
1. Multi-span over-cover silently drops the last span. With heights
(1, 10, 3),row.span.htmlgrows to length 12 whilecbind()takes its row count fromcell.html(4). Because 4 is not a multiple of 12,cbindwarnsnumber of rows of result is not a multiple of vector length (arg 1)and the final span's cell is discarded entirely. The table itself is not misaligned — the oversizedrowspan="10"is emitted literally and the browser clamps it.2. All-height-1
row.spansplus sticky positioning crashes.rm.indexis only populated for spans taller than one row, so with every height equal to 1 it staysNULLand the pruning expression becomestop.position[-NULL], which errors:Trigger is specific — the test asserts all three cases: all heights 1 with
row.height+num.header.rowserrors; all heights 1 without the sticky arguments works; mixed heights(2, 1, 1)with the sticky arguments works.Note the test plan's expectations for two scenarios did not match the code and were corrected rather than forced: the plan expected N distinct CSS classes on the default path (actually one shared class, and not a defect), and expected the sticky count to decrease (it increases by one).
Jira: https://numbers.atlassian.net/browse/RS-21803
Stacked PR
Based on
RS-21803-spacercol(#67), notmaster— these plans all add tests totests/testthat/test-createcustomtable.Rand share its helpers. Merge order: #66, #67, then this. The diff against that base is this plan's work only.Test plan
118 passing, 0 failures, 0 warnings.
🤖 Generated with Claude Code