Report @Ignore tests in xml reports from JUnit and create report for tests that fail in initialization

Review Request #3571 — Created March 15, 2016 and submitted

cheister
pants
add-ignore-junit-xml-reports
3044
pants-reviews
gmalmquist, jsirois, zundel
Report @Ignore tests in xml reports from JUnit and create report for tests that fail in initialization

Travis CI: https://travis-ci.org/pantsbuild/pants/builds/116726375

This PR adds @Ignore tests to the TEST_*.xml reports generated by the ConsoleRunner for testing. And generates the xml report when the testSuite fails during initialization. It also adds several test cases for the AntJunitXmlReportListener.

Old report with @Ignored test

<testsuite name="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" errors="0" failures="0" hostname="cheister.local" tests="1" time="0.000278" timestamp="2016-03-17T10:14:47">
<testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlPassing" time="0.000177"/>
</testsuite>

to

<testsuite name="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" errors="0" failures="0" skipped="1" hostname="cheister.local" tests="2" timestamp="2016-03-17T01:27:13">
<testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlPassing" time="0.000120"/>
<testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlIgnored" time="0">
<skipped></skipped>
</testcase>
</testsuite>

For tests that fail during initialization there used to be no TEST-*.xml file created at all, now we'll get a file with one testcase that represents the whole TestSuite and has an error with the stacktrace from the failure.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. I noticed that the travis-ci build has some test failures. I looked over the change and I'm not sure why but let me know if you want to pair up on it.

  2. 
      
  1. 
      
  2. I sniffed around a bit for why this would be the case.

    In ConsoleRunnerImpl, if the -xmlreport argument is not active, then the path won't automatically be created. So I'm guessing that with -xmlreport first, it isn't actually getting set in the Options object. If that is what is going on, you could also workaround this by creating the directory 'outdirPath' before invoking the console runner.

    There is a more recent version of the args4j artifact (2.33) that we could upgrade to, but I looked through the commit history and there didn't seem to be any bug that was fixed.

  3. What is the reason for this change? Also, I noticed that you changed this to a mutable list, but I don't see anywhere that we intend to change the values.

    1. If the properties are an ImmutableList then JAXB can't populate the object from XML because clear throws an UnsupportedOperationException. This only happens in tests right now because that is the only place where we unmarshal an XML file into a AntJunitXmlReportListener.TestSuite.

      Here is the stack trace

      java.lang.UnsupportedOperationException
                              at com.google.common.collect.ImmutableCollection.clear(ImmutableCollection.java:156)
                              at com.sun.xml.internal.bind.v2.runtime.reflect.Lister$CollectionLister.startPacking(Lister.java:284)
                              at com.sun.xml.internal.bind.v2.runtime.reflect.Lister$CollectionLister.startPacking(Lister.java:253)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.Scope.start(Scope.java:127)
                              at com.sun.xml.internal.bind.v2.runtime.property.ArrayERProperty$ItemsLoader.startElement(ArrayERProperty.java:104)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.UnmarshallingContext._startElement(UnmarshallingContext.java:559)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.UnmarshallingContext.startElement(UnmarshallingContext.java:538)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.SAXConnector.startElement(SAXConnector.java:153)
                              at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.startElement(AbstractSAXParser.java:509)
                              at com.sun.org.apache.xerces.internal.impl.XMLNSDocumentScannerImpl.scanStartElement(XMLNSDocumentScannerImpl.java:380)
                              at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl$FragmentContentDriver.next(XMLDocumentFragmentScannerImpl.java:2787)
                              at com.sun.org.apache.xerces.internal.impl.XMLDocumentScannerImpl.next(XMLDocumentScannerImpl.java:606)
                              at com.sun.org.apache.xerces.internal.impl.XMLNSDocumentScannerImpl.next(XMLNSDocumentScannerImpl.java:118)
                              at com.sun.org.apache.xerces.internal.impl.XMLDocumentFragmentScannerImpl.scanDocument(XMLDocumentFragmentScannerImpl.java:510)
                              at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:848)
                              at com.sun.org.apache.xerces.internal.parsers.XML11Configuration.parse(XML11Configuration.java:777)
                              at com.sun.org.apache.xerces.internal.parsers.XMLParser.parse(XMLParser.java:141)
                              at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1213)
                              at com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser.parse(SAXParserImpl.java:643)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.UnmarshallerImpl.unmarshal0(UnmarshallerImpl.java:243)
                              at com.sun.xml.internal.bind.v2.runtime.unmarshaller.UnmarshallerImpl.unmarshal(UnmarshallerImpl.java:214)
                              at javax.xml.bind.helpers.AbstractUnmarshallerImpl.unmarshal(AbstractUnmarshallerImpl.java:157)
                              at javax.xml.bind.helpers.AbstractUnmarshallerImpl.unmarshal(AbstractUnmarshallerImpl.java:162)
                              at javax.xml.bind.helpers.AbstractUnmarshallerImpl.unmarshal(AbstractUnmarshallerImpl.java:171)
                              at javax.xml.bind.helpers.AbstractUnmarshallerImpl.unmarshal(AbstractUnmarshallerImpl.java:189)
      
  4. Thank you for getting in here and expanding test coverage! Knowing which callbacks get invoked in error scenarios is very tricky.

  5. Please add standard header here and on all other new source files

    // Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
    // Licensed under the Apache License, Version 2.0 (see LICENSE).
    
  6. You didn't add this one, but I noticed it was also missing a header. Could you please add one?

  7. Did you mean to leave this test commented out? Its already marked @Ignore

    1. No, I didn't mean to leave that change in. It's been removed.

  8. 
      
  1. Can you include/attach some examples of how you expect the output to change? This is backwards compatible, right?

    1. Yes, the intent is it is backwards compatible. There was some logic in the AntJunitXmlReportListener that seemed odd but no comments or tests to explain why it was done. I tried to add tests of all the basic cases and ran it against some of the larger test suites to doublecheck the changes.

      For the @Ignore tests the relevant parts of the TEST-*.xml file that will change are:

      Old report with @Ignored test

      <testsuite name="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" errors="0" failures="0" hostname="cheister.local" tests="1" time="0.000278" timestamp="2016-03-17T10:14:47">
          <testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlPassing" time="0.000177"/>
      </testsuite>
      

      to

      <testsuite name="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" errors="0" failures="0" skipped="1" hostname="cheister.local" tests="2" timestamp="2016-03-17T01:27:13">
          <testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlPassing" time="0.000120"/>
          <testcase classname="org.pantsbuild.tools.junit.impl.XmlReportFirstTestIngoredTest" name="testXmlIgnored" time="0">
              <skipped></skipped>
          </testcase>
      </testsuite>
      

      For tests that fail during initialization there used to be no TEST-*.xml file created at all, now we'll get a file with one testcase that represents the whole TestSuite and has an error with the stacktrace from the failure.

  2. 
      
  1. Ship It!
  2. 
      
  1. LGTM! I have a few minor nitpicks, but this is so much clearer.

  2. nit: period at end of sentence.

  3. nit: period at end of sentence.

    Also, is it that ignored tests don't have those callbacks or that junit doesn't call testStarted and testFinished for tests that have been ignored?

  4. Why were suite and testcase re-ordered? Does it matter?

    I feel like the test case error is a little more descriptive than the suite one. It also makes sense to me that the suite would be finished after the case it contains.

    1. I moved them so that testFinished, testIgnored and testFailure all have similar logic of testing the TestSuite and then the TestCase. Switching it back for testFinished is ok by me.

  5. Is the null check for time important? It's inconsistently applied in other places that test for "0" time values. Maybe it'd be worth extracting a helper assertion method?

    1. Yeah, I want to test that is not null and not equal to "0". The intent being that it is something greater than 0. Probably a better assertion would be to convert it to a float and make sure it is greater than 0.

  6. 
      
  1. Ship It!
  2. 
      
  1. Thanks Stu, Nick, Garrett and Chris for the update. This is in master @ 4b90ab1. Please mark this RB as submitted and close associated github issues.

  2. 
      
Review request changed

Status: Closed (submitted)

Loading...