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

Add total line count to coverage paint for programmatic usage #79

Merged

Conversation

beamerblvd
Copy link
Contributor

As Jenkins Pipeline scripts are becoming more prevalent, programmatic usage of plugin results will become more common. In this case, the result of the Cobertura plugin invocation in the pipeline can be used to feed other plugins, or published to code review systems. In support of such ends, CoveragePaint was missing a bit of information: The total number of lines in the file, including non-executable lines. Absent this, our code is having to re-open and re-read every line of every source file which the SourceCodePainter has already opened and read, just to count the total number of lines to send to our code review system along with the coverage paint information. This simple change eliminates the duplicate file reading and makes the Cobertura results more usable in a programmatic way.

NOTE: I had planned to file a feature request for this and THEN submit the pull request for it, but the issue tracking system flagged me as spam and I am still waiting on someone to review it.

@jeffpearce jeffpearce self-assigned this Oct 25, 2017
@jeffpearce
Copy link
Contributor

I want to make sure I understand correctly. This doesn't affect the existing report, but makes the plugin more useful to other plugins? Can you add at least one simple test, even if it's just get/set. Sounds trivial, but it would help make sure a future maintainer doesn't accidentally break it.

@beamerblvd
Copy link
Contributor Author

That's exactly correct, Jeff. I can add a test when I get home from work tonight. Is src/test/java/hudson/plugins/cobertura/targets/CoveragePaintTest.java the correct location for such a test?

@jeffpearce
Copy link
Contributor

Seems like a good place. Thanks for the PR, BTW

@beamerblvd
Copy link
Contributor Author

Alright, Jeff. I added a test and some asserts to the existing test, and the build passed. I believe it's ready to merge if you're good with it.

Copy link
Contributor

@jeffpearce jeffpearce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR!

@jeffpearce jeffpearce merged commit 70db38f into jenkinsci:master Oct 26, 2017
@beamerblvd beamerblvd deleted the add-line-count-to-coverage-paint branch October 26, 2017 13:46
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