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

Junit reporter #481

Merged
merged 50 commits into from Dec 19, 2016

Conversation

Projects
None yet
10 participants
@lbartnik
Contributor

lbartnik commented May 28, 2016

Hi @jspitzen, @hadley

I've rewritten the JUnit reporter to match the new API. I understand xml2 and its own tests are missing, I'll work on that in the coming days. In the meantime, could you please let me know if there is anything missing/flawed as far as JUnit/testthat designs go?

I want to integrate testthat with JUnit/XML output but I'm not a JUnit expert.

Lukasz

floybix and others added some commits Sep 7, 2011

First cut at JUnitReporter.
Need to rethink the mapping onto jUnit structure.
Currently test identifiers change if they fail.
Use expect_that() call as test name/identifier.
Works for expect_that() but not expect_equals() etc.
newJUnitReporter() wrapper function;
fix broken substitution of info arg;
a pseudo-test of info arg.
Merge remote-tracking branch 'floybix/master' #1
Conflicts:
	DESCRIPTION
	NAMESPACE
fix basic runtime issues
- change name to JunitReporter to make it searchable by "junit"
- add a default output file name
- fix outdated API calls

@lbartnik lbartnik referenced this pull request May 28, 2016

Closed

jUnit reporter #260

@codecov-io

This comment has been minimized.

codecov-io commented May 28, 2016

Current coverage is 79.88% (diff: 100%)

Merging #481 into master will increase coverage by 1.28%

@@             master       #481   diff @@
==========================================
  Files            56         57     +1   
  Lines          1668       1735    +67   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1311       1386    +75   
+ Misses          357        349     -8   
  Partials          0          0          

Powered by Codecov. Last update 46d15da...c095ae3

@lbartnik

This comment has been minimized.

Contributor

lbartnik commented May 30, 2016

Hi, @hadley given your comment I have taken a look at xml2 and it seems that this package does not have a convenient API to build a XML document the way you can do it with XML. Any hints on how to replace XML with xml2 in this case?

@hadley

This comment has been minimized.

Member

hadley commented Dec 15, 2016

Could you please merge/rebase to clean up the commit history? Then I'll do a more thorough review.

Also need to add to test-reporter.R for regression tests.

I'm planning for a release early next year so it would be great if we can (finally) get this in.

@hadley hadley referenced this pull request Dec 15, 2016

Closed

Exit code once a test fails #515

lbartnik added some commits Dec 15, 2016

@lbartnik

This comment has been minimized.

Contributor

lbartnik commented Dec 15, 2016

I've merged but I'm not sure if that's enough to "clean up the commit history". Can you elaborate what exactly would be your goal? I could copy the current diff into a new branch and that way you'd have a single commit with no history there - but I thought you would rather keep track of all actual contributors.

I've also added a few lines to test-reporter.R but now I'm not sure if the current design is correct. By default this reporter writes to a file ./report.xml and the standard output gets something much more simplified and human-readable. But maybe the XML should be displayed directly on stdout and the file should be an option that's not used by default?

@hadley

You have 37 commits which seems rather a lot but if you don't want to squash the commits, I'll simply do it when I merge it.

I agree that you should be outputting the xml, not the summary reporter results.

}
#' @importFrom xml2 xml_attr<- xml_add_child
add_broken <- function (parent, type, message) {

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

I'd prefer that you not import these functions and instead use fully qualified calls.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Can I ask why?

#' This reporter includes detailed results about each test and summaries,
#' written to a file (or stdout) in jUnit XML format. This can be read by
#' the Jenkins Continuous Integration System to report on a dashboard etc.
#' Requires the XML package.

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

"xml2"

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

#' the Jenkins Continuous Integration System to report on a dashboard etc.
#' Requires the XML package.
#'
#' This works for \code{\link{expect_that}} but not for the wrappers like

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

Why not?

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

I don't know... I think I must have left it since the original (previous) pull request.

#' References for the jUnit XML format:
#' \url{http://llg.cubic.org/docs/junit/}
#'
#' Output drawn from the SummaryReporter is printed to the standard

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

One reporter should do one thing, so I agree that you should drop this and simply output xml

self$suite_time <- self$suite_time + time
# XML node for test case
testcase <- xml_add_child(self$suite, "testcase")

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

Can eliminate add_attrs in favour of:

testcase <- xml_add_child(self$suite, 
  "testcase",
  time = time,
  classname = paste0(classnameOK(context), '.', classnameOK(name)))
)

(And please note my preferred indenting style)

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

By the way, I used the default RStudio indentation setting.

# XML node for test case
testcase <- xml_add_child(self$suite, "testcase")
name <- if (is.null(test) || !nchar(test)) "(unnamed)" else test

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

Please use name <- test %||% "(unnamed)"

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

add_broken(testcase, 'error', message)
self$errors <- self$errors + 1
}
else if (expectation_failure(result)) {

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

Please more else to previous line

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

#' @importFrom xml2 xml_attr<- xml_add_child
add_broken <- function (parent, type, message) {
child <- xml_add_child(parent, type)

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

You can assign the attributes during creation

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

@@ -14,6 +14,7 @@ install:
build_script:
- travis-tool.sh install_deps
- travis-tool.sh install_r Rcpp

This comment has been minimized.

@hadley

hadley Dec 15, 2016

Member

Why is this necessary?

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

AppVeyor's build would fail on Windows without this - Rcpp wouldn't be installed. I don't know why but this fixed the problem.

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

@krlmlr any ideas?

This comment has been minimized.

@jimhester

jimhester Dec 19, 2016

Member

Rcpp is not being installed probably due to r-lib/devtools#1246 or possibly r-lib/devtools#1409. Use devel devtools in appveyor should fix it.

@hadley

Getting close. Thanks!

NAMESPACE Outdated
@@ -131,6 +132,10 @@ importFrom(crayon,green)
importFrom(crayon,red)
importFrom(crayon,yellow)
importFrom(magrittr,"%>%")
importFrom(xml2,"xml_attr<-")

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

These can go away now

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

self$cat(context, ": ")
self$suite <- xml2::xml_add_child(self$root, "testsuite")
add_attrs(self$suite,

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Could now be inlined into call above, and add_attrs() deleted, right?

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

@@ -14,6 +14,7 @@ install:
build_script:
- travis-tool.sh install_deps
- travis-tool.sh install_r Rcpp

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

@krlmlr any ideas?

add_broken(testcase, 'error', message)
self$errors <- self$errors + 1
} else
if (expectation_failure(result)) {

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

if should be on same line as else

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Oh, I didn't get that. That's really hard to read. How come this is your preferred style?

# add child XML node if not success
if (expectation_error(result)) {
self$cat_tight(single_letter_summary(result))

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

I don't think you should be printing these out at all

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

That's deleted in my branch...

if (expectation_error(result)) {
self$cat_tight(single_letter_summary(result))
add_broken(testcase, 'error', message)
self$errors <- self$errors + 1

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

And I don't think you're using errors anywhere, so you can remove.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

I do use them, in end_context

gsub("[ \\.]", "_", text)
}
add_broken <- function (parent, type, message) {

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Don't need assignment any more, and maybe should be add_failure_node()? Are you sure the type is both the node name and an attribute?

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

I followed the previous author here but type should be either the "exception class" (for error) or "type of the assertion" for failure). However, it doesn't seem we have an equivalent value here and XSD requires a type. Do you have a better idea what to put there?

self$cat("\n")
# this causes a segfault write_xml(xmlDoc, self$file, format = )
cat(toString(self$doc), file = self$file)

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

This should just be printed to stdout like all the other reporters.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

What if I'd like to output XML to a file? Do you expect people to capture.output() ?

@lbartnik

This comment has been minimized.

Contributor

lbartnik commented Dec 19, 2016

OK, waiting for the next batch of comments... ;-)

@hadley

Almost there. Just a few little things left.

I think redirecting the output to the right place is outside the scope of testthat. I think usually some other process will be calling R, and it will take care of saving the output to the right place.

@@ -0,0 +1,18 @@
createJunitReporterMock <- function (file) {

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Could you add a brief comment along the lines of "Fix components of JunitReporter that otherwise vary from run-to-run"

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

end_reporter = function() {
if (inherits(self$file, "connection")) {
file <- tempfile()

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Would you mind filing an issue for this on xml2 as well? (It's fine to leave this workaround as is, but long-term xml2 should also supporting writing to connections)

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Submitted under: r-lib/xml2#157

} else if (is.character(self$file)) {
xml2::write_xml(self$doc, self$file, format = TRUE)
}
else {

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Should be on same line as previous.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

context("JUnitReporter")
test_that("Junit reporter regression", {

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Do you think this is needed as well as the saved xml file? It seems a bit redundant to me.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

No, you're right.

@@ -0,0 +1,26 @@
# Fix components of JunitReporter that otherwise vary from run-to-run

This comment has been minimized.

@hadley

hadley Dec 19, 2016

Member

Oh I just noticed this in R/ not tests/testthat. Can you please move there? (And then we're done 🎉)

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Oh, shoot... Sorry for that.

This comment has been minimized.

@lbartnik

lbartnik Dec 19, 2016

Contributor

Done.

@lbartnik

This comment has been minimized.

Contributor

lbartnik commented Dec 19, 2016

It seems I don't need the tempfile() workaround but then XML won't be pretty-formatted. r-lib/xml2#157

@hadley

This comment has been minimized.

Member

hadley commented Dec 19, 2016

Sorry, I forgot to ask for a bullet in NEWS.md — can you please add?

@lbartnik

This comment has been minimized.

Contributor

lbartnik commented Dec 19, 2016

Sure. Is that OK?

@hadley hadley merged commit 3b2f225 into r-lib:master Dec 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Dec 19, 2016

Perfect - thanks!

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