RS-21803: checkImageTag style attribute test

[SurreyHughesDisplayr]

Summary

Adds unit test coverage for checkImageTag's handling of the style attribute and its surrounding link-cleaning paths (R/checkimagetag.R) — 7 test blocks in the existing test-checkimagetag.R.

• Covers a style attribute before and after src, exercising the two different quote/bracket cleaning paths rather than the same one twice.
• Covers double-quoted and unquoted src values, and src followed by trailing text.
• Covers both syntax-error branches with one input each: an <img ...> that matches the tag regex but has no src=, and an input with no closing > that misses the tag regex first. The two warnings are byte-identical (each echoes the whole input), so no assertion on the message can tell them apart — separate inputs are what pins the branches.
• Asserts the success path wraps the original text, not the matched img tag.
• Each success block asserts the whole returned string in one expect_equal rather than several substring greps.

Also closes a pre-existing gap: the file's txt.dq case was single-quoted and identical to txt.sq, so double quotes were never actually tested. The style literal is now a single hoisted constant, replacing a dangling unused assignment.

Note on the src= regex

The last block pins current behaviour for a decoy input where an earlier attribute value contains src=. regexpr("src=(\S+)", ...) is unanchored, so alt='mysrc=http://x.invalid/' is matched in preference to the real src, and a valid image is silently removed with a user-visible invalid link which has been removed: http://x.invalid/ warning. The warning is anchored on the extracted link, so the assertion pins the decoy rather than merely a failed GET, and the decoy uses the RFC 2606 reserved .invalid TLD so it is inert regardless of what the network's DNS does.

This looks like a genuine low-severity defect rather than intended behaviour: nothing in the function or its callers suggests the loose match is deliberate, and a boundary requirement such as (?:^|\s)src= would be safe because the enclosing <img [^>]+> match guarantees whitespace before every attribute. Not fixed here — filed as RS-23603 (Bug, Severity D - Minor), which the test comment references. Separately, the trailing-whitespace sub() in the same function is unreachable, since (\S+) cannot capture whitespace — inert rather than wrong.

Network dependency

The success-path blocks perform a live HTTP GET of a real image URL — the pre-existing convention in this file, not introduced here. Rather than failing offline, every block that needs a link now guards on skipIfOffline(), which probes the exact links the block fetches (kImageLink, and kDropboxLink for the first block) once per link per run.

testthat::skip_if_offline() is deliberately not used: it calls skip_on_cran(), which would skip these blocks in any run where NOT_CRAN is unset, network or no. Verified by pointing both link constants at an unreachable host — 6 skips, 6 passes, 0 failures.

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

Stacked PR

Based on RS-21803-colwidthsandcustomcss (#71). Merge order: #66, #67, #68, #69, #70, #71, then this. This is the last of the seven RS-21803 plans; it is the only one touching checkimagetag.R rather than createcustomtable.R, so it shares no test file with the others.

Test plan

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

28 passing, 0 failures, 0 warnings with network up; 6 passing and 6 skipped without it.

🤖 Generated with Claude Code

[chschan]

Reviewed the diff and ran it: 24 passed / 0 failed / 0 skipped here with network up, so the test-plan claim holds. DESCRIPTION correctly stays at 1.11.11 for a tests-only change, and the txt.dq fix is real — double quotes were genuinely never exercised before, and nchar == 84 still holds since both quote characters are one byte. No caller behaviour changes.

Approving with comments. Three worth doing before merge:

1. The PR description is stale relative to the head commit. The "Network dependency" section says the blocks "fail offline. Documented in a comment at the top of the file" — but 3b8025a added skipIfOffline(), so the behaviour is now skip, not fail. Please update the body.
2. The offline guard is incomplete and the header NOTE overstates it (inline at line 7).
3. The decoy test's failure mode depends on DNS resolution (inline at line 117).

The rest are cleanups.

---

One minor note that can't be inlined (lines 43-47 fall between the two diff hunks, so GitHub won't take a comment there):

Unlike every other branch asserted in this file, the empty-link path never checks the return value - there's no res <- and no expect_equal. Worth pinning, because that branch returns via sub(imgtag, "", text) rather than the bare return("") the two syntax-error branches use, and the difference only shows up with surrounding text:

expect_warning(res <- checkImageTag("<img src=''>Some text"),
               "an image tag with an empty link")
expect_equal(res, "Some text")

Pre-existing and entirely optional for this PR.

[chschan]

This overstates what's guarded. The test_that("checkImageTag") block below is also network-dependent and has no skipIfOffline() — line 57 fetches exactly kImageLink, and lines 36/40 fetch a Dropbox URL.

Simulating an offline run (stubbed failing GET()): the five new blocks skip cleanly, but that block produces 3 hard failures — nchar(res) 0 vs 84 twice, plus the txt.withattr equality. So the file still fails offline rather than skipping.

Either guard that block too, or narrow this wording to name the blocks it actually covers.

(The skip_if_offline() / skip_on_cran() rationale in the second paragraph is a good catch and worth keeping.)

[SurreyHughesDisplayr]

Right — guarded that block rather than narrowing the wording (29bfde7). The probe now caches per link, skipIfOffline() takes the links a block needs, and the first block guards on c(kImageLink, kDropboxLink). Header NOTE rewritten to say what is actually covered, including that the empty-link assertions need no network but skip with the block they live in. Simulated offline (both constants pointed at an unreachable host): 6 skips, 6 passes, 0 failures — previously that block gave 3 hard failures.

[chschan]

This is the unguarded one. It fetches the same kImageLink URL the probe already checks, so skipIfOffline() here is free and correct.

The Dropbox assertions at lines 36/40 need their own probe, or the header NOTE should say the file isn't fully offline-safe.

[SurreyHughesDisplayr]

Fixed in 29bfde7 — that block now calls skipIfOffline(c(kImageLink, kDropboxLink)), and the Dropbox URL is a kDropboxLink constant the probe checks alongside the wiki image, so neither needs its own wording caveat. Its two nchar(res) == 84 assertions are now full equality against paste0("<div>", txt, "</div>"), which both subsumes the byte count and stops the expected value drifting if the constant changes.

[chschan]

The name overstates what the function does — the decoy block at line 108 demonstrates it is not order-independent once an earlier attribute value contains src=.

Suggest "src is found when it is the first attribute".

[SurreyHughesDisplayr]

Renamed to "checkImageTag: src is found when it is the first attribute" in 29bfde7, with a comment pointing at the decoy block as the counter-example.

[chschan]

txt.order here (and txt.dq2 at line 79, txt.unquoted at line 87) hard-code the wiki URL as a literal, while the probe and kTxtStyle use kImageLink.

The guard is then correct only by coincidence of duplicated literals: change kImageLink (new host, moved image) and the probe reports "reachable" while these three blocks still GET the stale URL — turning the intended skip back into a hard failure, which is the exact failure mode this PR set out to remove.

paste0("<img src='", kImageLink, "' style='margin:0 auto;' width='50'>") etc.

[SurreyHughesDisplayr]

Good catch — all duplicated literals are gone in 29bfde7. txt.order, txt.dq2, txt.unquoted, txt.decoy and the txt.withattr fixture in the first block are now built with paste0(..., kImageLink, ...), so the probe and the requests can no longer diverge. The Dropbox URL got the same treatment via kDropboxLink.

[chschan]

Inconsistent absence-assertion across structurally identical blocks: line 64 uses expect_warning(..., NA), but this line (and 80, 88, 104) uses only expect_error(..., NA), which says nothing about warnings.

A regression that makes checkImageTag warn-and-strip is pinned as a clear failure in the first block, but surfaces in these as a testthat warning plus a downstream equality failure — which reads like a network problem rather than a behaviour change.

expect_warning(..., NA) uniformly.

[SurreyHughesDisplayr]

Done in 29bfde7 — every success-path call now uses expect_warning(..., NA), including the two in the first block that previously only used expect_error(..., NA). An error still fails the test, so nothing is lost by dropping the error form.

[chschan]

The PR description says this pins the echoed input "so it is distinguishable from the tag-regex-miss branch that emits a byte-identical syntax error prefix". It doesn't — both branches in checkimagetag.R warn with "...syntax error which has been removed: ", text, i.e. the same text, so for this input the two messages are byte-identical:

"<img style=1"                            -> syntax error which has been removed: <img style=1
"<img style='margin:0 auto;' width='50'>" -> syntax error which has been removed: <img style='margin:0 auto;' width='50'>

It happens not to matter today, since this input does match <img [^>]+> and so can only reach the src= branch. But if the tag regex is later loosened or tightened so this input stops matching, the test keeps passing while silently covering the other branch.

Distinguishing them needs two inputs — one that fails the tag regex, one that fails only src= — not a pinned echo. Either add the second input or drop the claim from the description.

[SurreyHughesDisplayr]

Correct — the echo distinguishes nothing, since both branches warn with the same text. Took the "add the second input" option in 29bfde7: the block is now "both syntax-error branches warn and return an empty string" and covers one input per branch — the existing <img style='margin:0 auto;' width='50'> matches <img [^>]+> and so can only reach the src= branch, and "<img style='margin:0 auto;' width='50'" (no closing >) misses the tag regex first. The comment states outright that no assertion on the warning can tell them apart. The stale claim is also removed from the PR description.

[chschan]

This assertion depends on DNS luck. The extracted link is the bare string x, and the block only passes because httr::GET("x") fails to resolve it. On a network with a DNS search suffix that makes a single-label x resolvable, or behind a proxy/captive portal that answers arbitrary hosts with 200, checkImageTag takes the success path, emits no warning, and this fails with a misleading "no warning" message rather than a pinned behaviour change.

Use an RFC 2606 reserved TLD so it's inert regardless of network:

txt.decoy <- paste0("<img alt='mysrc=http://x.invalid/' src='", kImageLink, "'>")
expect_warning(res <- checkImageTag(txt.decoy),
               "invalid link which has been removed: http://x\.invalid/$")

Note the escaped . — expect_warning matches its second argument as a regex.

(Confirmed RS-23603 exists and its summary matches this comment, so the ticket reference is sound.)

[SurreyHughesDisplayr]

Took your suggestion verbatim in 29bfde7, escaped dot included: txt.decoy <- paste0("<img alt='mysrc=http://x.invalid/' src='", kImageLink, "'>") with the warning anchored on "invalid link which has been removed: http://x\.invalid/$". Added a note that the block deliberately carries no skipIfOffline() — the decoy is matched first, so kImageLink is never fetched and the block is genuinely network-free. Confirmed in the simulated-offline run: it passes there.

[chschan]

Some minor comments, but otherwise LGTM

[SurreyHughesDisplayr]

All seven inline comments plus the un-inlinable note are addressed in 29bfde7; PR description updated for the three stale claims (the "fails offline" wording, the syntax-error distinguishability claim, and the missing RS-23603 reference).

The one non-obvious call: I guarded the first test_that block rather than narrowing the header NOTE. That means its two GET-free assertions (txt.empty, txt.empty2) now skip offline along with the rest of the block, since testthat skips whole blocks. Splitting them into a separate block would keep them running, but restructures pre-existing tests for little gain — the header NOTE says so explicitly.

Offline verification (both link constants pointed at an unreachable host): 6 skips, 6 passes, 0 failures — previously the first block gave 3 hard failures. With network: 28 passing, 0 failures, 0 warnings.