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: |
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
My comment is based on gut feeling rather than actual testthat knowledge
| } 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. |
|
|
| signalCondition(exp) | ||
| }, | ||
| continue_test = function(e) NULL | ||
| muffle_expectation = function(e) NULL |
There was a problem hiding this comment.
Maybe also add a warning message within continue_test for a smoother transition?
| muffle_expectation = function(e) NULL | |
| muffle_expectation = function(e) NULL, | |
| continue_test = function(e) NULL |
Changing the news entry for testthat 2.3.0 (and not adding an entry for 3.3.0) feels odd.
There was a problem hiding this comment.
I think the news change was accidental. You're saying your package relied on the continue_test restart? I think my working assumption was that it was an internal implementation detail that no one new about 😞
There was a problem hiding this comment.
Correct. Shinytest2 used it.
All good. Making a patch fix.
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.