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

Support Jacoco XML report #86

Closed
wants to merge 6 commits into from
Closed

Conversation

peterg79
Copy link
Contributor

@peterg79 peterg79 commented Oct 25, 2018

Implement support for jacoco code coverage (xml output, not the exec format)
XML Report DTD can be found here: https://www.jacoco.org/jacoco/trunk/doc/

Addresses issues #80 and #82

@peterg79 peterg79 changed the title support Jacoco XML report DO NOT MERGE YET - support Jacoco XML report Oct 25, 2018
@coveralls
Copy link

coveralls commented Oct 26, 2018

Coverage Status

Coverage increased (+0.4%) to 99.073% when pulling 6549858 on peterg79:jacoco into 7d80023 on Bachmann1234:master.

@peterg79 peterg79 changed the title DO NOT MERGE YET - support Jacoco XML report Support Jacoco XML report Oct 26, 2018
@Bachmann1234
Copy link
Owner

Neat! Thanks for this. I'll try to look at it this weekend

elif jacoco_xml_roots:
coverage = JacocoXmlCoverageReporter(jacoco_xml_roots, src_roots)
else:
assert False
Copy link
Owner

Choose a reason for hiding this comment

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

Whats happening here? Should we be raising something specific?

for root in self._src_roots:
_files = [_file
for _file in pkg.findall('sourcefile')
if GitPathTool.relative_path(os.path.join(root, pkg.get('name'), _file.get('name')))
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick, but perhaps making this a named method can make it clearer what this is doing.
maybe something like _get_measured_source_path(src_root, package_name, file_name)

# First case, need to define violations initially
if violations is None:
violations = set(
Violation(int(line.get('nr')), None)
Copy link
Owner

Choose a reason for hiding this comment

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

any idea what 'nr' stands for? Can it be a constant?

@Bachmann1234
Copy link
Owner

This is looking pretty good. Thank you! I left a couple small comments.

@peterg79
Copy link
Contributor Author

Thanks! Comments addressed. Let me know if you need more changes. Please see #87 - I don't think this should be a separate release.

@peterg79 peterg79 deleted the jacoco branch October 30, 2018 16:58
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.

None yet

3 participants