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

Capture warnings and messages and print all at end #310

Closed
hadley opened this issue Oct 16, 2015 · 11 comments
Closed

Capture warnings and messages and print all at end #310

hadley opened this issue Oct 16, 2015 · 11 comments

Comments

@hadley
Copy link
Member

@hadley hadley commented Oct 16, 2015

Display on summary as W.

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Oct 16, 2015

👍

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 6, 2015

Perhaps related: Would it be possible to print a dot or something for each evaluated top-level expression (in addition to the dot for each successful expectation)?

@gaborcsardi
Copy link
Contributor

@gaborcsardi gaborcsardi commented Nov 7, 2015

@krlmlr: That would create a lot of dots imo. Why not just put the ones that you want output for in an expectation, e.g. expect_silent?

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Nov 7, 2015

Agreed. There will be more dots, and it could also hurt test performance. I can print a dot myself with expect_null(NULL), too.

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Feb 17, 2016

I think that both warnings and messages should be handled by the reporter, the default handling for messages would be to muffle them. I'm currently struggling with a case where I'd really like to see the messages that occur within a test.

@hadley
Copy link
Member Author

@hadley hadley commented Feb 18, 2016

I think that's reasonable, although I think the default should be to handle messages in the same way as warnings (i.e. with an M and a summary at the end)

@krlmlr krlmlr changed the title Capture warnings and print all at end Capture warnings and messages and print all at end Feb 19, 2016
@hadley hadley closed this in 9828938 Feb 29, 2016
@hadley
Copy link
Member Author

@hadley hadley commented Feb 29, 2016

This is a first stab at it - please let me know if you have suggestions for improvement

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 1, 2016

Thanks. This looks very nice, I like it.

  • Are you also going to handle messages the same way?
  • I've looked at dplyr results, most warnings come from testthat: "info"/"label" has been deprecated. I wonder if these should be handled specially.
  • The warnings that occur outside of test_that() calls are collected separately.
  • Do you plan to treat warnings as errors eventually? I understand that this is not an option for this release.
@hadley
Copy link
Member Author

@hadley hadley commented Mar 1, 2016

  • I think I'll leave messages for now. They seem like more like output where it's reasonable to ignore them by default - otherwise it makes for frustrating testing.
  • I'm as concerned as I used to be about capturing warnings etc that happen outside of tests.
  • You mean treat as test failures? Maybe that could be an option, but I don't see it as being particularly important.
@krlmlr
Copy link
Member

@krlmlr krlmlr commented Mar 1, 2016

  1. I'd appreciate at least an "M" in the SummaryReporter, perhaps verbose output should be "opt-in".
  2. I don't understand this -- does that mean you don't care about what happens outside tests?
  3. I'd argue that warning=failure should be the default, but I fear that many packages that use testthat are not ready for this.
@hadley
Copy link
Member Author

@hadley hadley commented Mar 1, 2016

  1. I'm concerned that even that is too much output for most people.
  2. Right - if it's not in a test, why should testthat be involved?
  3. It's not clear to me that warnings should be failures; regardless, I think it's too late to change now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants