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

Always report the topLevel testcase in JUnit reporting backend #1310

Closed
wants to merge 3 commits into from
Closed

Always report the topLevel testcase in JUnit reporting backend #1310

wants to merge 3 commits into from

Conversation

achamayou
Copy link

Description

This pull request changes the JUnit report to always report topLevel testcase entries in a testsuite (as opposed to the nested ones that are created to represent sections etc), regardless of wether they are empty/passing or not.

There are two reasons for this change:

  1. With the latest Jenkins release, and with the Tutorial example, the JUnit plugin actually does not work and complains that the results of the tests are empty, since it is looking for at least one testcase element. With this change, it correctly parses the xml report, and displays the factorial test.
  2. In general, there are two benefits to reporting passing testcases. One is that Jenkins will display their elapsed time, which is useful when trying to find out where a (passing) suite spends most of its time. The other is it's showing diffs of added and removed tests since last build. And then it's just nice to be able to navigate the table of all tests if you're new to a project.

GitHub Issues

N/A, but happy to open one if it helps.

@achamayou
Copy link
Author

I am not sure why a few of the targets fails, and running them on the same commit in my fork, I haven't been able to reproduce the failures: https://github.com/achamayou/Catch2/pull/2

I am guessing they are transient, and will go away if the build is restarted, which seems to be something that only the repo owners can do?

@codecov
Copy link

codecov bot commented Jun 27, 2018

Codecov Report

Merging #1310 into master will increase coverage by 3.69%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
+ Coverage   78.87%   82.56%   +3.69%     
==========================================
  Files         113      102      -11     
  Lines        3299     4307    +1008     
==========================================
+ Hits         2602     3556     +954     
- Misses        697      751      +54

@achamayou
Copy link
Author

Reconciled against latest master changes. Feedback would be lovely.

@horenmar
Copy link
Member

First, in regards to the build failure, Travis has started having transient failures lately, where the build image errors out while getting the required packages.

Anyway, if I understand the problem correctly, I think this is the wrong way to go about fixing it. Instead, the JUnit reporter should be given all of the finished assertions the way it used to before changes that caused #1264 and #1267.

I will be committing a change to address this soon, can you instead see if that works for you?

horenmar added a commit that referenced this pull request Jul 14, 2018
By opting the JUnit and XML reporters into it, we no longer run
into problem where they underreport the results without `-s` flag.

Related to #1264, #1267, #1310
@achamayou
Copy link
Author

@horenmar thank you for taking a look. I thought the state of things was deliberate, and meant to minimise report size. I wasn't aware that it was an unintended side effect of a recent change, which is why I'd made this change to only report top level testcases on success.

If we can have full details on success instead, I'm certainly very happy with that. I will test your change and confirm that it addresses the issue (but it sounds like it will).

@achamayou
Copy link
Author

@horenmar I have tested the latest revision of master, including your change, and it does report passing tests in junit format in such a way that they are reported accurately in Jenkins. Thank you!

If it was possible to have a release including this change not too far from now, it would be very convenient. If I can help with that in any way, please let me know how.

@achamayou achamayou closed this Jul 17, 2018
@horenmar
Copy link
Member

v2.3.0 is out now.

@achamayou
Copy link
Author

Thank you!

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