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

Junit reporter #318

Closed
byzantic opened this issue Dec 14, 2019 · 15 comments
Closed

Junit reporter #318

byzantic opened this issue Dec 14, 2019 · 15 comments

Comments

@byzantic
Copy link

byzantic commented Dec 14, 2019

I would just like to add some weight to the requirement for a Junit reporter; I know it's scheduled for the 2.4 release, but currently it is what is stopping me from moving to doctest from Catch2. Otherwise I'm very impressed with doctest.

Junit is a very useful format for interacting with CI systems like Team City, Jenkins, and in my case GitLab.

As such, the output format does NOT need to support all of the Junit schema, in fact it is better to provide the absolute minimum that will be parsed by these systems. By the way, the one that seems to cause the most issues is the 'classname' attribute which needs to be present, even though its not much use if the source language is not Java.

The tricky issues would seem to be:

  1. Dealing with SUBCASES
  2. Junit seems to imply that the test statistics must be emitted as attributes in the opening tag of a testcase; which causes a problem with a lightweight framework, as we dont want to have to store intemediate results.

My thoughts on these points:

  1. Is something specific to Doctest, so I think you are the best person to have an opinion.
  2. This can be sidestepped by NOT emitting the statistics attributes. Most CI frameworks are just interested in detecting a failure tag.

In any event, if we need to emit attribute tags, it is fairly straightforward to build a post-processor program that can be run over the file. I built a simple one using a python package I found somewhere (can't remember the name now).

It looks as though it should be possible to provide a Junit reporter by modifying the example XML reporter; in fact by dropping about 90% of the code! I did a quick trial to confirm this, but it's not suitable for a PR as my understanding of the API and subcases is poor.

@onqtam
Copy link
Member

onqtam commented Dec 16, 2019

Hi!

About the subcases - I guess Catch2 has the same problems because they have sections - perhaps we can see how they handle that?
About the attributes - how does Catch2 handle that? I think some of the reporters there were blocking/buffering - perhaps precisely for this reason. I think it's OK for doctest to do whatever Catch2 does. A post-process is a solution but I think doctest should provide this out-of-the-box.

I'd be happy if someone contributes the JUnit reporter - it should be placed in doctest.cpp right below the XmlReporter.

Currently all tests are executed twice - with the console reporter and the xml reporter - that can be seen here. The JUnit one should be added there as well (with a 3rd invocation) and a new JUNIT_OUTPUT option to the doctest_add_test_impl function. Then you'll need to generate the test output by following the instructions in here. I'd be grateful if someone does this job - I'm not at all familiar with the JUnit format...

@anders-wind
Copy link

@byzantic Can you post a link to your example implementation? I only require quite a limited featureset so just having the simple reporter would be fine for me.

@anders-wind
Copy link

anders-wind commented Jan 7, 2020

@onqtam I recall reading somewhere that you do not allow any imports of the standard library in doctest for compile-time reasons. Is that correct? it seems that there are some headers in doctest.h (vector, atomic and so on).
I'm working on a junit reporter and for now I am using some std::string methods (substr, find_last_of). Since the JUnit format does not support nested test_cases I also use std::vector for storing a stack of subcases (std::stack would probably more describing but since std::vector already was included i used that one).
There a number of optional fields on junit elements which are not available at creation time, but I hope that an initial PR without these will be fine.

EDIT: Just read the previous comments more thoroughly. I have not looked at how Catch2 handles subcases. My approach is as follows: when we hit a subcase, we stop the current test_case tag, store the test object on a stack and starts a new one corrosponding to the new subcase. When we exit a subcase we set the current testcase to the one on the top of the stack.
This flattens the structure but keeps the assertion results in the relevant cases. Only problem is that the test_case name will appear multiple times (though the same asserts will not).

As for the stats, I do not have a good idea ;)

@onqtam
Copy link
Member

onqtam commented Jan 30, 2020

@Awia00 Sorry for the late reply - been really busy lately.

The doctest.h header is actually constructed from doctest_fwd.h and doctest.cpp (from the parts folder) which are just stitched together - there are no includes in the interface part (_fwd.h) and that's why doctest compiles so fast - all of the includes are in the .cpp. Since the reporter would end up in the .cpp right after the XmlReporter class it would be fine to use any headers there.

About the statistics which should be as attributes in the opening tags - I guess Catch2 buffers the results for each test case and emits the right opening tag after the test case has completely finished executing. If such an attribute is necessary for the root XML tag - then I guess everything can be buffered - we should do whatever Catch2 does in this regard.

@dhoer
Copy link

dhoer commented Apr 20, 2020

@onqtam While the reporter tool is wonderful for transforming output to data formats like xml or yaml, having a junit reporter in doctest is really a violation of separation of concern pattern. Junit xml is not formally defined and has many xml schema definitions to define it. Looking at Jenkins xUnit plugin (https://plugins.jenkins.io/xunit/), they went with accepting 2 Junit xml formats, but even that has cause some heartburn: https://issues.jenkins-ci.org/browse/JENKINS-52152.

I would like to propose changing the roadmap to remove junit, but add a doctest xml schema definition (xsd) instead. This would allow for a clearly defined contract for downstream consumers to develop their own flavor of xml transformation.

Example:

${test_binary} -s -d --reporters=xml --out=/tmp/doctest.xml
$xmllint --noout --schema doctest.xsd --path=/tmp/doctest.xml

Users can lint and verify that the doctest output is valid. This helps downstream xml transform developers with troubleshooting issues (it's nice to know if the problem is on the transform side or doctest side). Now users can transform the xml using a XSL Transformations (xslt) and verify output:

$xsltproc doctest-cpp-to-ant-junit.xslt /tmp/doctest.xml > /tmp/junit.xml
$xmllint --noout --schema ant-junit.xsd --path=/tmp/junit.xml

This approach keeps doctest focused on doctest and not get bogged down in the discussion of what junit flavor(s) should doctest support.

What are your thoughts about this approach?

@onqtam
Copy link
Member

onqtam commented Apr 20, 2020

@dhoer I usually try to leave complexity outside of doctest and this approach sounds reasonable indeed, however I do think that there should be some default JUnit reporter. But it wouldn't hurt to define a schema for the XML format! Can't promise a date though...

@dhoer
Copy link

dhoer commented Apr 20, 2020

@onqtam I got started on looking at Junit when I had issues with getting doctest reporting to work with jenkins xunit plugin. Here is the jist of those issues:


There is doctest-junit-report tool (https://github.com/ujiro99/doctest-junit-report) that takes raw output from docktest and converts it to xml . But there is a problem with the junit xml that causes the following errors:

cvc-pattern-valid: Value '6.680322' is not facet-valid with respect to pattern '(([0-9]{0,3},)*[0-9]{3}|[0-9]{0,3})*(\.[0-9]{0,3})?' for type 'SUREFIRE_TIME'
cvc-complex-type.2.4.a: Invalid content was found starting with element 'properties'. One of '{testsuite}' is expected.
cvc-complex-type.4: Attribute 'errors' must appear on element 'testsuite'.

So you will have to format the duration time to 3 decimal places, remove properties tag, and add errors attribute to testsuite element. Here is an example of calling doctest with doctest-junit-report and further formating with sed:

${test_binary} -s -d | doctest-junit-report \
    | sed -r 's/(time="[0-9]*\.[0-9][0-9][0-9])([0-9]*?)(")/\1\3/g' \
    | sed '/<properties><\/properties>/d' \
    | sed 's/<testsuite /<testsuite errors="0" /g' > junit.xml

Now you can add a call to xunit in your Jenkins declarative pipeline script:

xunit thresholds: [ skipped(failureThreshold: '0'), failed(failureThreshold: '0') ], tools: [ JUnit(pattern: 'build/test/**/junit.xml') ]

I started wondering if that issue would still be a problem with doctest 2.4. So I thought I would write an xslt this weekend (yes, I might have been guilty of suffering from cabin fever), but I ran into the problem of not knowing if the xml that is coming out from the code I'm working with would hold up as other users write their doctests. Having a formal xsd would help with that concern. I would even be willing to make it public for others to use and maybe work with jenkins xunit plugin peeps to get doctest officially supported in their plugin.

I wish you good luck on supporting junit. I think it will be a headache since there is no formal definition of junit, only interpretations.

BTW, I was pleasantly pleased how easy it was to do xslt (thanks to w3schools). I don't think that should be a problem for developers to write their own xslt, once there is a formal xsd. Especially if there is an xslt example for them to use.

@dhoer
Copy link

dhoer commented Apr 20, 2020

@onqtam are you planning to target a specific flavor of junit like ant-junit (https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd) or surefire (http://maven.apache.org/surefire/maven-surefire-plugin/xsd/surefire-test-report.xsd)?

@onqtam
Copy link
Member

onqtam commented Apr 21, 2020

@dhoer I haven't thought about it yet - I'll most likely mimic what Catch2 is doing though...

@dhoer
Copy link

dhoer commented Apr 21, 2020

@onqtam thanks for the information. I started working with surefire since that is what jenkins xunit ingested doctest-junit-report output as. But then I realized that ant junit would be more appropriate. It seems that catch2 is using that as well. Here is the xsl that I started working on: https://github.com/dhoer/doctest-cpp-to-ant-junit.

I wonder if I can be selfish and ask if doctest xml could be extended to include timestamp, errors, failures, and skipped attributes for testsuite element. It would make the transform logic cleaner and simpler.

@onqtam
Copy link
Member

onqtam commented Apr 22, 2020

@dhoer I would love to, but I really don't have the capacity in the near term, so I wouldn't expect this anytime soon... I'm mostly fixing critical issues in doctest these days (and responding to issues/PRs) - still in the first month of a new job.

@dhoer
Copy link

dhoer commented Apr 24, 2020

@onqtam thanks, I understand. I have xsl done and tested against jenkins via xunit: https://github.com/dhoer/doctest-cpp-to-ant-junit.

When you start working on the junit reporter, feel free to reach out to me to talk about the nuances of junit and jenkins. I tried to document it with the caveats, known issues, and references.

@onqtam
Copy link
Member

onqtam commented May 22, 2020

@phil-zxx wrote an initial version for a JUnit reporter - I might integrate it into the next release (along with tests and CI and whatnot). I'd be happy if others chimed in and gave some feedback as in what's missing and what could be improved with concrete suggestions/code (ping @dhoer ) - I still have little bandwidth for development but it would be great to finally get this JUnit thing done.

@byzantic
Copy link
Author

byzantic commented May 23, 2020

@phil-zxx 's implementation (#376) seems good enough for me. My requirement was just to have a simple implementation, such that it be correctly recognised and parsed by GitLab.

As many people have pointed out, JUnit is not a well-specified format, so unfortunately, we probably need to wait and see whether it also meets the requirements for Jenkins and maybe a smattering of other CI systems.

Or not. Just putting it in as-is at least gets it out there, and if anyone has a problem, they are likely to complain here.

onqtam added a commit that referenced this issue Jun 4, 2020
…few TODOs right above the JUnit reporter class definition which would need to be addressed + the output hasn't been inspected if it's correct or stable across compilers/platforms - WIP! relates #376 and #318 - thanks to @phil-zxx for the implementation and to @byzantic , @ARCRL and @dhoer for their inputs!
onqtam added a commit that referenced this issue Jun 11, 2020
…few TODOs right above the JUnit reporter class definition which would need to be addressed + the output hasn't been inspected if it's correct or stable across compilers/platforms - WIP! relates #376 and #318 - thanks to @phil-zxx for the implementation and to @byzantic , @ARCRL and @dhoer for their inputs!
onqtam added a commit that referenced this issue Jun 26, 2020
…- thanks to @phil-zxx for the implementation and to @byzantic , @ARCRL and @dhoer for their inputs!
@onqtam
Copy link
Member

onqtam commented Jun 26, 2020

Just pushed a finished version of it in the dev branch - will release doctest 2.4.0 probably tomorrow - once the CI passes!

@onqtam onqtam closed this as completed in 1fb630b Jun 27, 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

No branches or pull requests

4 participants