Skip to content

Address review feedback: use helper for error snapshots and add test function #2522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
krlmlr merged 5 commits into from
Jan 24, 2026
Merged

Address review feedback: use helper for error snapshots and add test function #2522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
afab014
Initial plan
Copilot Jan 22, 2026
d905292
feat: add wrapper for error snapshot testing and test function
Copilot Jan 22, 2026
331d876
fix: remove punctuation from test error message
Copilot Jan 22, 2026
f628871
fix: address review feedback on test function export and snapshot format
Copilot Jan 24, 2026
98a83e4
Tweaks
krlmlr Jan 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,23 @@ modify_list <- function(x, y) {

utils::modifyList(x, y)
}

#' Test function to verify error formatting with file and line information
#'
#' @description
#' This is a test function that throws an error from C code with file and line
#' information.
#' The error message should include the source file and line number where the
#' error occurred.
#'
#' @return This function never returns; it always throws an error.
#' @keywords internal
#' @noRd
#' @examples
#' \dontrun{
#' # This will throw an error with source location information
#' test_error_with_source()
#' }
test_error_with_source <- function() {
.Call(Rx_igraph_test_error_with_source)
}
2 changes: 2 additions & 0 deletions src/cpp11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ extern SEXP Rx_igraph_st_vertex_connectivity(SEXP, SEXP, SEXP);
extern SEXP Rx_igraph_star(SEXP, SEXP, SEXP);
extern SEXP Rx_igraph_subcomponent(SEXP, SEXP, SEXP);
extern SEXP Rx_igraph_subisomorphic_lad(SEXP, SEXP, SEXP, SEXP, SEXP, SEXP, SEXP);
extern SEXP Rx_igraph_test_error_with_source(void);
extern SEXP Rx_igraph_transitivity_local_undirected_all(SEXP, SEXP);
extern SEXP Rx_igraph_union(SEXP, SEXP);
extern SEXP Rx_igraph_vcount(SEXP);
Expand Down Expand Up @@ -980,6 +981,7 @@ static const R_CallMethodDef CallEntries[] = {
{"Rx_igraph_star", (DL_FUNC) &Rx_igraph_star, 3},
{"Rx_igraph_subcomponent", (DL_FUNC) &Rx_igraph_subcomponent, 3},
{"Rx_igraph_subisomorphic_lad", (DL_FUNC) &Rx_igraph_subisomorphic_lad, 7},
{"Rx_igraph_test_error_with_source", (DL_FUNC) &Rx_igraph_test_error_with_source, 0},
{"Rx_igraph_transitivity_local_undirected_all", (DL_FUNC) &Rx_igraph_transitivity_local_undirected_all, 2},
{"Rx_igraph_union", (DL_FUNC) &Rx_igraph_union, 2},
{"Rx_igraph_vcount", (DL_FUNC) &Rx_igraph_vcount, 1},
Expand Down
7 changes: 7 additions & 0 deletions src/rinterface_extra.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7760,6 +7760,13 @@ SEXP Rx_igraph_create_bipartite(SEXP types, SEXP edges, SEXP directed) {
return R_igraph_create_bipartite(types, edges, directed);
}

/* Test function to verify error formatting with file and line information */
attribute_visible SEXP Rx_igraph_test_error_with_source(void) {
igraph_errorf("Test error message for verifying source location formatting",
__FILE__, __LINE__, IGRAPH_EINVAL);
return R_NilValue;
}

SEXP Rx_igraph_finalizer(void) {
return R_igraph_finalizer();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/_snaps/centrality.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
Condition
Error in `arpack()`:
! ARPACK error. N must be positive
Source: linalg/arpack.c:<linenumber>
Source: linalg/arpack.c:xx

---

Expand Down
9 changes: 9 additions & 0 deletions tests/testthat/_snaps/error-formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# error messages include source file and line information

Code
test_error_with_source()
Condition
Error in `test_error_with_source()`:
! Test error message for verifying source location formatting. Invalid value
Source: rinterface_extra.c:<linenumber>
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The snapshot uses <linenumber> as a placeholder, but the codebase convention is to use :xx. The helper function expect_snapshot_igraph_error() (tests/testthat/helper.R:47-56) uses gsub(":(\\d+)", ":xx", y), and this pattern is used in the vast majority of error snapshots (e.g., tests/testthat/_snaps/aaa-auto.md and tests/testthat/_snaps/centrality.md).

Only a couple of tests in test-games.R use <linenumber>, which appears to be an exception to the established convention. For consistency, consider using the expect_snapshot_igraph_error() helper and updating the snapshot to use :xx instead of <linenumber>.

Copilot uses AI. Check for mistakes.

10 changes: 3 additions & 7 deletions tests/testthat/test-centrality.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,13 +845,9 @@ test_that("eigen_centrality() deprecated scale argument", {

test_that("arpack() errors well", {
f <- function(x, extra = NULL) x
expect_snapshot(
error = TRUE,
{
arpack(f, options = list(nev = 2, ncv = 4), sym = TRUE)
},
transform = function(x) sub("\\:[0-9]+", ":<linenumber>", x)
)
expect_snapshot_igraph_error({
arpack(f, options = list(nev = 2, ncv = 4), sym = TRUE)
})
expect_snapshot(error = TRUE, {
arpack(
f,
Expand Down
8 changes: 8 additions & 0 deletions tests/testthat/test-error-formatting.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
test_that("error messages include source file and line information", {
expect_snapshot(
error = TRUE,
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is missing a transform parameter to replace line numbers in the snapshot. The snapshot file shows <linenumber>, but without a transform, the snapshot will contain actual line numbers that will change when the C code is modified.

According to the PR description, this test should use transform = function(x) sub(":(\\d+)", ":<linenumber>", x), but this is missing from the test. Alternatively, consider using the existing helper function expect_snapshot_igraph_error() which already handles this transformation (though it uses :xx instead of :<linenumber>).

Suggested change
error = TRUE,
error = TRUE,
transform = function(x) sub(":(\\d+)", ":<linenumber>", x),

Copilot uses AI. Check for mistakes.
{
test_error_with_source()
}
)
})