Recursively check for errors/failures in produced JUnit result XMLs

[ngmor]

I added a recursive check for errors/failures in JUnit result XMLs for the ament_add_test() runner to address the issue described here.

Signed-off-by: Nick Morales [email protected]

[clalancette]

We need to check the interaction of this with colcon test and colcon test-result, and also with Jenkins for our CI jobs.

[cottsay]

I'm a bit conflicted about where to go from here.

My first thought was that an unbounded recursive traversal was overkill to just descend 1 extra level deep. Then I was reading the documentation you linked and it mentions:

A test case is considered successful (passed) unless there is another result element underneath it. Most tools support the result elements skipped, failure and error.

So it seems that a valid JUnit file need not include any errors or failures attributes at all. This (frustratingly) means there could exist a valid JUnit file which contradicts itself, reporting a testsuite with 0 errors via the attribute but a testcase with a error child. It also means that a recursive crawl through the whole tree is the only way to know for sure that there aren't any errors or failures. Maybe we could optimize a little by not descending into a testsuite which has an attribute for errors and/or failures, which would also give us a clear answer about what to do if there is a contradiction.

---

I definitely agree that we should make a change here. If your goal is only to support Catch2's outputs, I think we should punt the recursive crawl and instead add an explicit check for skipping the testsuites root element.

[ngmor]

@cottsay apologies for the delay on this. I like your suggestion to limit recursion and select a clear answer in cases of contradiction, since that optimizes but also (hopefully) gives us a little more flexibility than just Catch2 JUnit XMLs. I've implemented that in my latest commit. Let me know what you think.

[ngmor]

@cottsay bump on this, let me know if you have any thoughts on my lastest update.

[cottsay]

This seems like a reasonable improvement. I'm still not thrilled about the recursive traversal, but that's the specification's fault not yours.

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[ngmor]

Sorry, this is my first time making a contribution to a ROS repository. Is there anything else that needs to be done on my end to close this out?

[cottsay]

Is there anything else that needs to be done on my end to close this out?

No, it's on me to get this merged. CI passed, but I'd like to do some manual smoke testing to ensure that test failures continue to surface correctly.

[cottsay]

No regressions that I can see, thanks!

[ngmor]

Any chance this could be backported to Iron and/or Humble? I've got a package that relies on a custom test runner (which is essentially just the change I committed here) as a workaround for this issue in those distros.

[cottsay]

@clalancette, how do you and/or the ament_cmake maintainers feel about this backport? I view it as a feature and I think it would be OK, but it walks the line.

[clalancette]

@clalancette, how do you and/or the ament_cmake maintainers feel about this backport? I view it as a feature and I think it would be OK, but it walks the line.

I think it would be OK to backport to both; it doesn't change API, and enables downstream users.