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

Provide detailed output from Catch #1008

Merged
merged 15 commits into from
Jun 17, 2020
Merged

Conversation

jimhester
Copy link
Member

No description provided.

Copy link
Member Author

@jimhester jimhester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This uses the XML reporter from Catch, and then extracts the successes and failures from the XML file and generates expectations from them. This makes the Catch tests look basically the same as R tests when they are run.

@@ -13,14 +13,14 @@ void ouch() {

} // anonymous namespace

context("Example Unit Test") {
context("Catch: Example Unit Test") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are now showing up in the main testthat results I added a prefix

@@ -123,7 +123,8 @@ inline Catch::Session& catchSession()

inline bool run_tests()
{
return catchSession().run() == 0;
const char* argv[] = {"catch", "-r", "xml"};
return catchSession().run(3, argv) == 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely there must be a better way to do this, but the docs related to this only seem to reference the command line interface (https://github.com/catchorg/Catch2/blob/master/docs/command-line.md#choosing-a-reporter-to-use)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To the best of my knowledge this is indeed the only way to go :(

test_that("Catch unit tests pass", {
expect_cpp_tests_pass("testthat")
})
#test_that("Catch unit tests pass", {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this new style we would get duplicate results if we run it within a test_that() block

failure_srcref <- srcref(srcfile(file.path("src", filename)), c(line, line, 1, 1))

exp <- expectation("failure", org_text, srcref = failure_srcref)
exp$test <- test_name
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly awkward, but the reporters expect a test file as a list element. It is normally added by test_code() at

e$test <- test %||% "(unknown)"

@jimhester
Copy link
Member Author

@kevinushey it would be good to have a second set of eyes look at this, also maybe you know of a way to set the reporter apart from the command line interface.

@jimhester jimhester marked this pull request as ready for review April 9, 2020 20:16
Copy link
Collaborator

@kevinushey kevinushey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packages which have called testthat::use_catch() in the past will already have an old version of tests/testthat/test-cpp.R. Will this update then change (or break) existing tests for those users?

If so, I would propose that we:

  1. Keep expect_cpp_tests_pass() the same as before;
  2. Introduce a new helper function; e.g. run_cpp_tests() or similar that could be called;
  3. Update tests/testthat/test-cpp.R to use the new run_cpp_tests() helper.

This way, I believe users could "upgrade" to the new form by just invoking testthat::use_catch() again.

@@ -21,10 +26,66 @@ expect_cpp_tests_pass <- function(package) {
}
)

# Drop first line of output (it's jut a '####' delimiter)
info <- paste(output[-1], collapse = "\n")
report <- xml2::read_xml(paste(output, collapse = "\n"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in tryCatch() in case Catch produces invalid XML (e.g. maybe something goes wrong during a segfault)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, that is probably safest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I guess if the XML is invalid then xml2 should fail, which is what we want in this case I think?

Copy link
Member Author

@jimhester jimhester Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this happened to me yesterday, when my tests segfaulted which made the xml file not exist and I am not sure of the best way to handle it. Even if we throw a normal error here it ends up being caught in the testthat machinery, but not shown, so you see there was a warning and an error, but not what they were.

I guess we could throw a registered error like we do below...

I think the reason it gets swallowed is because no context has started, maybe we should have a separate context just for the overall catch run to handle this case?

R/test-compiled-code.R Outdated Show resolved Hide resolved
@hadley hadley merged commit b684f1e into r-lib:master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants