-
Notifications
You must be signed in to change notification settings - Fork 174
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
Add features handle-panics
and backtrace
#421
Conversation
proptest/src/test_runner/runner.rs
Outdated
|
||
static INIT_ONCE: Once = Once::new(); | ||
// NB: no need for external sync, value is mutated only once, when init is performed | ||
static mut DEF_HOOK: Option<Box<dyn Fn(&PanicInfo<'_>) + Send + Sync>> = |
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.
please expand this to DEFAULT_HOOK
for clarity, i'm concerned that "DEF" could be mistaken for something else
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.
Done
@@ -210,6 +212,96 @@ where | |||
}) | |||
} | |||
|
|||
#[cfg(feature = "handle-panics")] | |||
mod panicky { |
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.
given the unsafe usage, this section could use a bit more documentation i think. possibly specifying what the responsibility of each static variable and each function as well as clarifying the order of operations. i don't think it needs to be long but just enough to provide more clarity
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.
Done. Added more comments, doc comments for funcs and variables, and module-level doc comment with overall description of whole process
proptest/src/test_runner/errors.rs
Outdated
(Self::Fail(l0, _, l2), Self::Fail(r0, _, r2)) => { | ||
l0 == r0 && l2 == r2 | ||
} | ||
_ => false, |
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.
i think i'd prefer if we exhaustively match here, even if it's more verbose
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.
Done
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.
thank you for this pr, i think this will be a very nice addition :)
My biggest concern is that I had to change |
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.
thank you for the detailed docs, this looks great 👍
I had started going through this last night. will finish up later tonight |
Ah right, thank you for raising this. I don't think we can merge then until we move to a new major version unfortunately, or without making it a separate feature like you suggested, and yes I can see how that's unappealing. |
Ok, then I guess the best course is to take out |
we're looking at a 2.0 release so we can include this and a couple other things that are pending. it'd probably take 1-2 months to get everything together if those timelines work for you. also happy to accept changes with no backwards compatibility issues in the meantime if you want a faster turnaround |
This got closed when i switched branch from master to main. I'll re-open against the main branch. |
handle-panics
attempts to capture panic backtrace when one happens, instead of spitting it to consolebacktrace
enables actual backtraces usingstd::backtrace
Backtrace::frames()
andBacktraceFrame
are still experimental.P.S: Sorry for "PR spam", prev one #419 was closed prematurely due to accident
closes #356