RS-21803: Col Widths and Custom CSS test

[SurreyHughesDisplayr]

Summary

Adds unit test coverage for col.widths and custom CSS in CreateCustomTable (R/createcustomtable.R) — 13 test blocks.

• Pins the emitted <col> tags exactly, including the literal surrounding spaces that paste(..., sep = " ") produces: <col width=' 200px '>. Percentage widths are pinned as an explicit vector so the assertion fails if col.widths is ignored and the default 25% is emitted instead.
• Covers col.widths counting the row-header column, fewer widths than columns, and more widths than columns (7 widths against a 5-column render, asserting the surplus is emitted untruncated).
• col.widths.fill.container on and off, with the extracted <table> tag length-guarded so an empty match cannot pass vacuously.
• Asserts the calc() table-width offset tracks cell.border.width, pinned at a non-default c(5, 2): the width side takes max() = 5 and the height side rev()[1] = 2, so each assertion fails if the two expressions are swapped. A scalar could not tell them apart.
• Covers the scroll half of the wrapper switch: a row.height (y scroll) or enable.x.scroll switches the widget from boxIframeless to a plain Box, and the x case also pins the emitted div#outer-table-container overflow rule. The custom.css half is already covered by the pre-existing iframes test.
• Covers override.borders, including the case that pins its substring match as genuinely unanchored — an unrelated .x:nth-child(2) rule alongside border-top suppresses the border. This is the argument's intended opt-out escape hatch, not a defect.

Every argument under test uses a distinctive non-default value, so no assertion can pass while the argument is ignored.

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

Stacked PR

Was stacked on RS-21803-headervisibilityandcellformatting (#70), which has since merged, so the base is now master. Earlier plans in the stack (#66, #67, #68, #69, #70) are all merged; the diff here is this plan's work only.

Test plan

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

252 passing, 0 failures, 0 warnings.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds targeted CreateCustomTable tests for column widths, custom CSS, border overrides, and widget hosting behavior.

Changes:

• Tests column-width parsing, defaults, overflow, and container sizing.
• Tests custom CSS emission and border overrides.
• Tests iframe-less versus scrolling widget hosts.

---

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

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tests/testthat/test-createcustomtable.R:1066

• The test claims that both tokens are required, but its only false case contains border without nth-child. If the implementation dropped the border predicate and keyed solely on nth-child, every assertion here would still pass. Add the reciprocal nth-child-only control.

    cssBorderOnly <- "table.mycustom { border-top: 4px solid rgb(9,9,9); }"

tests/testthat/test-createcustomtable.R:992

• This negative assertion only excludes the exact 5px value, so the test still passes if disabling col.widths.fill.container incorrectly emits another width:calc(...) value. Assert that the table tag contains no calculated width at all to pin the behavior named by the test.

    expect_false(grepl("width:calc(100% - 5px)", table, fixed = TRUE))

[SurreyHughesDisplayr]

Both suppressed comments from the latest Copilot review are addressed in 22ec6e8.

col.widths.fill.container = FALSE (was line 992) — the negative assertion excluded only the exact 5px value the TRUE case emits, so a wrongly-emitted width of some other value would have passed. It now asserts the <table> tag carries no width:calc( at all. The height-side calc() positive control is unchanged.

override.borders (was line 1066) — added the reciprocal control: custom.css containing nth-child with the literal substring border absent anywhere leaves the border declaration in place. With the existing border-only case, the pair now pins that both tokens are required; keying on either one alone satisfies one case but not the other.

Both verified by mutation against R/createcustomtable.R:

• dropping the border predicate (override.borders <- grepl("nth-child", custom.css, fixed = TRUE)) fails only the new nth-child-only assertion;
• emitting a different width when the argument is FALSE (else "width:calc(100% - 1px)") fails only the new width:calc( assertion.

Source reverted after each. Suite: 248 passing, 0 failures, 0 warnings.

[chschan]

Test-only PR, suite is green (FAIL 0 | WARN 0 | SKIP 0 | PASS 248). Most of the new coverage is load-bearing — the override.borders block pins both tokens, and the col.widths.fill.container TRUE/FALSE pair is not vacuous. Three assertions are weaker than their comments claim; details inline. (Separately, a few of these tests brush past real product defects in R/createcustomtable.R — those will go in their own PR rather than here.)

[chschan]

This comment claims cell.border.width = 5 pins the offset as coming from the width-side calc rather than the height-side one, but a scalar can't distinguish them. Width uses max(0, max(cell.border.width)) (R/createcustomtable.R:653), height uses rev(cell.border.width)[1] (line 651) — with a scalar both render 5px, so the assertion passes whichever mechanism produced it.

cell.border.width = c(5, 2) emits width:calc(100% - 5px); height:calc(100% - 2px), which does pin it: a refactor swapping max() for rev()[1] (or vice versa) would then fail. Worth changing the FALSE case at L988 to the same vector so the height:calc(100% - 2px) positive control at L997 stays aligned, and updating the comment to say what it's actually distinguishing.

[SurreyHughesDisplayr]

Confirmed and fixed in 2c05983. With a scalar both sides render 5px, so neither assertion pinned its own mechanism. Both cases now use cell.border.width = c(5, 2): the TRUE case asserts width:calc(100% - 5px) (max) and height:calc(100% - 2px) (rev[1]), the FALSE case keeps the aligned height:calc(100% - 2px) control. Mutation-checked — setting table.width.offset <- rev(cell.border.width)[1] now fails the width assertion, which it did not before.

[chschan]

Only the vector leg is new. The test at L940 already pins tagsString against this same literal vector for "20%, 30%, 50%", so expect_equal(tagsVector, <literal>) plus expect_equal(tagsString, tagsVector) just re-asserts it.

Dropping resString/hString/tagsString (L950, L952-953, L957) leaves expect_equal(tagsVector, c(...)), which is the whole point of the test and still fails if vector input stops being handled equivalently.

[SurreyHughesDisplayr]

Right — dropped the string leg in 2c05983. The block is now just the vector call asserted against the literal, with a comment noting the literal is the one the comma-separated test above pins, which is what makes it an equivalence rather than a standalone assertion.

[chschan]

This block has no failure mode:

• L1041 — mytesttoken is a string invented by the test, so no regression in custom.css handling can make it appear in a default render. expect_false can't fail.
• L1040 — duplicates the pre-existing iframes test (L22-23), which asserts the same can-run-in-root-dom TRUE on a default render (different fixture, same assertion) and also already covers the custom.css → NULL half.
• L1045-1046 — duplicates the test at L1030, which already pins that supplied custom.css text reaches the HTML verbatim.

Suggest dropping the whole test_that. If the intent was to pin that the default custom.css = "" emits no user rule at all, assert against the actual markup (e.g. no extra rule beyond the generated .celldefault*/th/table set) rather than against a token the test made up.

[SurreyHughesDisplayr]

Agreed on all three points — the whole test_that is deleted in 2c05983. mytesttoken is the test's own invention so the expect_false had no failure mode, and the other two halves were already covered by the iframes test and by the verbatim-custom.css test immediately above. I did not add the stronger "no rule beyond the generated set" version here: it would pin the whole predefined-CSS block, which is a separate concern from this plan's arguments.

[chschan]

Two coverage gaps, both cheap to close from tests already in this diff.

[chschan]

override.borders gates six emission sites, and this PR covers three: celldefault (R/createcustomtable.R:473, previous test), rowheaderdefault (line 493) and colheaderdefault (line 558). Untested: rowspandefault (line 527), cornerdefault (line 574), colspandefault (line 606).

cornerdefault is the cheap one — it's emitted for this exact fixture (x2 has both dimnames, and the block at line 570 only requires show.row.headers), so it's one more regmatches in this test:

res <- CreateCustomTable(x2, custom.css = cssBoth, col.header.border.width = 4, col.header.border.color = "red",
            row.header.border.width = 4, row.header.border.color = "blue",
            corner.border.width = 4, corner.border.color = "green")
...
cornerRule <- regmatches(h, regexpr("\.cornerdefault1\{[^}]*\}", h))
expect_length(cornerRule, 1)
expect_false(grepl("border: 4px solid green", cornerRule, fixed = TRUE))

plus the matching positive control against resNoCss at L1111. The two span rules need a spanned fixture, so those are reasonable to leave — but worth naming them in the test title (or a comment) so this doesn't read as covering everything override.borders touches.

[SurreyHughesDisplayr]

Added in 2c05983 — corner.border.width = 4, corner.border.color = "green" on both the custom.css call and the resNoCss positive control, with the cornerdefault1 rule asserted both ways. Confirmed against the emitted CSS: suppressed with cssBoth, border: 4px solid green present without it. Title now names all three rules, and a comment states that rowspandefault and colspandefault are the two gated sites this file does not cover, since they need a spanned fixture.

[chschan]

This reaches enable.scroll only through the y-side (row.height → enable.y.scroll), so the enable.x.scroll || term in enable.scroll <- enable.x.scroll || enable.y.scroll (R/createcustomtable.R:634) is untested — deleting that term still passes the whole suite. It would break two things at once: the wrong host is chosen at line 708, and the div#outer-table-container overflow rule (lines 640-641) is never emitted.

One line closes it:

expect_equal(attr(CreateCustomTable(x2, enable.x.scroll = TRUE), "can-run-in-root-dom"), NULL)

Asserting overflow-x: auto in the emitted div#outer-table-container rule pins it more precisely, if you want the stronger version.

[SurreyHughesDisplayr]

Took the stronger version in 2c05983: enable.x.scroll = TRUE asserts the NULL host attribute, plus the emitted div#outer-table-container rule containing overflow-x: auto and overflow-y: hidden — the y half pins which axis the argument actually drove. Mutation-checked: enable.scroll <- enable.y.scroll (first term deleted) now fails 4 assertions; before this it passed the whole suite.

[chschan]

LGTM