-
Notifications
You must be signed in to change notification settings - Fork 341
fix(shinytest2): Fix broken shinytest2 testing; Legacy muffle expectation and muffled snapshot_fail support #2294
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
While we don't support `continue_test` directly, it has the same intent as `muffle_expectation` as it was the previous name. So support it in the handler.
… and return `TRUE`
| 4: expectation("failure", message, srcref = srcref, trace = trace) | ||
| 5: exp_signal(exp) | ||
| 6: withRestarts(if (expectation_broken(exp)) { | ||
| stop(exp) | ||
| } else { | ||
| signalC | ||
| 7: withRestartList(expr, restarts) | ||
|
|
||
| 1: f() | ||
| 2: expect_true(FALSE) | ||
| 3: expect_waldo_constant_(act, exp, info = info, ignore_attr = TRUE) | ||
| 4: fail(msg, info = info, trace_env = trace_env) | ||
| 5: expectation("failure", message, srcref = srcref, trace = trace) | ||
| 6: exp_signal(exp) | ||
| 7: withRestarts(if (expectation_broken(exp)) { | ||
| stop(exp) | ||
| } else { | ||
| signalC | ||
| 8: withRestartList(expr, restarts) |
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 don't know why this change is necessary for the PR.
Did I do something odd to require the 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.
I think DebugReporter automatically trims off some number of frames to avoid showing too much testthat machinery. You wouldn't think that your change would affect this but I'm pretty sure withRestarts(code, a, b) is implemented as withRestarts(withRestarts(code, a), b) internally. Let me see if I can tweak some magic numbers somewhere.
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.
Only took like 4 tries but I figured it out.
|
Thank you! |
Fixes #2293
Fixes #2288
shinytest2::AppDriver$expect_values()takes a snapshot of the values received from Shiny. No problems.However, it also takes a screenshot snapshot of the app at the time of the values. This extra snapshot is added for git tracking purposes, not unit testing. This snapshot should never fail.
When
return(snapshot_fail())expectation was muffled, the return was equivalent toreturn(invisible())which is a NULL value. The fix is to not immediately return and to carry on as iffail_on_new == FALSE