Skip to content
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

Respect options(warn = -1), fixes #958 #959

Merged
merged 3 commits into from Nov 5, 2019

Conversation

@jeroen
Copy link
Member

jeroen commented Nov 4, 2019

I don't know how to test this. Is there something like expect_no_warning()?

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 4, 2019

expect_warning(foofy(), NA)

(can you please also merge/rebase to resolve some of the spurious check failures?)

NEWS.md Outdated
@@ -34,6 +34,8 @@ This release mostly focusses on an overhaul of how testthat works with condition
should be lazily registered, e.g. with `vctrs::s3_register()`. This
is useful for introducing an experimental error class without
encouraging users to depend on the class in their tests.

* Testthat now respects options(warn = -1) to ignore all warnings.

This comment has been minimized.

Copy link
@hadley

hadley Nov 4, 2019

Member

Can you please acknowledge yourself and the issue as in https://style.tidyverse.org/news.html#acknowledgement

@jeroen jeroen force-pushed the warning branch from 9b05c3d to ce0ba6c Nov 4, 2019
@jeroen

This comment has been minimized.

Copy link
Member Author

jeroen commented Nov 4, 2019

I'm so confused. This PR fixed my real world problem, but the unit tests still fails :/ Would you mind having a look to find a solution that satisfies the test?

@jeroen

This comment has been minimized.

Copy link
Member Author

jeroen commented Nov 4, 2019

The confusing thing is that the bug actually does not appear when in expect_warning() but in the reporter when warnings were raised but not expect_'ed.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 4, 2019

The way this works is nightmarishly complex, so I think this will have to wait until this release, unfortunately.

@jeroen

This comment has been minimized.

Copy link
Member Author

jeroen commented Nov 5, 2019

OK well the fix in this PR at least fixes the reporter problem about uncaught warnings. So I think it's definitely an improvement, and maybe it can be merged?

I'll remove the test, because this actually tests something else than the reporter issue, namely if expect_warning correctly deals with options.

@jeroen jeroen force-pushed the warning branch from 7e1e7b1 to ce0ba6c Nov 5, 2019
# When options(warn) >= 2, a warning will be converted to an error.
# So, do not handle it here so that it will be handled by handle_error.
if (getOption("warn") >= 2) {
if (getOption("warn") < 0 || getOption("warn") >= 2) {

This comment has been minimized.

Copy link
@hadley

hadley Nov 5, 2019

Member

I think maybe you need:

if (getOption("warn") < 0) {
  handled <<- TRUE
  return()
}

I'll try later if you don't get a chance to get to it.

This comment has been minimized.

Copy link
@hadley

hadley Nov 5, 2019

Member

Actually, I'm going to take a look now since this PR is literally the only thing standing in the way of a testthat release 😄

@hadley hadley merged commit fa75bd9 into master Nov 5, 2019
8 of 9 checks passed
8 of 9 checks passed
macOS (3.6)
Details
macOS (devel) macOS (devel)
Details
linux (3.2) linux (3.2)
Details
linux (3.4) linux (3.4)
Details
linux (3.5) linux (3.5)
Details
linux (3.6) linux (3.6)
Details
windows
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Nov 5, 2019

Thanks!

@hadley hadley deleted the warning branch Nov 5, 2019
@jeroen

This comment has been minimized.

Copy link
Member Author

jeroen commented Nov 6, 2019

Awesome thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.