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

Adds name attribute to <testcase> elements in junit output (#575) #576

Merged
merged 9 commits into from
Feb 18, 2017

Conversation

akbertram
Copy link
Contributor

@akbertram
Copy link
Contributor Author

Fixes Issue #575

@akbertram
Copy link
Contributor Author

@hadley I believe I've fixed the test failures but it looks like neither AppVeyor nor Travis are rebuilding.

Perhaps a different version of libxml is being used than to
generate the original output.
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Do the checks pass for you locally?

@@ -89,7 +89,8 @@ JunitReporter <- R6::R6Class("JunitReporter", inherit = Reporter,
name <- test %||% "(unnamed)"
testcase <- xml2::xml_add_child(self$suite, "testcase",
time = toString(time),
classname = paste0(classnameOK(context), '.', classnameOK(name))
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind fixing the indenting while you're in here?

@akbertram
Copy link
Contributor Author

@hadley When I run test_package("testthat") all tests pass. But if I run devtools::check() . This seems to be because the junit reporter includes stack depth in the error message, which can vary depending on how the test is invoked:

 <failure type="failure" message="expectation_failure in structure(list(fail()), start_frame = 19): Failure has been forced&#10;&#10; @reporters/tests.R#8"/>

I would suggest removing the start_frame attribute from the output anyway, it makes the deparsed function call harder to read?

@hadley
Copy link
Member

hadley commented Feb 17, 2017

Yeah, that looks like a mistake anyway - "structure(list(fail()), start_frame = 19)" looks like a deparsed object, not something deliberately included. Can you remove it?

@hadley
Copy link
Member

hadley commented Feb 17, 2017

I can't really follow the logic of the code that currently generates the message, and it's not clear to me what it should be, so I trust your judgement as to what would be more helpful.

-  "message" attribute mapped directly to expectation message
- failure/error body set to format(expectation)

This produces the same output as the summary reporter for
errors/failures.
Matched the stack depth numbers to those output by the
summary reporter.
 This adds two options:
    * testthat.default_check_reporter: allows configuration of a different reporter
      to use for test_check(). For example, 'junit'
    * testthat.junit.output_file: specifies a file to which the junit xml should
      be written.

These are needed so that Ci systems can run test scripts in R packages
without modification, but first set these options() so that test output will
be written to a known location where it can be read and parsed.
@akbertram
Copy link
Contributor Author

Phew. That seems to work now. I've added one more commit that allows CI systems to override the reporter used for test_check() and specify and output location for the junit reporter.

@hadley
Copy link
Member

hadley commented Feb 17, 2017

Looks good. Now just needs a bullet in NEWS.md, and I can merge.

@hadley hadley merged commit cb7cae0 into r-lib:master Feb 18, 2017
@hadley
Copy link
Member

hadley commented Feb 18, 2017

Thanks!

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.

2 participants