Failure with dev testthat

[hadley]

ERROR (testthat.section.R:86:3): (code run outside of `test_that()`)
Error: Yo0.71072455169633
Backtrace:
  1. [ base::eval(...) ] with 1 more call
  9. my.unitizer + expr.1
 11. unitizer:::exec(getItem(e2), test.env, e1@global)
 12. unitizer:::.local(x, ...)
 13. unitizer:::eval_with_capture(x.to.eval, test.env, global = global)
 14. unitizer:::eval_user_exp(...)
 15. unitizer:::user_exp_handle(exp, env, "", unitizerUSEREXP)
 20. [ base::eval(...) ] with 1 more call
 22. base::withVisible(stop("Yo", runif(1)))

Again, not sure what's going on here, but please let me know if there's anything I can do to help.

I'm planning to submit testthat to CRAN in about a month.

[brodieG]

I'll have some time next week to look at this, but in the meantime, I can imagine this type of thing will happen if code outside of testthat is being run under handlers (I capture conditions with withCallingHandlers, but due to deficiencies in withCallingHandlers, that cannot be further nested inside other handlers).

Last time this came about we had one of those fun conversations. I recommend you don't read through that whole thing again, but just throwing it out there in case it rings a bell with any changes that may be fresh in your mind relating to wrapping out of test_that block code.

I could also be completely wrong about my guess. I'll confirm next week.

[hadley]

That makes me wonder if https://github.com/r-lib/testthat/issues/1004 is related, but looking at the code I don't think it changed how the code is executed, just how errors are reported.

[brodieG]

So the issue that I linked last time was that wrap had been turned to automatically TRUE with no way to turn it off for test_dir. At the time, I convinced you access to the parameter should be maintained (I run it with wrap=FALSE on my tests).

Now it's been both deprecated and removed (well, strictly speaking it hasn't been removed, but it's removed from test_files so effectively it's removed). I can understand (though disagree) with the desire/need to remove the parameter, but would it be possible to leave it in deprecated status without removing it?

[hadley]

The problem is that it adds complexity to a part of testthat is almost as complex as my brain can handle, and as far as I can tell, you're the only person that uses it.

[brodieG]

I understand, and it's unfortunate that the behavior changed from when I started to use testthat for this project. Unfortunately "fixing" this in my code is not completely trivial (and might even require C code, or adding a dependency to rlang, neither of which I'm really that interested in) as it involves a very complex part of the unitizer engine. It may not be too bad, but it's one of those things that could affect other parts of the system, and I'm wary of changing it without a bit of thought and time.

Would you consider just deprecating this for at least one release cycle instead of immediately removing it? In my mind "deprecation" involves some kind of managed glide path were folks are given a few release cycles to wean off functionality that is considered no longer maintenance-worthy.

[hadley]

I'd consider it if you did a PR, but as I said, it's a complex part of testthat that I don't really want to mess with. I agree that deprecation would be desirable here, but this is just a place where I have to make a judgement call based on the amount of existing code in the wild that uses it.

[brodieG]

Ok, I'll submit a PR. From looking at the code I think it will be much simpler to restore this functionality at least temporarily than try to resolve the withCallingHandlers issue that is at the root of all this. I have an initial cut ready but I need to figure out a couple of failing tests. I'm traveling this weekend so I may not be able to get this in until Monday.

[brodieG]

Fixed by https://github.com/r-lib/testthat/pull/1183