RS-21803: Spacer Col test

[SurreyHughesDisplayr]

Summary

Adds unit test coverage for the spacer.col argument of CreateCustomTable (R/createcustomtable.R).

• Pins the emitted class="spacer" header cell and the complete .spacer { … } CSS rule, including the use.predefined.css variant.
• Asserts the .spacer rule is emitted exactly once and is namespaced under the generated container class rather than defined globally. The container name is randomised per call, so its shape is matched rather than its value.
• Asserts the full index-shift behaviour: spacer.col indexes emission position, not data column, and each leading corner cell shifts it by one. corner.styles[1] is prepended once for show.row.headers and again for row.spans, so the offset is +0/+1/+2, not a fixed +1. All three are pinned against the same spacer.col = 2; in the +2 case index 2 lands on the second corner cell and no data column is marked at all.
• Asserts the spacer cell keeps its own column label rather than being blanked.
• Asserts the non-spacer header cells keep colheaderdefault1, pinned as exact strings.
• Asserts col.header.fill genuinely displaces background: transparent, so the two CSS assertions discriminate rather than coincide.

Pinned production defects — RS-23589

spacer.col is never validated at its point of use. Two tests pin the consequences, and both will fail loudly when the bounds check lands.

1. No bounds check. col.header.styles[spacer.col] <- "spacer" silently extends the vector with NA_character_, and the outcome then depends on whether the index happens to be a multiple of the header length:

spacer.col = 3 -> OK    (3 <th>, 0 NA)   <- in range
spacer.col = 4 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 5 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 6 -> OK    (6 <th>, 2 NA)
spacer.col = 7 -> ERROR: arguments cannot be recycled to the same length

At a multiple (6) it renders wrongly and silently — literal class="NA" cells, and 6 <th> for a 3-column header because the labels recycle too, re-emitting X and Y. Anywhere else it is a hard error from sprintf whose message never names spacer.col. The realistic off-by-one, ncols + 1, is the crash. Both modes are asserted, plus the in-range case, so the errors are attributable to the out-of-range value rather than to spacer.col generally.

2. Silently ignored when show.col.headers = FALSE. The whole spacer.col application sits inside the if (show.col.headers) block, so with the header row off the argument is dropped with no warning. The .spacer CSS rule is still emitted (the predefined block is unconditional), leaving a rule no cell uses — asserted alongside the negative so it fails because no cell was marked, not because the stylesheet went missing.

No production code is changed in this PR.

Jira: https://numbers.atlassian.net/browse/RS-21803

Base

Originally opened stacked on RS-21803-sigleadercircles (#66). #66 has since merged to master, and this PR is now based on master directly with no conflicts — the diff is this plan's work only. No merge ordering is required any more.

Test plan

Rscript -e "devtools::load_all('.'); testthat::test_file('tests/testthat/test-createcustomtable.R')"

88 passing, 0 failures, 0 warnings (24 test_that blocks; 12 added here).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds unit coverage for CreateCustomTable’s spacer.col header and CSS behavior.

Changes:

• Tests spacer placement, labels, classes, and row-header offset.
• Covers CSS options and current out-of-range behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chschan]

Three notes on the new spacer.col coverage.

[chschan]

Coverage gap: spacer.col is never exercised together with show.col.headers = FALSE.

The whole spacer.col application sits inside the if (show.col.headers) branch (R/createcustomtable.R:592-593), so the argument is silently dropped when the header row is off. Verified against master:

h <- CreateCustomTable(m4, show.col.headers = FALSE, spacer.col = 2)$x$text
grepl('class="spacer"', h, fixed = TRUE)   # FALSE

Both arguments are covered separately (show.col.headers = FALSE at L246) but never in combination, so this silent-ignore path stays unpinned. One more case would lock in whichever behaviour is intended here.

[SurreyHughesDisplayr]

Confirmed and covered. Reproduced exactly as you describe — no class="spacer" cell is emitted.

Added "spacer.col is silently ignored when show.col.headers is FALSE", which pins the drop as silent (expect_warning(..., NA)) rather than merely absent, so adding validation later has to be a deliberate decision.

One extra thing the repro turned up, worth having in the same test: the .spacer CSS rule is still emitted, because the predefined CSS block is unconditional. So the stylesheet carries a rule no cell uses. I assert that alongside the negative, which keeps the negative honest — it now fails because no cell was marked, not because the stylesheet went missing. A flip-check with headers on confirms the same call does mark a cell.

Filed as RS-23589 together with the bounds-check defect, since both come down to spacer.col not being validated at its point of use; the ticket asks explicitly whether this path should warn.

[chschan]

The comment calls this "the index-shift contract", but the shift isn't a stable +1 — it depends on two unrelated arguments.

corner.styles[1] is prepended once for show.row.headers (R/createcustomtable.R:583) and again for row.spans (:589), so the offset is +0/+1/+2. Verified against master:

CreateCustomTable(m4, show.row.headers = TRUE,
                  row.spans = list(list(height = 3, label = "G")),
                  spacer.col = 2)
# <th class="cornerdefault1"></th> | <th class="spacer"></th> | <th class="colheaderdefault1 ">W</th>

With row.spans also set, index 2 lands on the second corner cell (empty label) and all four data columns are untouched — the opposite of what this test pins. The assertion is correct for the case it covers; it's the wording that will mislead the next reader. Suggest scoping the comment to show.row.headers alone, or adding the row.spans case alongside it.

[SurreyHughesDisplayr]

You are right, and the wording was the misleading part — the assertion itself was fine. Verified all three offsets against the same spacer.col = 2:

neither                     -> <th class="spacer">X</th>        (+0, second data column)
show.row.headers            -> <th class="spacer">W</th>        (+1, first data column)
show.row.headers + row.spans -> <th class="spacer"></th>        (+2, second CORNER cell)

Took the second of your two suggestions rather than just narrowing the comment, since the +2 case is the one that actually surprises: index 2 lands on the second corner cell and all four data columns are left untouched, so the spacer marks no data column at all. Narrowing the wording would have documented the trap without pinning it.

Renamed to "Each leading corner cell shifts the spacer.col index by one" and all three offsets are now asserted in one test, with the comment stating that corner.styles[1] is prepended once at :583 and again at :589, so the offset is +0/+1/+2 and not a fixed +1.

Note the fourth combination — row.spans with show.row.headers = FALSE — is not included because it errors on object 'corner.styles' not found. That is RS-23585, pinned separately in #68.

[chschan]

This pins the benign value and leaves the realistic one uncovered.

spacer.col = 6 survives sprintf() recycling precisely because it's a multiple of the 3-column header vector. The off-by-one someone would actually hit — ncols + 1 — is a hard error. Verified against master:

spacer.col = 4 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 5 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 6 -> OK

Since the fix is going onto the follow-up ticket rather than this PR, pinning the crash is worth as much as pinning the class="NA" output:

expect_error(CreateCustomTable(x2local, show.row.headers = FALSE, spacer.col = 4))

[SurreyHughesDisplayr]

Agreed — pinning the benign value alone undersold the defect. Reproduced your table exactly:

spacer.col = 3 -> OK    (3 <th>, 0 NA)   <- in range
spacer.col = 4 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 5 -> ERROR: arguments cannot be recycled to the same length
spacer.col = 6 -> OK    (6 <th>, 2 NA)
spacer.col = 7 -> ERROR: arguments cannot be recycled to the same length

Added the crash assertions to the same test, anchored on the message so an unrelated future error cannot satisfy them:

for (k in c(4, 5, 7))
    expect_error(CreateCustomTable(x2local, show.row.headers = FALSE, spacer.col = k),
        "arguments cannot be recycled to the same length", fixed = TRUE, info = k)

expect_error(CreateCustomTable(x2local, show.row.headers = FALSE, spacer.col = 3), NA)

I extended your suggestion in two small ways. 5 and 7 are included so the pin is "every out-of-range value that is not a multiple of the header length", rather than one sample that a future partial fix could special-case. And 3 is asserted to succeed, so the errors are attributable to the out-of-range value rather than to spacer.col generally — without it the block would still pass if spacer.col broke outright.

On the follow-up ticket: there was not one. Now filed as RS-23589, covering both failure modes, the k = 3..7 repro, the show.col.headers silent-ignore path from your other comment, and a note that these pinned assertions flip when the bounds check lands. Both test comments reference it.

[chschan]

LGTM