-
Notifications
You must be signed in to change notification settings - Fork 341
Entrace unexpected errors in snapshots #2283
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.
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
Conversation
This ensures that their backtraces point to the source of the error, not the internals of `expect_snapshot()` Fixes #2277
|
Claude finished @hadley's task —— View job Reviewing PR: Entrace unexpected errors in snapshotsTodo List:
Review AnalysisThe core change in Key observations:
Issues found:
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances error handling in snapshot testing by ensuring that unexpected errors within expect_snapshot() produce backtraces that point to the source of the error rather than the internals of the snapshot mechanism. This is achieved by adding error entracing via rlang::entrace() in the verify_exec handler.
Key Changes:
- Added error entracing to the
verify_exec()function's output handler to capture meaningful backtraces for errors occurring within snapshots - Added a test case to verify that errors in snapshots produce useful backtraces
- Updated snapshot test outputs to reflect the new error handling behavior
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| R/verify-output.R | Added rlang::entrace() as an error handler in the evaluate output handler to capture backtraces |
| tests/testthat/reporters/backtraces.R | Added test case demonstrating error backtrace behavior in snapshots |
| tests/testthat/test-reporter-progress.R | Set TESTTHAT_MAX_FAILS to Inf to ensure all test failures are captured |
| tests/testthat/_snaps/reporter-progress.md | Updated snapshot to include the new test case and adjusted line numbers |
| tests/testthat/_snaps/snapshot.md | Changed error class from simpleError to rlang_error due to entracing |
| tests/testthat/_snaps/snapshot-reporter.md | Updated error message format to include "Error:" prefix |
| NEWS.md | Documented the improvement to snapshot error backtraces |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * `expect_snapshot()` on reports how to resolve failures once when running inside `R CMD check`. | ||
| * `expect_snapshot()` no longer skips on CRAN, as that skips the rest of the test. Instead it just returns, neither succeeding nor failing (#1585). | ||
| * `expect_snapshot()` and `expect_snapshot_file()` hints now include the path to the package, if it's not the current working directory (#1577). | ||
| * `expect_snapshot()` gives a more informative backtrace when the code inside the snapshot errors (#2277). |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'Entrace' to 'Entrance' in PR title; however, the term should be 'Entraces' (verb form) to match the technical usage in the code.
lionel-
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG!
This ensures that their backtraces point to the source of the error, not the internals of
expect_snapshot()Fixes #2277