Skip to content

Conversation

tasomaniac
Copy link

@tasomaniac tasomaniac commented Sep 15, 2020

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 with jacocoTestReport, 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

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

Also use apis for configuration avoidance

Signed-off-by: Taso Dane <[email protected]>
@JLLeitschuh
Copy link
Contributor

I'd rather document the performance impact over removing the suggestion. Can you instead update the documentation with a better solution?

@tasomaniac
Copy link
Author

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.

@JLLeitschuh
Copy link
Contributor

🤔 Perhaps suggest that they attach the jacoco tasks to check? Maybe?

@JLLeitschuh
Copy link
Contributor

@big-guy your thoughts here?

@tasomaniac
Copy link
Author

That's a good idea. I will change it.

@tasomaniac tasomaniac changed the base branch from master to am-native-client November 18, 2020 23:21
@tasomaniac tasomaniac changed the base branch from am-native-client to master November 18, 2020 23:21
@JLLeitschuh
Copy link
Contributor

Thanks @tasomaniac!

@gradle gradle deleted a comment from JLLeitschuh Nov 19, 2020
@JLLeitschuh
Copy link
Contributor

@bot-gradle test SanityCheck plz

@bot-gradle
Copy link
Collaborator

OK, I've already triggered SanityCheck build for you.

@JLLeitschuh
Copy link
Contributor

@bot-gradle test QuickFeedback

@bot-gradle
Copy link
Collaborator

OK, I've already cancelled the old build and triggered a new QuickFeedback build for you.

@stale
Copy link

stale bot commented Jan 31, 2021

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.

@stale stale bot added the stale label Jan 31, 2021
@tasomaniac
Copy link
Author

Let's keep this open. It is small but very useful documentation change

@stale stale bot removed the stale label Jan 31, 2021
@JLLeitschuh JLLeitschuh requested a review from bmuskalla February 5, 2021 15:30
@jjohannes jjohannes removed the @jvm label Mar 22, 2021
@tasomaniac
Copy link
Author

Any updates on this? Can someone trigger the CI again? There is not much reason why this would fail in the CI?

Copy link
Member

@ljacomet ljacomet left a 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 {
Copy link
Member

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

@donat
Copy link
Member

donat commented Jun 4, 2021

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.

@donat donat closed this Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants