Use warning() for "warnings" instead of cat()

scottkosty commented 7 years ago

First of all, thank you to everyone working on this package! I find it very useful.

I prefer to turn warnings into errors. (related discussion: http://r.789695.n4.nabble.com/Turn-warnings-or-notes-into-errors-on-CMD-check-td4698214.html)

However, testthat does not use R's warning mechanism for the warnings it prints. I think it would be better to rely on R's centralized warning mechanism than catting the string "warning".

More details:

R CMD check currently gives me:

[cut]
* checking tests ...
  Running ‘test_no_loess_warnings.R’
  Running ‘test_smoother_stuff.R’
  Running ‘testthat.R’
  Running ‘testz_smoother_output.R’
[cut]

As you can see, there is no warning in the output here. However, testthat does give warnings (see the output at the bottom for the specific warnings given).

If I inspect the created testthat.Rout file, I could see the warnings. But I do not want to manually set up a grep.

testthat could either use the warning() function for each warning. Or it could use cat but at least at the end use the warning() function once, such as warning("there were warnings").

If testthat does not want to use R's warnings for some reason, perhaps it could just use a different word (to me, a "warning" is well-defined in R), or perhaps respect conventions such as the "warn" option. For example, the warn option is ignored in the following:

> options(warn = 2)
> source('testthat.R', echo = TRUE)

> options(warn = 2)

> library(testthat)

> test_check("qmutest", report = "summary")
Loading required package: qmutest
formulas: ...........
invert: ...
utilities: .....................................
shape: ......
ugrid: W.W..W........
formulas: .....................W.W.

Warnings --------------------------------------------------------------------------------------------------------------------------------------------
1. ugrid must have a minimum length (@test_ugrid.R#9) - `not()` is deprecated.

2. ugrid must have a minimum length (@test_ugrid.R#11) - `not()` is deprecated.

3. ugrid should be sorted (@test_ugrid.R#25) - `not()` is deprecated.

4. errors given when unknown terms found but not for interactions (@test_Uterm_loReplace.R#125) - `not()` is deprecated.

5. errors given when unknown terms found but not for interactions (@test_Uterm_loReplace.R#127) - `not()` is deprecated.

DONE ================================================================================================================================================
>

hadley commented 7 years ago

What is your end goal here? I don't think it makes sense to use the built-in warning function, for the same reason that testthat automatically captures stop().

Do you just want the tests to fail overall if there are warnings present?

scottkosty commented 7 years ago

Thanks for the response.

I don't think it makes sense to use the built-in warning function, for the same reason that testthat automatically captures stop().

I see, that makes sense. But what about the second idea I mentioned above, of using the built-in warning at the end if any warnings were captured during any of the tests. Something like

warn("A warning occurred during a test")

Alternatively, whether a test is defined to "pass" or "fail" could depend on the "warn" option. If the "warn" option is 2, then if there is a warning the test "fails".

Do you just want the tests to fail overall if there are warnings present?

Yes, and I would prefer to be able to control that behavior by setting the 'warn' option.

hadley commented 7 years ago

Ok, that makes sense. I'll need to think through if we can just make this work with warn or it would make sense to have another option (but I'm pretty sure we can just make use of warn)

r-lib / testthat

Use warning() for "warnings" instead of cat() #619