-
Notifications
You must be signed in to change notification settings - Fork 5k
Jacoco documentation: fix a small issue #14519
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
Conversation
Also use apis for configuration avoidance Signed-off-by: Taso Dane <[email protected]>
I'd rather document the performance impact over removing the suggestion. Can you instead update the documentation with a better solution? |
I cannot see how it can be improved since it is just simply unnecessary. With my suggestion, it is simpler. If you want jaco report, you simply run jaco tasks and they automatically depend on tests. All works fine. If you have any suggestions, I'm open to change it. |
🤔 Perhaps suggest that they attach the jacoco tasks to |
@big-guy your thoughts here? |
That's a good idea. I will change it. |
Signed-off-by: Taso Dane <[email protected]>
c385bf6
to
9919f3e
Compare
Thanks @tasomaniac! |
@bot-gradle test SanityCheck plz |
OK, I've already triggered SanityCheck build for you. |
@bot-gradle test QuickFeedback |
OK, I've already cancelled the old build and triggered a new QuickFeedback build for you. |
This pull request has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be closed if no further activity occurs. If you intend to work on this pull request, please reopen the PR. Thank you for your contributions. |
Let's keep this open. It is small but very useful documentation change |
Any updates on this? Can someone trigger the CI again? There is not much reason why this would fail in the CI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make snippet changes in the context of where this is included.
See https://docs.gradle.org/current/userguide/jacoco_plugin.html#sec:jacoco_getting_started and the text before example 2
// tag::testtask-dependency[] | ||
test { | ||
finalizedBy jacocoTestReport // report is always generated after tests run | ||
tasks.named("check").configure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes make no sense in isolation. The text before the inclusion of that snippet should be adapted as well then.
Another option is to make it clearer here that what is proposed is optional
I'm closing this PR as there hasn't been any progress for quite a while now. @tasomaniac let me know if you are still interested in finishing this work. |
Context
While integrating the Jacoco plugin into our repo, I realized a small build performance issue with the suggestion provided in the docs.
When
test
task is always finalized withjacocoTestReport
, it causes jacoco report to be generated when not needed. Example: Recent versions of IntelliJ IDEA delegates to Gradle to run a single test. This is done during development workflow and supposed to be very fast. In this case, we don't need to generate jacoco report every time a single test is run.Solution
Remove the suggestion to make tests being finalized by jacoco task. Making
jacocoTestReport
depend on the test task should be enough.Also change the example usage to use Task configuration avoidance APIs.
Contributor Checklist
<subproject>/src/integTest
) to verify changes from a user perspective<subproject>/src/test
) to verify logic./gradlew <changed-subproject>:check
Gradle Core Team Checklist