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

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

Closed
scottkosty opened this Issue Sep 16, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@scottkosty

scottkosty commented Sep 16, 2017

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

This comment has been minimized.

Member

hadley commented Oct 1, 2017

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

This comment has been minimized.

scottkosty commented Oct 2, 2017

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

This comment has been minimized.

Member

hadley commented Oct 2, 2017

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)

@hadley hadley added the feature label Oct 2, 2017

@hadley hadley closed this in d73d1d1 Oct 3, 2017

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