Working implementation of jacoco. #4978
Conversation
There was a problem hiding this comment.
Looks good, thanks! A few comments.
Regarding the snapshot: if you can confirm that the artifact is only fetched if someone actually attempts to enable Jacoco (at which point they'd need to fiddle with their ivy settings), then I think we could ship with the snapshot. But please open a pantsbuild ticket about fixing this to a stable version, and point to it from the ivy settings.
| intransitive=True) | ||
| ]) | ||
|
|
||
| # We need to inject the jacoco agent at test runtime |
| classpath=[ | ||
| JarDependency( | ||
| org='org.jacoco', | ||
| name='org.jacoco.cli', |
There was a problem hiding this comment.
The name of the artifact is really org.jacoco.cli? That's pretty odd.
There was a problem hiding this comment.
| if execution_failed_exception: | ||
| self._context.log.warn('Test failed: {0}'.format(execution_failed_exception)) | ||
| if self._coverage_force: | ||
| self._context.log.warn('Generating report even though tests failed.') |
There was a problem hiding this comment.
Message should probably indicate why ("because flag is set").
|
|
||
| report_dir = os.path.join(self._settings.coverage_dir, 'reports') | ||
| safe_mkdir(report_dir, clean=True) | ||
| for report_format in ['xml', 'csv', 'html']: |
There was a problem hiding this comment.
Report format seems like it should be a Subsystem option with a default (or perhaps a list_option that you can specify multiple values for?) People aren't going to need all of these.
There was a problem hiding this comment.
this is consistent with how cobertura currently functions, but i'm open to changing it. what do you think the interaction should be between coverage-open which causes the report to open after execution, and the report format? should open force html and fail if another format is specified? should open attempt to open csv/xml reports if that's what's specified?
| raise TaskError("java {0} ... exited non-zero ({1})" | ||
| " 'failed to report'".format(main, result)) | ||
|
|
||
| def get_target_classpaths(self): |
There was a problem hiding this comment.
Good idea to prefix private properties/methods with _.
| return self.make_multiple_arg('--classfiles', target_paths) | ||
|
|
||
| def get_source_roots(self): | ||
| source_roots = {t.target_base for t in self._targets if Jacoco.is_coverage_target(t)} |
There was a problem hiding this comment.
The set of relevant targets is computed a few times... maybe make it a memoized property, or extract to a method def _relevant_targets(self):
There was a problem hiding this comment.
moved to a property
| source_roots = {t.target_base for t in self._targets if Jacoco.is_coverage_target(t)} | ||
| return self.make_multiple_arg('--sourcefiles', source_roots) | ||
|
|
||
| def make_multiple_arg(self, arg_name, arg_list): |
There was a problem hiding this comment.
This method is a bit obscure... could use a one-line comment.
| # TODO(jtrobec): implement code coverage using jacoco | ||
| pass | ||
| if self._settings.coverage_open: | ||
| report_file_path = os.path.join(self._settings.coverage_dir, 'reports/html', 'index.html') |
There was a problem hiding this comment.
The reports_dir path is computed a few times. Worth extracting to a property?
…on on coverage_force parameter.
…of jacoco when one containing the cli tools becomes available.
|
Merged based on previous green CI. Thanks. |
Problem
Jacoco is now available as an option, but the existing implementation is a no-op stub.
Solution
Filled out the implementation of the jacoco code coverage engine.
Result
Specifying jacoco as the code coverage processor now creates jacoco reports for junit tests. See the attachment for an example of the jacoco output against pants java code:
coverage.zip
One place where I could use advice: what's the best way to handle working with the snapshot?
I'm using the snapshot because it was the only available version of jacoco that included the client tools. I could write a wrapper around the reporting classes, but it seemed best to avoid that if there was already a cli I could use, and there is...just not in any of the release versions. It's not clear when the next release will be (https://groups.google.com/forum/#!topic/jacoco/gd8xD30TDNo), so my feeling is that moving forward with the current snapshot build (as long as it seems to work) is the best bet. However, maybe pants has some space where I could push the current snapshot so we can guarantee a stable version until the next jacoco release hits maven central? Please let me know what seems best.