From 88dbd9690e26b90c71c7e8fa74fa4481c18143de Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sun, 28 Jun 2026 14:12:54 +0000 Subject: [PATCH 1/2] Raise test coverage to 100% (mark unreachable defensive branches) Baseline R-side + compiled coverage was 99.45% (covr 3.6.5, type="tests"), with exactly three uncovered lines, all of which are unreachable defensive branches rather than untested logic: - R/thin.R:77 - the `is.array(image) && length(dim(image)) == 2L` recursion target in as_binary_matrix(). In R (>= 4.2, the package's floor) every object with a length-2 `dim` attribute also satisfies is.matrix(), so the is.matrix() branch above always handles 2-D arrays first. Verified by exhaustively tracing every array/data.frame/list-array input: none reach this line. - src/lee.cpp:42 - `default: on_boundary = 0;` for a `sub` value outside 0-3. `sub` is driven only by the internal `for (int sub = 0; sub < 4; sub++)` loop, so out-of-range values never occur. - src/distance_transform.cpp:235 - `default: Rcpp::stop(...)` for an unknown metric code. The R wrapper validates `metric` with match.arg() and maps it to codes 0/1/2 only, so an out-of-range code never reaches the C++. Per the skip-coverage policy these three (and only these) are wrapped in `# nocov` with a one-line category + reason comment. After the markers, non-nocov coverage is 100% on every R and C++ file. Also adds strict exact-value tests (the existing suite leaned on expect_lt/expect_gt bounds): - tests/testthat/test-coercion-helpers.R (new) - pins as_binary_matrix() foreground-collapse and dimension behaviour, the 2-D-array dispatch equivalence, the documented error messages, and restore_storage() round-trips, all with expect_identical. - exact skeletons for zhang_suen/guo_hall/thinImage in test-thin.R. - exact medial-axis skeleton + Euclidean distance map in test-medial-axis.R. - exact full-matrix manhattan/chessboard/euclidean DTs in test-distance-transform.R. devtools::test(): 0 failures. lintr::lint_package(): 0 lints. R CMD check --as-cran: 0 errors, 0 warnings, 1 NOTE (pre-existing non-portable compilation-flag note on Ubuntu). Co-Authored-By: Claude Opus 4.8 --- R/thin.R | 7 ++ src/distance_transform.cpp | 5 +- src/lee.cpp | 5 +- tests/testthat/test-coercion-helpers.R | 123 +++++++++++++++++++++++ tests/testthat/test-distance-transform.R | 22 ++++ tests/testthat/test-medial-axis.R | 25 +++++ tests/testthat/test-thin.R | 32 ++++++ 7 files changed, 217 insertions(+), 2 deletions(-) create mode 100644 tests/testthat/test-coercion-helpers.R diff --git a/R/thin.R b/R/thin.R index f14b175..b835db8 100644 --- a/R/thin.R +++ b/R/thin.R @@ -74,7 +74,14 @@ as_binary_matrix <- function(image) { storage.mode(image), "'.") } if (is.array(image) && length(dim(image)) == 2L) { + # nocov start + # Unreachable invariant guard: in R (>= 4.2) every object with a + # length-2 `dim` attribute also satisfies is.matrix(), so any 2-D + # array is already handled by the is.matrix() branch above. Kept as + # a defensive recursion target in case base R's array/matrix + # semantics ever diverge. return(as_binary_matrix(matrix(image, nrow = dim(image)[1], ncol = dim(image)[2]))) + # nocov end } stop("thinr::thin() expects a 2-D matrix. ", "Higher-dimensional arrays are not yet supported.") diff --git a/src/distance_transform.cpp b/src/distance_transform.cpp index 49edc44..8003236 100644 --- a/src/distance_transform.cpp +++ b/src/distance_transform.cpp @@ -232,7 +232,10 @@ NumericMatrix distance_transform_cpp(IntegerMatrix img, int metric) { case 0: return dt_euclidean(img); case 1: return dt_manhattan(img); case 2: return dt_chessboard(img); - default: Rcpp::stop("Unknown metric code passed to distance_transform_cpp."); + // Defensive default: the R wrapper distance_transform() validates + // `metric` with match.arg() and maps it to codes 0/1/2 only, so an + // out-of-range code never reaches here. Not exercisable from R. + default: Rcpp::stop("Unknown metric code passed to distance_transform_cpp."); // # nocov } return NumericMatrix(0, 0); // unreachable } diff --git a/src/lee.cpp b/src/lee.cpp index 8ff8894..a732d6a 100644 --- a/src/lee.cpp +++ b/src/lee.cpp @@ -39,7 +39,10 @@ static inline int lee_can_delete(int p2, int p3, int p4, int p5, case 1: on_boundary = (p4 == 0); break; // east case 2: on_boundary = (p6 == 0); break; // south case 3: on_boundary = (p8 == 0); break; // west - default: on_boundary = 0; + // Defensive default: `sub` is driven only by the internal + // `for (int sub = 0; sub < 4; sub++)` loop below, so values + // outside 0-3 never reach here. Not exercisable from R. + default: on_boundary = 0; // # nocov } if (!on_boundary) return 0; diff --git a/tests/testthat/test-coercion-helpers.R b/tests/testthat/test-coercion-helpers.R new file mode 100644 index 0000000..4bb8078 --- /dev/null +++ b/tests/testthat/test-coercion-helpers.R @@ -0,0 +1,123 @@ +# Strict exact-value tests for the internal coercion helpers +# (as_binary_matrix / restore_storage) and the array-input path of +# thin(). These pin the precise dispatch behaviour rather than just +# the output shape. + +describe("as_binary_matrix: collapses foreground to exactly 1", { + it("integer input keeps zeros and maps every non-zero to 1L", { + img <- matrix(c(0L, 2L, 0L, + 5L, 0L, 255L, + 0L, 9L, 0L), + nrow = 3, byrow = TRUE) + out <- thinr:::as_binary_matrix(img) + expect_identical( + out, + matrix(c(0L, 1L, 0L, + 1L, 0L, 1L, + 0L, 1L, 0L), + nrow = 3, byrow = TRUE) + ) + expect_type(out, "integer") + }) + + it("logical input becomes a 0/1 integer matrix", { + img <- matrix(c(TRUE, FALSE, FALSE, TRUE), nrow = 2) + out <- thinr:::as_binary_matrix(img) + expect_identical(out, matrix(c(1L, 0L, 0L, 1L), nrow = 2)) + expect_type(out, "integer") + }) + + it("numeric input maps any non-zero (incl. negatives/fractions) to 1", { + img <- matrix(c(0, -3.5, 0.2, 0), nrow = 2) + out <- thinr:::as_binary_matrix(img) + expect_identical(out, matrix(c(0L, 1L, 1L, 0L), nrow = 2)) + expect_type(out, "integer") + }) + + it("preserves matrix dimensions", { + img <- matrix(1L, nrow = 3, ncol = 7) + out <- thinr:::as_binary_matrix(img) + expect_identical(dim(out), c(3L, 7L)) + }) +}) + +describe("as_binary_matrix: array input dispatches through the matrix path", { + # A 2-D object built with array() is, in R (>= 4.2), already a matrix, + # so it is handled identically to the equivalent matrix() input. This + # pins that equivalence with exact values. + it("a 2-D integer array yields the same result as the matrix form", { + arr <- array(0L, dim = c(5L, 5L)) + arr[2:4, 2:4] <- 1L + mat <- matrix(0L, nrow = 5, ncol = 5) + mat[2:4, 2:4] <- 1L + expect_identical( + thinr:::as_binary_matrix(arr), + thinr:::as_binary_matrix(mat) + ) + }) + + it("thin() accepts a 2-D array and matches the matrix result exactly", { + arr <- array(0L, dim = c(5L, 5L)) + arr[2:4, 2:4] <- 1L + mat <- matrix(0L, nrow = 5, ncol = 5) + mat[2:4, 2:4] <- 1L + expect_identical( + thin(arr, method = "zhang_suen"), + thin(mat, method = "zhang_suen") + ) + }) +}) + +describe("as_binary_matrix: unsupported inputs error with the documented message", { + it("character matrix names the storage mode in the message", { + expect_error( + thinr:::as_binary_matrix(matrix("a", 2, 2)), + "does not know how to interpret a matrix of mode 'character'", + fixed = TRUE + ) + }) + + it("a 3-D array reports the 2-D requirement", { + expect_error( + thinr:::as_binary_matrix(array(0L, dim = c(2L, 2L, 2L))), + "expects a 2-D matrix" + ) + }) + + it("a 1-D array reports the 2-D requirement", { + expect_error( + thinr:::as_binary_matrix(array(0L, dim = 4L)), + "expects a 2-D matrix" + ) + }) +}) + +describe("restore_storage: returns the storage mode of the original input", { + skel <- matrix(c(0L, 1L, 0L, 1L), nrow = 2) + + it("logical original -> logical skeleton with identical values", { + out <- thinr:::restore_storage(skel, matrix(TRUE, 2, 2)) + expect_identical(out, matrix(c(FALSE, TRUE, FALSE, TRUE), nrow = 2)) + expect_type(out, "logical") + }) + + it("double original -> double skeleton with identical values", { + out <- thinr:::restore_storage(skel, matrix(1.0, 2, 2)) + expect_identical(out, matrix(c(0, 1, 0, 1), nrow = 2)) + expect_type(out, "double") + }) + + it("integer original -> the integer skeleton is returned unchanged", { + out <- thinr:::restore_storage(skel, matrix(1L, 2, 2)) + expect_identical(out, skel) + expect_type(out, "integer") + }) + + it("preserves the skeleton dimensions for every storage mode", { + sk <- matrix(0L, nrow = 4, ncol = 6) + expect_identical(dim(thinr:::restore_storage(sk, matrix(TRUE, 4, 6))), + c(4L, 6L)) + expect_identical(dim(thinr:::restore_storage(sk, matrix(1.0, 4, 6))), + c(4L, 6L)) + }) +}) diff --git a/tests/testthat/test-distance-transform.R b/tests/testthat/test-distance-transform.R index 9fc14fc..b650e60 100644 --- a/tests/testthat/test-distance-transform.R +++ b/tests/testthat/test-distance-transform.R @@ -62,6 +62,28 @@ describe("distance_transform: euclidean distance", { }) }) +describe("distance_transform: exact full-matrix values from a corner", { + # A 4x4 all-foreground image with a single background pixel at (1, 1). + # Pin every cell of every metric, not just a handful. + img <- matrix(1L, nrow = 4, ncol = 4) + img[1, 1] <- 0L + + it("manhattan: every cell equals (r-1) + (c-1)", { + expected <- outer(0:3, 0:3, `+`) + expect_equal(distance_transform(img, metric = "manhattan"), expected) + }) + + it("chessboard: every cell equals max(r-1, c-1)", { + expected <- outer(0:3, 0:3, pmax) + expect_equal(distance_transform(img, metric = "chessboard"), expected) + }) + + it("euclidean: every cell equals sqrt((r-1)^2 + (c-1)^2)", { + expected <- outer(0:3, 0:3, function(a, b) sqrt(a^2 + b^2)) + expect_equal(distance_transform(img, metric = "euclidean"), expected) + }) +}) + describe("distance_transform: all-background image returns zeros", { for (metric in c("euclidean", "manhattan", "chessboard")) { local({ diff --git a/tests/testthat/test-medial-axis.R b/tests/testthat/test-medial-axis.R index a774fa7..a34d9fe 100644 --- a/tests/testthat/test-medial-axis.R +++ b/tests/testthat/test-medial-axis.R @@ -50,6 +50,31 @@ describe("medial_axis: return_distance returns skeleton + distance", { }) }) +describe("medial_axis: exact skeleton and distance on a known rectangle", { + # The 7x9 solid 3x5 rectangle from the roxygen example. Pins the exact + # ridge pattern and the per-pixel distance values, not just a subset. + img <- matrix(0L, nrow = 7, ncol = 9) + img[3:5, 3:7] <- 1L + + it("returns the documented skeleton pattern", { + expected <- matrix(0L, nrow = 7, ncol = 9) + expected[3, 3:4] <- 1L + expected[3, 6:7] <- 1L + expected[4, 3:7] <- 1L + expected[5, 3:4] <- 1L + expected[5, 6:7] <- 1L + expect_identical(medial_axis(img), expected) + }) + + it("returns the exact Euclidean distance map", { + expected <- matrix(0, nrow = 7, ncol = 9) + expected[3, 3:7] <- 1 + expected[4, 3:7] <- c(1, 2, 2, 2, 1) + expected[5, 3:7] <- 1 + expect_equal(medial_axis(img, return_distance = TRUE)$distance, expected) + }) +}) + describe("medial_axis: storage mode of output skeleton matches input", { it("logical in, logical out", { img <- matrix(FALSE, nrow = 5, ncol = 5) diff --git a/tests/testthat/test-thin.R b/tests/testthat/test-thin.R index 1bc6147..3c5836f 100644 --- a/tests/testthat/test-thin.R +++ b/tests/testthat/test-thin.R @@ -369,6 +369,38 @@ describe("complex shapes do not crash and yield smaller skeletons", { } }) +describe("exact skeletons on small known shapes", { + # The property tests above bound the skeleton size; these pin the + # exact pixel pattern so a change in any algorithm's output is caught. + it("zhang_suen collapses a 3x3 solid block to its single centre pixel", { + img <- matrix(0L, nrow = 5, ncol = 5) + img[2:4, 2:4] <- 1L + expected <- matrix(0L, nrow = 5, ncol = 5) + expected[3, 3] <- 1L + expect_identical(thin(img, method = "zhang_suen"), expected) + }) + + it("guo_hall collapses a 3x3 solid block to its single centre pixel", { + img <- matrix(0L, nrow = 5, ncol = 5) + img[2:4, 2:4] <- 1L + expected <- matrix(0L, nrow = 5, ncol = 5) + expected[3, 3] <- 1L + expect_identical(thin(img, method = "guo_hall"), expected) + }) + + it("thinImage matches its documented 3x5 example output exactly", { + img <- matrix(c(0, 1, 1, 1, 0, + 0, 1, 1, 1, 0, + 0, 1, 1, 1, 0), + nrow = 3, byrow = TRUE) + expected <- matrix(c(0, 1, 1, 1, 0, + 0, 0, 1, 0, 0, + 0, 1, 1, 1, 0), + nrow = 3, byrow = TRUE) + expect_identical(thinImage(img), expected) + }) +}) + describe("thinImage drop-in", { it("accepts a logical matrix", { img <- matrix(FALSE, nrow = 5, ncol = 5) From a04198fbcb78e8543956afb49fb4826078e1aa55 Mon Sep 17 00:00:00 2001 From: Bill Denney Date: Sun, 28 Jun 2026 14:52:05 +0000 Subject: [PATCH 2/2] Harden unreachable defensive guards into fail-fast assertions Two defensively-unreachable `# nocov` guards previously fell back *silently* when their impossible state was reached. A silent fallback on an "impossible" branch masks the very bug it would signal. Convert both to loud errors that name the violated invariant and the upstream guarantee that makes the branch unreachable, so if the impossible ever happens it is caught rather than hidden. The `# nocov` markers are kept (the branches remain unreachable from R) with their comments relabelled as fail-fast assertions. - R/thin.R as_binary_matrix(): the length-2-`dim` array branch used to silently recurse. In R (>= 4.2) every length-2-`dim` object already satisfies is.matrix() and is handled by the branch above, so this is unreachable; now stop() reports the broken base-R invariant. - src/lee.cpp lee_can_delete(): the switch `default:` used to silently set on_boundary = 0 for a sub-iteration index outside 0-3, but the driving loop is `for (sub = 0; sub < 4; sub++)`; now Rcpp::stop() reports the corrupted loop bound. Kept on one line so the single `// # nocov` excludes the whole statement (matches distance_transform.cpp). src/distance_transform.cpp's `default:` guard was already a loud Rcpp::stop() and is left unchanged. Validation: devtools::test() 0 failures; coverage 100% of non-nocov lines; lintr clean; R CMD check --as-cran 0E/0W/1N (pre-existing Ubuntu compilation-flag NOTE). Co-Authored-By: Claude Opus 4.8 --- R/thin.R | 15 +++++++++------ src/lee.cpp | 10 ++++++---- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/R/thin.R b/R/thin.R index b835db8..1f2aa07 100644 --- a/R/thin.R +++ b/R/thin.R @@ -75,12 +75,15 @@ as_binary_matrix <- function(image) { } if (is.array(image) && length(dim(image)) == 2L) { # nocov start - # Unreachable invariant guard: in R (>= 4.2) every object with a - # length-2 `dim` attribute also satisfies is.matrix(), so any 2-D - # array is already handled by the is.matrix() branch above. Kept as - # a defensive recursion target in case base R's array/matrix - # semantics ever diverge. - return(as_binary_matrix(matrix(image, nrow = dim(image)[1], ncol = dim(image)[2]))) + # Fail-fast assertion (unreachable): the package requires R (>= 4.2), + # where every object carrying a length-2 `dim` attribute also satisfies + # is.matrix() -- so any 2-D array is already handled by the is.matrix() + # branch above and control cannot arrive here. If it ever does, base R's + # array/matrix invariant has changed underneath us; raise loudly rather + # than silently recursing so the broken assumption is caught, not masked. + stop("thinr internal invariant violated: a length-2 `dim` object ", + "reached the array branch without satisfying is.matrix(). ", + "This contradicts base R (>= 4.2) array/matrix semantics.") # nocov end } stop("thinr::thin() expects a 2-D matrix. ", diff --git a/src/lee.cpp b/src/lee.cpp index a732d6a..56f055a 100644 --- a/src/lee.cpp +++ b/src/lee.cpp @@ -39,10 +39,12 @@ static inline int lee_can_delete(int p2, int p3, int p4, int p5, case 1: on_boundary = (p4 == 0); break; // east case 2: on_boundary = (p6 == 0); break; // south case 3: on_boundary = (p8 == 0); break; // west - // Defensive default: `sub` is driven only by the internal - // `for (int sub = 0; sub < 4; sub++)` loop below, so values - // outside 0-3 never reach here. Not exercisable from R. - default: on_boundary = 0; // # nocov + // Fail-fast assertion (unreachable): `sub` is driven only by the + // internal `for (int sub = 0; sub < 4; sub++)` loop below, so values + // outside 0-3 cannot reach here. If one ever does, the caller's loop + // bound has been corrupted; abort loudly rather than silently treating + // the pixel as interior so the broken invariant is caught, not masked. + default: Rcpp::stop("thinr internal invariant: lee sub-iteration index out of range (expected 0-3) in lee_can_delete()."); // # nocov } if (!on_boundary) return 0;