Add tests to improve coverage

LynxJinyangii · LynxJinyangii · commit 7e866e4b1280 · 2026-04-02T14:22:44.000+01:00
diff --git a/RcppTskit/R/RcppExports.R b/RcppTskit/R/RcppExports.R
@@ -9,6 +9,18 @@ rtsk_variant_iterator_next <- function(iterator) {
     .Call(`_RcppTskit_rtsk_variant_iterator_next`, iterator)
 }
 
+test_rtsk_variant_iterator_force_null_first_allele <- function(enabled) {
+    invisible(.Call(`_RcppTskit_test_rtsk_variant_iterator_force_null_first_allele`, enabled))
+}
+
+test_rtsk_variant_iterator_set_site_bounds <- function(iterator, next_site_id, stop_site_id) {
+    invisible(.Call(`_RcppTskit_test_rtsk_variant_iterator_set_site_bounds`, iterator, next_site_id, stop_site_id))
+}
+
+test_variant_site_index_range <- function(start, stop) {
+    invisible(.Call(`_RcppTskit_test_variant_site_index_range`, start, stop))
+}
+
 test_validate_options <- function(options, supported) {
     .Call(`_RcppTskit_test_validate_options`, options, supported)
 }
diff --git a/RcppTskit/src/RcppExports.cpp b/RcppTskit/src/RcppExports.cpp
@@ -37,6 +37,39 @@ BEGIN_RCPP
     return rcpp_result_gen;
 END_RCPP
 }
+// test_rtsk_variant_iterator_force_null_first_allele
+void test_rtsk_variant_iterator_force_null_first_allele(const bool enabled);
+RcppExport SEXP _RcppTskit_test_rtsk_variant_iterator_force_null_first_allele(SEXP enabledSEXP) {
+BEGIN_RCPP
+    Rcpp::RNGScope rcpp_rngScope_gen;
+    Rcpp::traits::input_parameter< const bool >::type enabled(enabledSEXP);
+    test_rtsk_variant_iterator_force_null_first_allele(enabled);
+    return R_NilValue;
+END_RCPP
+}
+// test_rtsk_variant_iterator_set_site_bounds
+void test_rtsk_variant_iterator_set_site_bounds(const SEXP iterator, const int next_site_id, const int stop_site_id);
+RcppExport SEXP _RcppTskit_test_rtsk_variant_iterator_set_site_bounds(SEXP iteratorSEXP, SEXP next_site_idSEXP, SEXP stop_site_idSEXP) {
+BEGIN_RCPP
+    Rcpp::RNGScope rcpp_rngScope_gen;
+    Rcpp::traits::input_parameter< const SEXP >::type iterator(iteratorSEXP);
+    Rcpp::traits::input_parameter< const int >::type next_site_id(next_site_idSEXP);
+    Rcpp::traits::input_parameter< const int >::type stop_site_id(stop_site_idSEXP);
+    test_rtsk_variant_iterator_set_site_bounds(iterator, next_site_id, stop_site_id);
+    return R_NilValue;
+END_RCPP
+}
+// test_variant_site_index_range
+void test_variant_site_index_range(const std::string start, const std::string stop);
+RcppExport SEXP _RcppTskit_test_variant_site_index_range(SEXP startSEXP, SEXP stopSEXP) {
+BEGIN_RCPP
+    Rcpp::RNGScope rcpp_rngScope_gen;
+    Rcpp::traits::input_parameter< const std::string >::type start(startSEXP);
+    Rcpp::traits::input_parameter< const std::string >::type stop(stopSEXP);
+    test_variant_site_index_range(start, stop);
+    return R_NilValue;
+END_RCPP
+}
 // test_validate_options
 int test_validate_options(const int options, const int supported);
 RcppExport SEXP _RcppTskit_test_validate_options(SEXP optionsSEXP, SEXP supportedSEXP) {
@@ -771,6 +804,9 @@ END_RCPP
 static const R_CallMethodDef CallEntries[] = {
     {"_RcppTskit_rtsk_variant_iterator_init", (DL_FUNC) &_RcppTskit_rtsk_variant_iterator_init, 6},
     {"_RcppTskit_rtsk_variant_iterator_next", (DL_FUNC) &_RcppTskit_rtsk_variant_iterator_next, 1},
+    {"_RcppTskit_test_rtsk_variant_iterator_force_null_first_allele", (DL_FUNC) &_RcppTskit_test_rtsk_variant_iterator_force_null_first_allele, 1},
+    {"_RcppTskit_test_rtsk_variant_iterator_set_site_bounds", (DL_FUNC) &_RcppTskit_test_rtsk_variant_iterator_set_site_bounds, 3},
+    {"_RcppTskit_test_variant_site_index_range", (DL_FUNC) &_RcppTskit_test_variant_site_index_range, 2},
     {"_RcppTskit_test_validate_options", (DL_FUNC) &_RcppTskit_test_validate_options, 2},
     {"_RcppTskit_test_rtsk_wrap_tsk_size_t_as_integer64", (DL_FUNC) &_RcppTskit_test_rtsk_wrap_tsk_size_t_as_integer64, 2},
     {"_RcppTskit_kastore_version", (DL_FUNC) &_RcppTskit_kastore_version, 0},
diff --git a/RcppTskit/src/RcppTskit.cpp b/RcppTskit/src/RcppTskit.cpp
@@ -172,6 +172,8 @@ using rtsk_variant_iterator_t =
     Rcpp::XPtr<rtsk_variant_iterator_state_t, Rcpp::PreserveStorage,
                rtsk_variant_iterator_free, true>;
 
+bool g_test_force_null_first_allele = false;
+
 // INTERNAL
 // @title Convert \code{Rcpp::Nullable} vector to empty-or-value vector
 // @param value nullable vector from \code{R}
@@ -205,6 +207,9 @@ std::vector<tsk_id_t> int_vector_to_tsk_id_vector(
   return out;
 }
 
+void validate_variant_site_index_range(const tsk_size_t start,
+                                       const tsk_size_t stop);
+
 std::pair<tsk_id_t, tsk_id_t>
 compute_variant_iteration_bounds(rtsk_treeseq_t &ts_xptr, const double left,
                                  const double right) {
@@ -233,15 +238,19 @@ compute_variant_iteration_bounds(rtsk_treeseq_t &ts_xptr, const double left,
       static_cast<tsk_size_t>(std::lower_bound(begin, end, left) - begin);
   const tsk_size_t stop =
       static_cast<tsk_size_t>(std::lower_bound(begin, end, right) - begin);
+  validate_variant_site_index_range(start, stop);
 
+  return std::make_pair(static_cast<tsk_id_t>(start),
+                        static_cast<tsk_id_t>(stop));
+}
+
+void validate_variant_site_index_range(const tsk_size_t start,
+                                       const tsk_size_t stop) {
   const tsk_size_t max_id =
       static_cast<tsk_size_t>(std::numeric_limits<tsk_id_t>::max());
   if (start > max_id || stop > max_id) {
     Rcpp::stop("Site index exceeds tsk_id_t range");
   }
-
-  return std::make_pair(static_cast<tsk_id_t>(start),
-                        static_cast<tsk_id_t>(stop));
 }
 
 // INTERNAL
@@ -355,6 +364,10 @@ SEXP rtsk_variant_iterator_next(const SEXP iterator) {
   }
 
   const tsk_variant_t &variant = iterator_xptr->variant;
+  if (g_test_force_null_first_allele && variant.num_alleles > 0) {
+    iterator_xptr->variant.alleles[0] = nullptr;
+    g_test_force_null_first_allele = false;
+  }
 
   Rcpp::IntegerVector genotypes(variant.num_samples);
   for (tsk_size_t j = 0; j < variant.num_samples; ++j) {
@@ -377,6 +390,39 @@ SEXP rtsk_variant_iterator_next(const SEXP iterator) {
       Rcpp::_["has_missing_data"] = variant.has_missing_data);
 }
 
+// TEST-ONLY
+// [[Rcpp::export]]
+void test_rtsk_variant_iterator_force_null_first_allele(const bool enabled) {
+  g_test_force_null_first_allele = enabled;
+}
+
+// TEST-ONLY
+// [[Rcpp::export]]
+void test_rtsk_variant_iterator_set_site_bounds(const SEXP iterator,
+                                                const int next_site_id,
+                                                const int stop_site_id) {
+  rtsk_variant_iterator_t iterator_xptr(iterator);
+  iterator_xptr->next_site_id = static_cast<tsk_id_t>(next_site_id);
+  iterator_xptr->stop_site_id = static_cast<tsk_id_t>(stop_site_id);
+}
+
+// TEST-ONLY
+// [[Rcpp::export]]
+void test_variant_site_index_range(const std::string start,
+                                   const std::string stop) {
+  unsigned long long start_parsed = 0;
+  unsigned long long stop_parsed = 0;
+  try {
+    start_parsed = std::stoull(start);
+    stop_parsed = std::stoull(stop);
+  } catch (const std::exception &) {
+    Rcpp::stop("start and stop must be valid base-10 unsigned integer strings");
+  }
+  const tsk_size_t start_size = static_cast<tsk_size_t>(start_parsed);
+  const tsk_size_t stop_size = static_cast<tsk_size_t>(stop_parsed);
+  validate_variant_site_index_range(start_size, stop_size);
+}
+
 // TEST-ONLY
 // @title Test helper for validating tskit flags
 // @param options that will be validated
diff --git a/RcppTskit/tests/testthat/test_TreeSequence.R b/RcppTskit/tests/testthat/test_TreeSequence.R
@@ -92,7 +92,17 @@ test_that("TreeSequence$variants() validates compatibility args", {
   ts_file <- system.file("examples/test.trees", package = "RcppTskit")
   ts <- ts_load(ts_file)
 
+  expect_error(ts$variants(copy = NA), "copy must be TRUE/FALSE")
+  expect_error(ts$variants(copy = "yes"), "copy must be TRUE/FALSE")
   expect_error(ts$variants(copy = FALSE), "copy = FALSE is not supported yet")
+  expect_error(
+    ts$variants(impute_missing_data = NA),
+    "impute_missing_data must be TRUE/FALSE or NULL"
+  )
+  expect_error(
+    ts$variants(impute_missing_data = "yes"),
+    "impute_missing_data must be TRUE/FALSE or NULL"
+  )
   expect_warning(
     ts$variants(impute_missing_data = TRUE),
     "impute_missing_data is deprecated"
diff --git a/RcppTskit/tests/testthat/test_variant_iterator_low_level.R b/RcppTskit/tests/testthat/test_variant_iterator_low_level.R
@@ -51,3 +51,79 @@ test_that("low-level variant iterator supports sample subsets", {
   expect_false(is.null(v))
   expect_length(v$genotypes, length(samples))
 })
+
+test_that("low-level variant iterator validates bounds", {
+  ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+  ts_xptr <- rtsk_treeseq_load(ts_file)
+  seq_len <- rtsk_treeseq_get_sequence_length(ts_xptr)
+
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, left = NaN, right = seq_len),
+    "left and right must be finite numbers"
+  )
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, left = -1, right = seq_len),
+    "left and right must be >= 0"
+  )
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, left = seq_len + 1, right = seq_len),
+    "left and right must be <= sequence length"
+  )
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, left = 1, right = 0.5),
+    "left must be <= right"
+  )
+})
+
+test_that("low-level variant iterator validates site-index range helper", {
+  expect_error(
+    test_variant_site_index_range("2147483648", "0"),
+    "Site index exceeds tsk_id_t range"
+  )
+})
+
+test_that("low-level variant iterator validates samples and alleles inputs", {
+  ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+  ts_xptr <- rtsk_treeseq_load(ts_file)
+  n_nodes <- as.integer(rtsk_treeseq_get_num_nodes(ts_xptr))
+
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, samples = c(n_nodes + 1L)),
+    "Node out of bounds"
+  )
+  expect_error(
+    rtsk_variant_iterator_init(ts_xptr, alleles = c("A", NA_character_)),
+    "alleles cannot contain NA"
+  )
+
+  it <- rtsk_variant_iterator_init(ts_xptr, alleles = c("A", "C", "G", "T"))
+  v <- rtsk_variant_iterator_next(it)
+  expect_false(is.null(v))
+})
+
+test_that("low-level variant iterator decode error path can be triggered", {
+  ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+  ts_xptr <- rtsk_treeseq_load(ts_file)
+  n_sites <- as.integer(rtsk_treeseq_get_num_sites(ts_xptr))
+
+  it <- rtsk_variant_iterator_init(ts_xptr)
+  test_rtsk_variant_iterator_set_site_bounds(
+    it,
+    next_site_id = n_sites,
+    stop_site_id = n_sites + 1L
+  )
+  expect_error(
+    rtsk_variant_iterator_next(it),
+    regexp = "Site out of bounds|Bounds check"
+  )
+})
+
+test_that("low-level variant iterator maps null alleles to NA_character_", {
+  ts_file <- system.file("examples/test.trees", package = "RcppTskit")
+  ts_xptr <- rtsk_treeseq_load(ts_file)
+  it <- rtsk_variant_iterator_init(ts_xptr)
+  test_rtsk_variant_iterator_force_null_first_allele(TRUE)
+  v <- rtsk_variant_iterator_next(it)
+  expect_false(is.null(v))
+  expect_true(anyNA(v$alleles))
+})
