Ensure that expect_snapshot() registers restart
#2271
Conversation
Renamed `continue_test` to `muffle_expectation` to make it more clear what the purporse of this restart is — it's to allow us to opt-out of further handling.
|
Claude finished @hadley's task —— View job Reviewing PR changes... Todo List
AnalysisThe PR renames Changes Made:
Issues Found: Assessment: |
| } else { | ||
| cnd_signal(state$error) | ||
| # This might be a failed expectation, so we need to make sure | ||
| # that we can muffle it | ||
| withRestarts( | ||
| cnd_signal(state$error), | ||
| muffle_expectation = function() NULL | ||
| ) | ||
| } |
There was a problem hiding this comment.
I know very little about testthat internals, but is this supposed to be exp_signal(state$error)?
It feels like there should only be 1 place in testthat where you set up withRestarts(muffle_expectation =), and then other places should reuse that helper. Otherwise you end up with weird ad hoc calls like this one.
I'm not totally sure what state$error is though, and if calling exp_signal() on it is appropriate
There was a problem hiding this comment.
state$error could be any error, not just an expectation.
I agree that there shouldn't be multiple places where we have to add this restart, but the condition handling in snapshots is complicated and I know I've got it wrong before, so I don't really want to touch that code. (I just looked back at the blame and it seems like literally every line of that if statement required a different PR to get right 😭). (But I'll take one more look)
There was a problem hiding this comment.
Yeah, I tried again, and it got me into an even weirder state. So I don't think it's worth 100% reasoning through again, and this should be the only place where we need it.
|
Adding @lionel- just in case he has any insights. |
Renamed
continue_testtomuffle_expectationto make it more clear what the purporse of this restart is — it's to allow us to opt-out of further handling.