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

Adding `wrap` Parameter to `test_dir`, `test_check`, and `run_tests` #586

Closed
brodieG opened this Issue Apr 22, 2017 · 27 comments

Comments

Projects
None yet
4 participants
@brodieG
Contributor

brodieG commented Apr 22, 2017

@krlmlr, Since 37cc0d0 it is no longer possible to run code with test_dir that involves withCallingHandlers error handling. This is because the default source action now sets wrap=TRUE, which causes all sourced code to be run within test_code.

Since wrap is not exposed in test_dir there is no way to disable this behavior when using test_dir (or test_check).

A simple example of the problem (here we just use test_file since test_file has the argument so we can show both cases):

f <- tempfile()
cat(
  "
  withCallingHandlers(
    stop('This should be handled'),
    error=function(e) cat('handled\n')
  )
  test_that('truth', {expect_true(TRUE)})
  ",
  file=f
)
test_file(f)              # fails
test_file(f, wrap=FALSE)  # works

For reference, more details on the running withCallingHandlers inside try block issue.

I'm happy to submit a PR that addresses this if you'd like.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Apr 23, 2017

Thanks. A straightforward fix would be to expose the wrap argument. @hadley: Would you support that?

@hadley

This comment has been minimized.

Member

hadley commented Apr 23, 2017

I'd rather leave this for now, and think more about the handler support in rlang.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Apr 24, 2017

@hadley note that 37cc0d0 breaks existing use cases and without adding the wrap parameter to test_dir etal (as was done for test_file) there is no way to restore the original behavior. Seems odd to add this functionality and a wrap parameter to test_file to control it, but not to test_dir or test_check.

@hadley

This comment has been minimized.

Member

hadley commented Oct 2, 2017

This seems ok to me - handled is printed in both cases

library(testthat)
f <- tempfile()
cat(
  "
  withCallingHandlers(
    stop('This should be handled'),
    error=function(e) cat('handled\n')
  )
  test_that('truth', {expect_true(TRUE)})
  ",
  file=f
)
test_file(f)              # fails
#> handled
#> 1
#> Failed -------------------------------------------------------------------------------------------------------------------
#> 1. Error: (unknown) (@file11e33799dbdb7#2) -------------------------------------------------------------------------------
#> This should be handled
#> 1: withCallingHandlers(stop("This should be handled"), error = function(e) cat("handled\n")) at /tmp/RtmpoLCXK7/file11e33799dbdb7:2
#> 2: stop("This should be handled")
#> 
#> DONE =====================================================================================================================
#> Don't worry, you'll get it.
test_file(f, wrap=FALSE)  # works
#> handled
#> Error in withCallingHandlers(stop("This should be handled"), error = function(e) cat("handled\n")): This should be handled
#> 
#> DONE =====================================================================================================================
@hadley

This comment has been minimized.

Member

hadley commented Oct 2, 2017

If that's not correct, it would be useful for you to write up what you do expect, and why.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 2, 2017

I'm sorry I was not very clear about the exact nature of the problem:

  • In the first run, testthat declares that the tests in our file failed with 1 error.
  • In the second run the error is handled by withCallingHandlers alone, but since it is handled AND wrap=FALSE, testthat does not declare a test failure (although there is some screen output)

To try to clarify a little, I run with the check reporter:

> test_file(f, reporter=CheckReporter)
handled
1. Error: (unknown) (@file6e7c7be4641e#2) --------------------------------------
This should be handled
1: withCallingHandlers(stop("This should be handled"), error = function(e) cat("handled\n")) at /tmp/Rtmpsuh4RO/file6e7c7be4641e:2
2: stop("This should be handled")

testthat results ================================================================
OK: 0 SKIPPED: 0 FAILED: 1
1. Error: (unknown) (@file6e7c7be4641e#2)

Error: testthat unit tests failed
> test_file(f, wrap=FALSE, reporter=CheckReporter)
handled
Error in withCallingHandlers(stop("This should be handled"), error = function(e) cat("handled\n")) :
  This should be handled
testthat results ================================================================
OK: 0 SKIPPED: 0 FAILED: 0

Notice how in the second run the tests are not considered to fail.

The problem is that with withCallingHandlers there is no way to fully capture the triggered error and prevent outer handling calls from wresting control from the inner ones. If I had used tryCatch then this would not be a problem; however, if I do that I cannot resume evaluation after non fatal conditions as I can with withCallingHandlers.

I explore the withCallingHandlers issue on SO in depth, if you're interested.

This matters to me because I built the tests for my own unit testing framework with testthat. My unit testing framework uses withCallingHandlers internally, so it itself cannot be run within a test_that block because evaluation would be interrupted. My testing pattern is to run my functions outside of test_that blocks, and then check the results stored in variables within test_that blocks. This worked fine before the change in paradigm discussed in this issue. Now my tests start failing because as soon a condition is issued inside my tests it is captured by the handlers added in test_file(..., wrap=TRUE).

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 2, 2017

The handle_fatal() handler could rethrow errors that come from outside of a test_that() block. However this feels wrong, I think it makes sense to catch errors here.

It seems the best solution would be to put your code within test_that() blocks. You can isolate your handling code from testthat's handlers by evaluating within a ToplevelExec() block. This is a C function but we'll expose it at R level in rlang.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

Thanks @lionel, I'll explore the use of ToplevelExec() for my use case. I will note that even if that works is probably useful to have a way of running expressions outside of handlers in test_dir et al., especially since that already exists for test_file (i.e. wrap=FALSE) and that is the status quo with the current version. Maybe an alternative could be a with_no_handlers block, although that seems like a lot of work compared to just adding the param already in test_file to the sister functions.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

It's just that the wrap argument feels a bit ill defined and complicates the API. So if it might be better to find solutions that don't require this argument (which doesn't exist on the current CRAN version IIUC). Rethrowing errors seems to preserve your use case. skip conditions could be resignaled as well.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

Note that I'm NOT looking for an error to be re-thrown, in fact, I'm looking for the exact opposite. I want withCallingHandlers to be able to fully handle conditions, which cannot be done if wrapped in a tryCatch:

f <- function()
  withCallingHandlers({
      warning("A");
      cat("/\\/\\ WOOHOO /\\/\\ \n")
    },
    warning = function(w) {}
  )

f()
## /\/\ WOOHOO /\/\                                              #   <-------- Notice this
## Warning message:
## In withCallingHandlers({ : A
tryCatch(f(), warning=function(w) warning(w))
## Warning message:
## In withCallingHandlers({ : A

A stand alone withCallingHandlers will resume execution after the warning, whereas when you wrap it in a tryCatch with a warning handler control is never returned to the interior evaluation. IMO this is a flaw in R's condition handling mechanism, which hopefully can be mitigated with ToplevelExec().

Previously I could just run some code that used withCallingHandlers outside of the test_that blocks. Now this is no longer possible with test_dir, test_check, etc because they auto-wrap things in test blocks. It is possible with test_file because it has the wrap argument that I can set to FALSE.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

Yes sorry I got confused. If we rethrow errors from the tryCatch the stack is of course unwound.

IMO this is a flaw in R's condition handling mechanism

I don't think it's flawed, it's just that it doesn't make sense to mark an exceptional condition (an error) as handled if the code that threw the error is not designed for this kind of recovery (i.e. it doesn't set a restart point).

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

But what about warnings? In my use case I'm evaluating arbitrary code inside withCallingHandlers. I need the code to keep evaluating normally after it issues a warning as it would if it were run on the command line. As far as I know there is no way to do this if withCallingHandlers is called within a tryCatch block that also handles warnings.

Keep in mind I'm evaluating arbitrary code. I don't know if it will contain warnings or not, e.g.:

withCallingHandlers(eval(exp), warning=...)

Where exp is supplied by the user.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

All warnings have restart points, which makes sense because they are not exceptional conditions. So you can invoke the muffleWarning restart from your handler to mark it as handled. Your local handler will be the last one called by R.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

Unfortunately base::signalCondition() doesn't set up restart points automatically. Unlike rlang::cnd_signal() which sets up a restart by default.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

Note that if you were able to mark errors as handled with a local handler, all kinds of undefined behaviours would arise because you'd be continuing code from errors issued anywhere, not just those issued at top level.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

My understanding of R semantics are that code should be allowed to continue after a condition is signaled. I.e. signaling a condition is not an interrupt per se. The interrupt can be optionally raised by that code after signaling the condition. As such, I want to be able to handle all conditions and let code interrupt only if it actually tries to interrupt. For example, in:

f2 <- function() {
  signalCondition(simpleError("hello world"))
  cat("goodbye world\n")
}
f2()
## goodbye world

I capture and record all conditions in my handling code, but because I use withCallingHandlers the provided code can keep on running merrily after the conditions are raised, as it would in unhandled code. The code is only interrupted if it also issues an interrupt as stop does:

f3 <- function() {
  stop(simpleError("hello world"))
  cat("goodbye world\n")
}
withRestarts(
  f3(),
  abort=function(a) {
    cat("Abort signalled!\n")
    invokeRestart("abort")
  }
)
## Error: hello world
## Abort signalled!

In other words: code should continue to evaluate after a condition is signaled, unless an interrupt is also raised as that is the default R behavior. This contrast to only resuming code if it sets a restart point. As you note signalCondition does not do this, and does not need to because standard R semantics are to keep evaluating after signalCondition unless an interrupt is also issued.

PS: Thanks for your time and patience in discussing this with me.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

standard R semantics are to keep evaluating after signalCondition unless an interrupt is also issued.

But standard semantics of R and Common Lisp also imply that all conditions are catchable.

Maybe what you're after is the ability to register handlers that have precedence over handlers added later in the call stack.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

I capture and record all conditions in my handling code, but because I use withCallingHandlers the provided code can keep on running merrily after the conditions are raised, as it would in unhandled code. The code is only interrupted if it also issues an interrupt as stop does:

IIUC, rethrowing fatal errors as suggested earlier would solve your problems?

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

But standard semantics of R and Common Lisp also imply that all conditions are catchable.

Only unhandled ones though. If a condition is handled by an inner handler it shouldn't have to be caught by an outer unless it is re-thrown. Compare:

tryCatch(
  tryCatch(
    signalCondition(simpleError("hello")),
    error=function(e) cat("Caught Inner\n")
  ),
  error=function(e) cat("Caught outer\n")
)
## Caught Inner
tryCatch(
  withCallingHandlers(
    signalCondition(simpleError("hello")),
    error=function(e) cat("Caught Inner\n")
  ),
  error=function(e) cat("Caught outer\n")
)
## Caught outer

Basically withCallingHandlers re-throws the condition (well, it doesn't quite do that, it just doesn't stop the condition bubbling and there is no way to stop it). My problem is that I can't stop withCallingHandlers, but I need the withCallingHandlers behavior of allowing code to resume after signals.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

tryCatch does what I want where the inner handling fully handles the condition, but it doesn't then allow resuming the code after the condition is signaled (without a restart set in the evaluated code, over which I have no control).

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

If a condition is handled by an inner handler

I think what you're missing is that merely running a handler doesn't mean that the condition is handled. It is only handled if control flow jumps out of the handler, e.g. by a non-local return or a restart invocation.

but it doesn't then allow resuming the code after the condition is signaled

Only fatal error are caught so IIUC you wouldn't want code to resume.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

I need to mimic what happens when code is run on the command line, without handlers, which is:

if a condition is signalled with no interrupt, code continues to evaluate (e.g. warning, or any signalCondition call)

Additionally, I need to record the condition that was signaled.

You can do this with withCallingHandlers, but only if withCallingHandlers isn't wrapped inside a tryCatch.

How would you solve this problem (let code continue to evaluate after a signalCondition call, but record the condition)? You are just given the code to evaluate as an expression.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

Again, this only happens for fatal conditions which would stop the eval() call anyway.

As for recording the fatal errors, this would be solved because rethrowing them would give a chance to run your handlers. However the call stack at time of handling will be different and that might be an issue. If that is an issue it might make sense to add the original call stack in the condition data.

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

Again, this only happens for fatal conditions which would stop the eval() call anyway.

Maybe I'm not understanding what you are saying. Let's try one more example. We have function f:

f <- function() {
  signalCondition(simpleCondition(sample(letters, 1)))
  sample(letters, 1)
}

And we want to record the values of the condition issued, in addition to the return value. Compare:

cond <- res <- character()
withCallingHandlers(
  res <- f(),
  condition=function(e) cond <<- conditionMessage(e)
)
cond
## [1] "s"
res
## [1] "h"

to:

cond <- res <- character()
tryCatch(
  withCallingHandlers(
    res <- f(),
    condition=function(e) cond <<- conditionMessage(e)
  ),
  condition=function(e) NULL
)
cond
## [1] "o"
res
## character(0)

There are no fatal conditions involved here. In the first case I can recover the condition and the result because the res <- f() call is allowed to complete. In the latter I can't because tryCatch interrupts evaluation of that expression.

@lionel-

This comment has been minimized.

Member

lionel- commented Oct 3, 2017

In your example you're catching all conditions whereas testthat only catches error and skip conditions....

Anyway, we're losing too much time discussing this. Apparently all we need is the ability for other packages to load the test files with all testthat infrastructural features (file handling, reporter invocation), and a wrap argument might very well be a reasonable way of achieving that so I've changed my mind ;)

@brodieG

This comment has been minimized.

Contributor

brodieG commented Oct 3, 2017

Great, thanks for your patience with me. FWIW the related PR is: #597.

Hopefully I can get your suggestion of ToplevelExec to work and then even this won't affect me.

@hadley hadley added the feature label Oct 5, 2017

@hadley

This comment has been minimized.

Member

hadley commented Oct 5, 2017

Ok, I'm convinced.

@hadley hadley added the wip label Oct 5, 2017

@hadley hadley closed this in 17b8b32 Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment