Add support for absolute paths for jacoco exec and report #34544
Conversation
|
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
| @ConfigItem | ||
| public Optional<String> dataFile; |
There was a problem hiding this comment.
What's the rationale of this change?
There was a problem hiding this comment.
I changed the default to be the equivalent of target/jacoco-quarkus.exec, but since I wanted to reuse outputTargetBuildItem.getOutputDirectory() and module.getWorkspaceModule().getBuildDir() in the processor, I could not use a WithDefault anymore.
so now it is optional, but a default is calculated in the processor directly.
there are no changes for people who did not set data-file.
for people that did set data-file=foo, they need to change it to target/foo.
your advice more than welcome.
There was a problem hiding this comment.
Makes sense, thanks.
What I would like however is some Javadoc explaining this process so users who might want to configure it can find this information on our config page (or their IDE if it supports it)
There was a problem hiding this comment.
sure thing, which I did I believe:
/**
* The jacoco data file. By default this will be target/jacoco-quarkus.exec.
* The path can be relative (to the module) or absolute.
*/
@ConfigItem
public Optional<String> dataFile;
do you think this should be more detailed?
| @ConfigItem | ||
| public Optional<String> reportLocation; |
There was a problem hiding this comment.
same reason as for data file
| @@ -11,10 +11,11 @@ | |||
| public class JacocoConfig { | |||
|
|
|||
| /** | |||
| * The jacoco data file | |||
| * The jacoco data file. By default this will be target/jacoco-quarkus.exec. | |||
There was a problem hiding this comment.
Could we use defaultValueForDocumentation instead?
There was a problem hiding this comment.
I didn't remember that at all. @vsevel fancy doing a new PR with this improvement?
There was a problem hiding this comment.
Sure. I will add some info on the guide as well
This changes the
data-fileandreport-locationconfig parameters to allow paths that are not relative to the build directory.we need this to be able to point to a common location for all modules in a multi module project.
today, if we specify a
data-file=jacoco/jacoco-quarkus.execin module foo, thedata-filepassed to jacoco will be<project root>/foo/target/jacoco/jacoco-quarkus.execin a multi-module project, we need the path to be something like /target/jacoco/jacoco-quarkus.exec
the only option we have right now is to use an expression such as
data-file=../target/jacoco/jacoco-quarkus.exec.but that works only for modules that are at the first level. if we have deeper sub-modules (which is the case in extensions), then we have to customize the number of
../../..for each module.This PR allows to configure
data-file=${maven.multiModuleProjectDirectory}/target/jacoco/jacoco-quarkus.execthis is a breaking change on the configuration, since existing configuration
data-file=foowould end up with./fooinstead of./target/foo. if accepted this will need to be properly documented in the migration guide.another option would be to add an additional config parameter
relative-to-multi-module-project-directory=true|false(false by default).see zulip Multi Module Code Coverage With Jacoco and #32254