Prevent adding both skipped and failure elements

[gchappel]

This is a potential fix to prevent both skipped and failure elements on a skipped violation in JUnit output format

Fixes: #1122

[patilpankaj212]

Hello @gchappel, thanks for creating the PR. Can you please fix the failing test?

[gchappel]

Hello @gchappel, thanks for creating the PR. Can you please fix the failing test?

I'll take a look - it wasn't failing when I opened the PR but leave it with me.

[gchappel]

I took a very quick look at the failing test, and it seemed weird that a test would fail when I only changed the behaviour of one specific output writer (and it's associated test).

So I checked out master, and the same tests are failing there too. I'm going to see if I can find a solution for it, but it may be the case that you need to get someone more familiar with the project to investigate this on master and then I can rebase this PR branch on that.

[gchappel]

OK, I think I got it.

When I opened this PR (Jan 21st) I am 99% sure that make unit-tests was passing on my laptop. Between that time and the time the tests were run here, it appears that something changed which needed this commit to address. When I re-tested master I was accidentally running from master in my own fork which was only up to date as of Jan 11th. Pulling master from origin allowed the unit tests to pass again, and rebasing this PR branch has fixed the tests here as well.

It seems like the tests may pull in external resources which are not under version control? It seems like there is some test data which changed and was no longer compatible with historical commit b0259e8b0650a2a4b9a6a6f3b8ca31745cfa9515, but only with 52e4cdb27e246e2210417bd53b2f58a5279dd135 and later.

edit - yep, tests check out some artifacts from https://github.com/accurics/KaiMonkey without specifying any versions, so checking out old commits and running make unit-tests can fail even if it passed previously because the test data has changed and the tests always pull the latest version. Perhaps some form of versioning could be used on this so that it remains possible to check out an old commit for testing and still be able to run the unit tests because they will download test data which is known to be compatible, even if it is not the latest.

[gchappel]

Thanks @patilpankaj212 for re-running the tests; once again, the KaiMonkey project has moved on, and they fail. So I will rebase on master again, and re-iterate my request that these external artifacts are somehow versioned to avoid these spurious test failures in future.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[codecov]

Codecov Report

Merging #1123 (76acfdc) into master (87ce30e) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
- Coverage   79.57%   79.55%   -0.03%     
==========================================
  Files         255      255              
  Lines        7091     7092       +1     
==========================================
- Hits         5643     5642       -1     
- Misses       1114     1115       +1     
- Partials      334      335       +1     

Impacted Files	Coverage Δ
pkg/writer/junit_xml.go	97.36% <50.00%> (-2.64%)	⬇️

[patilpankaj212]

Hello @gchappel, to track the issue with the artifacts being pulled from KaiMonkey, I have created a separate issue - #1142.

Thanks for your contribution.

[gchappel]

Hello @gchappel, to track the issue with the artifacts being pulled from KaiMonkey, I have created a separate issue - #1142.

Thanks for your contribution.

Perfect, hopefully this will make it easier for myself and/or others to contribute in future without having our PRs invalidated by a change outside of this repository in the test data; not to mention making the review process easier for yourselves.