fix: scrub file paths in error/warning snapshots and fix spacing#2538
fix: scrub file paths in error/warning snapshots and fix spacing#2538
Conversation
|
@copilot: Try again. |
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
61a2620 to
7937564
Compare
| Condition | ||
| Error in `hrg_sample_impl()`: | ||
| ! At <file>:<line> : Assertion failed: n >= 0. This is an unexpected igraph error; please report this as a bug, along with the steps to reproduce it. | ||
| ! At <file>:<line>: Assertion failed: n >= 0. This is an unexpected igraph error; please report this as a bug, along with the steps to reproduce it. |
There was a problem hiding this comment.
Why does the assertion here and below fail? If it's a bug, add a FIXME to the test. If this can never occur in practice, change the test.
There was a problem hiding this comment.
The assertions fail because passing NULL to these HRG functions triggers an assertion failure in the C code (the R wrapper converts NULL to an empty HRG structure, which the C code doesn't handle properly). This is a bug in the C code - it should validate the input and return a proper error message instead of an assertion failure. Added FIXME comments in 280ce43 to document this issue.
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates snapshot testing helpers and test cases to consistently scrub C source file paths/line numbers from error and warning snapshots, and regenerates affected snapshots to reflect the new normalized formatting.
Changes:
- Replace ad-hoc snapshot transforms in tests with
expect_snapshot_igraph_error()/ newexpect_snapshot_igraph()wrappers. - Centralize path/line scrubbing logic in
scrub_igraph_file_paths()withintests/testthat/helper.R. - Regenerate snapshot markdowns to use
Source: <file>:<line>and normalizeAt <file>:<line>:formatting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-games.R | Switches error snapshot assertions to the igraph-specific error snapshot helper for consistent scrubbing. |
| tests/testthat/test-aaa-auto.R | Adds clarifying FIXME notes for HRG assertion-based errors and uses a new snapshot helper for a warning+output snapshot. |
| tests/testthat/helper.R | Introduces centralized scrubbing helper and a new expect_snapshot_igraph() wrapper for non-error snapshots. |
| tests/testthat/_snaps/games.md | Updates snapshots to scrub source file path/line. |
| tests/testthat/_snaps/aaa-auto.md | Updates snapshots to scrub warning source paths and normalize At ... spacing. |
Comments suppressed due to low confidence (1)
tests/testthat/test-aaa-auto.R:11060
- This change introduces
expect_snapshot_igraph()to scrub warning/source locations, buttest-aaa-auto.Rstill contains otherexpect_snapshot()calls that capture warnings withSource: file:lineand therefore remain unsanitized inaaa-auto.md(e.g.,dot_product_game_impl basicaround lines 1572/1575 andtransitivity_barrat_impl basicaround lines 2689/2692). If the goal is to scrub file paths/line numbers from warning snapshots generally (as described in the PR), these cases should also useexpect_snapshot_igraph()(or otherwise applyscrub_igraph_file_paths).
expect_snapshot_igraph(connect_neighborhood_impl(
graph = g,
order = 1,
mode = c("all", "out", "in")
))
| y <- gsub("Source: [^:]+:(\\d+|xx|<linenumber>)", "Source: <file>:<line>", y) | ||
| y <- gsub("At [^:]+:(\\d+|xx) ?:", "At <file>:<line>:", y) |
There was a problem hiding this comment.
scrub_igraph_file_paths() uses [^:]+ to match the file path, which will fail to scrub paths that contain a colon (e.g., Windows drive paths like C:/.../file.c:123). This can leave unsanitized paths/line numbers in snapshots and potentially make tests OS-dependent. Consider matching up to the last :<line> segment instead (e.g., using a greedy match before the line number, or otherwise allowing colons in the path).
| y <- gsub("Source: [^:]+:(\\d+|xx|<linenumber>)", "Source: <file>:<line>", y) | |
| y <- gsub("At [^:]+:(\\d+|xx) ?:", "At <file>:<line>:", y) | |
| y <- gsub("Source: .+:(\\d+|xx|<linenumber>)", "Source: <file>:<line>", y) | |
| y <- gsub("At .+:(\\d+|xx) ?:", "At <file>:<line>:", y) |
This PR addresses feedback on #2515 to properly scrub file paths and line numbers from error and warning messages in test snapshots.
Original PR: #2515
Triggering review: #2515 (review)
Changes Made
File Path Scrubbing
test-games.Rto useexpect_snapshot_igraph_error()for proper file path scrubbingtest-aaa-auto.Rto useexpect_snapshot_igraph()for warning message scrubbinggames.md:Source: games/degree_sequence_vl/gengraph_mr-connected.cpp:<linenumber>→Source: <file>:<line>aaa-auto.md:Source: operators/connect_neighborhood.c:85→Source: <file>:<line>Spacing Fix
At [^:]+:(\d+|xx) :toAt [^:]+:(\d+|xx) ?:At <file>:<line>:instead ofAt <file>:<line> :Code Improvements
scrub_igraph_file_paths()helper function to centralize scrubbing logicexpect_snapshot_igraph()for snapshots with warnings and output (not just errors)Documentation
hrg_sample_impl,hrg_sample_many_impl,hrg_game_impl) documenting that they trigger assertion failures in the C code when passed NULL/empty HRG structures. This is a bug - the C code should validate input and return a proper error message instead of an assertion failure.Testing
This PR was prepared with assistance from GitHub Copilot.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.