test: nettoyage de la suite - doublon, ecritures cwd, test mort, assertions vides

NEWS.md

@@ -88,6 +88,23 @@
 
 ## Chores
 
+* Test-suite cleanup (issue #31): the byte-for-byte duplicated
+  "uses key parameter over YAML rules" block is gone; every test that wrote
+  a fixed-name YAML into the working directory now uses `tempfile()` +
+  `on.exit(unlink())` (CRAN policy, and what NEWS 0.4.4 already claimed);
+  the orphan committed fixtures `tests/testthat/rules.yaml` and `test.yaml`
+  (referenced by no test) are removed; the unrunnable
+  `test-huge-parquet.R` (skip-everywhere + hardcoded Windows paths) moves to
+  `dev/manual-tests/`; the silent conditional assertions of
+  `test-extraction-params.R` (`if (length(extracts) > 0) expect_...`) assert
+  their precondition first, so a cap check can no longer pass vacuously; and
+  the small internal helpers (`format_key_examples()`, `get_col_names()`,
+  `is_non_local()`/`is_arrow()`, the `file = NULL` branch of
+  `datadiff_report_html()`) get direct unit tests. The larger redundancy
+  compression the issue sketches (IEEE 754 in 7 copies, duplicate-key tests
+  in 3 files) is deliberately left for a dedicated pass: it is high-churn
+  refactoring of green tests.
+
 * Packaging and code cleanup (issue #34): `dev/` is excluded from the tarball
   (`.Rbuildignore`); the orphan `inst/templates/rules_template.yaml`
   (referenced nowhere) is gone; unused `@importFrom` entries dropped

tests/testthat/test-extraction-params.R

@@ -41,11 +41,10 @@ test_that("extract_failed = FALSE prevents row extraction", {
   expect_false(result$all_passed)
   # With extract_failed = FALSE, no rows should be extracted
   extracts <- pointblank::get_data_extracts(result$reponse)
-  # Extracts should be empty or contain empty data frames
-  if (length(extracts) > 0) {
-    total_rows <- sum(vapply(extracts, nrow, integer(1)))
-    expect_equal(total_rows, 0)
-  }
+  # Extracts must be empty or contain only empty data frames: computing the
+  # total over an empty list gives 0, so the assertion never silently skips
+  total_rows <- sum(vapply(extracts, nrow, integer(1)))
+  expect_equal(total_rows, 0)
 })
 
 test_that("extract_failed accepts logical values only in practice", {
@@ -86,12 +85,12 @@ test_that("get_first_n limits extracted rows", {
   extracts <- pointblank::get_data_extracts(result$reponse)
 
   # Each extract should have at most 5 rows
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 5)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 5)
   }
 })
 
@@ -106,12 +105,12 @@ test_that("get_first_n = 1 extracts only first failure", {
   expect_false(result$all_passed)
   extracts <- pointblank::get_data_extracts(result$reponse)
 
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 1)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 1)
   }
 })
 
@@ -160,12 +159,12 @@ test_that("sample_n limits extracted rows via random sampling", {
   expect_false(result$all_passed)
   extracts <- pointblank::get_data_extracts(result$reponse)
 
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 10)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 10)
   }
 })
 
@@ -180,12 +179,12 @@ test_that("sample_n = 1 extracts only one random failure", {
   expect_false(result$all_passed)
   extracts <- pointblank::get_data_extracts(result$reponse)
 
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 1)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 1)
   }
 })
 
@@ -279,12 +278,12 @@ test_that("sample_limit caps sample_frac results", {
   expect_false(result$all_passed)
   extracts <- pointblank::get_data_extracts(result$reponse)
 
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 5)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 5)
   }
 })
 
@@ -300,12 +299,12 @@ test_that("sample_limit = 1 limits to single row", {
   expect_false(result$all_passed)
   extracts <- pointblank::get_data_extracts(result$reponse)
 
-  if (length(extracts) > 0) {
-    for (ext in extracts) {
-      if (is.data.frame(ext) && nrow(ext) > 0) {
-        expect_lte(nrow(ext), 1)
-      }
-    }
+  # Assert the precondition first: a failing comparison with extraction
+  # enabled must produce at least one extract (a silent empty list would
+  # make the cap check vacuous)
+  expect_gt(length(extracts), 0)
+  for (ext in extracts) {
+    expect_lte(nrow(as.data.frame(ext)), 1)
   }
 })
 
@@ -339,10 +338,8 @@ test_that("extract_failed = FALSE overrides other extraction params", {
   extracts <- pointblank::get_data_extracts(result$reponse)
 
   # With extract_failed = FALSE, should have no extracted rows
-  if (length(extracts) > 0) {
-    total_rows <- sum(vapply(extracts, nrow, integer(1)))
-    expect_equal(total_rows, 0)
-  }
+  total_rows <- sum(vapply(extracts, nrow, integer(1)))
+  expect_equal(total_rows, 0)
 })
 
 test_that("get_first_n and sample_n are mutually exclusive (last one wins or pointblank handles)", {

tests/testthat/test-input-validation.R

@@ -9,13 +9,13 @@ test_that("write_rules_template handles invalid key parameter", {
   unlink(temp_path)
 
   # Test empty key vector (should error)
-  expect_error(write_rules_template(df, key = character(0), path = "test.yaml"))
+  expect_error(write_rules_template(df, key = character(0), path = tempfile(fileext = ".yaml")))
 
   # Test non-character key (should error)
-  expect_error(write_rules_template(df, key = 123, path = "test.yaml"))
+  expect_error(write_rules_template(df, key = 123, path = tempfile(fileext = ".yaml")))
 
   # Test non-existent column as key (should error)
-  expect_error(write_rules_template(df, key = "nonexistent", path = "test.yaml"))
+  expect_error(write_rules_template(df, key = "nonexistent", path = tempfile(fileext = ".yaml")))
 })
 
 test_that("write_rules_template handles invalid ignore_columns_default", {
@@ -96,16 +96,17 @@ test_that("write_rules_template handles extreme values", {
 
 test_that("write_rules_template handles invalid data types", {
   # Test with non-dataframe input (should error)
-  expect_error(write_rules_template("not_a_dataframe", key = "id", path = "test.yaml"))
+  expect_error(write_rules_template("not_a_dataframe", key = "id", path = tempfile(fileext = ".yaml")))
 
   # Test with empty dataframe (should error on key)
   empty_df <- data.frame()
-  expect_error(write_rules_template(empty_df, key = "id", path = "test.yaml"))
+  expect_error(write_rules_template(empty_df, key = "id", path = tempfile(fileext = ".yaml")))
 
   # Test with dataframe having no rows (should work - creates template with empty by_name)
   no_rows_df <- data.frame(id = integer(0), value = numeric(0))
-  expect_no_error(write_rules_template(no_rows_df, key = "id", path = "test_no_rows.yaml"))
-  if (file.exists("test_no_rows.yaml")) unlink("test_no_rows.yaml")
+  no_rows_path <- tempfile(fileext = ".yaml")
+  on.exit(unlink(no_rows_path), add = TRUE)
+  expect_no_error(write_rules_template(no_rows_df, key = "id", path = no_rows_path))
 })
 
 test_that("write_rules_template handles file path issues", {

tests/testthat/test-internal-helpers.R

@@ -0,0 +1,47 @@
+# Direct unit tests for small internal helpers that previously had no
+# dedicated coverage.
+
+test_that("format_key_examples formats and truncates", {
+  keys <- data.frame(id = c(1, 2, 3, 4), grp = c("a", "b", "c", "d"))
+  out <- format_key_examples(keys, c("id", "grp"))
+  expect_length(out, 4L)                 # 3 examples + "..."
+  expect_identical(out[4], "...")
+  expect_match(out[1], "id = 1")
+  expect_match(out[1], "grp = a")
+
+  out2 <- format_key_examples(keys[1:2, , drop = FALSE], c("id", "grp"))
+  expect_length(out2, 2L)                # below threshold: no marker
+})
+
+test_that("get_col_names works on data.frames and 0-column inputs", {
+  expect_identical(get_col_names(data.frame(a = 1, b = 2)), c("a", "b"))
+  expect_length(get_col_names(data.frame()), 0L)
+})
+
+test_that("is_non_local and is_arrow classify inputs", {
+  df <- data.frame(a = 1)
+  expect_false(is_non_local(df))
+  expect_false(is_arrow(df))
+
+  skip_if_not_installed("duckdb")
+  skip_if_not_installed("dbplyr")
+  con <- DBI::dbConnect(duckdb::duckdb())
+  on.exit(DBI::dbDisconnect(con, shutdown = TRUE), add = TRUE)
+  duckdb::dbWriteTable(con, "t", df)
+  lazy <- dplyr::tbl(con, "t")
+  expect_true(is_non_local(lazy))
+  expect_false(is_arrow(lazy))
+
+  skip_if_not_installed("arrow")
+  at <- arrow::arrow_table(df)
+  expect_true(is_non_local(at))
+  expect_true(is_arrow(at))
+})
+
+test_that("datadiff_report_html with file = NULL returns the report invisibly", {
+  ref <- data.frame(id = 1:2, x = c(1, 2))
+  res <- suppressMessages(compare_datasets_from_yaml(ref, ref, key = "id"))
+  vis <- withVisible(datadiff_report_html(res, file = NULL))
+  expect_false(vis$visible)
+  expect_false(is.null(vis$value))
+})