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

Made (Java)ExecFork implement ProcessForkOptions #15

Merged
merged 13 commits into from Jan 9, 2019

Conversation

Projects
None yet
2 participants
@DreierF
Copy link
Contributor

DreierF commented Dec 4, 2018

Hey there,

the pull request makes the two tasks implement Gradle's ProcessForkOptions interface. This allows us to attach JaCoCo to a JavaExecFork task. Furthermore this fixes #8.
I also added the annotation for #14. Most of the changes are however just formatting.

To be consistent with the ProcessForkOptions interface I removed the commandLine property in favour of executable.

Would be really nice if we could get this merged :)

@psxpaul
Copy link
Owner

psxpaul left a comment

Thanks for the PR! One other quick suggestion - can you either update the with an example Jacoco configuration? Or better yet, add a sample project for this.

Also, this is very minor, but I personally don't like type inference in most places. I appreciate all of the formatting changes, except a couple places where the explicit type was removed.

@DreierF

This comment has been minimized.

Copy link
Contributor Author

DreierF commented Dec 28, 2018

Sorry it took me so long to address the review. I re-added the explicit types as suggested.
Adding a sample project was indeed a good idea, because it revealed some more problems:

  • up-to-date check was no longer properly working when applying jacoco (see comment in code)
  • no coverage was generated, because the forked process was killed instead of terminated, which did not trigger JaCoCo's shutdown hooks to write coverage to disk

Both are fixed now 🙂

I also updated gradle to 5.0 and adjusted the plugin to use the new API's where necessary. So the plugin will now require Gradle 4.10.0 or newer.

@DreierF

This comment has been minimized.

Copy link
Contributor Author

DreierF commented Jan 8, 2019

Any chance you could have a look at the changes?

@psxpaul

psxpaul approved these changes Jan 9, 2019

Copy link
Owner

psxpaul left a comment

Changes look good to me. Thanks for doing this!

@psxpaul psxpaul merged commit ac5e0e9 into psxpaul:master Jan 9, 2019

@DreierF

This comment has been minimized.

Copy link
Contributor Author

DreierF commented Jan 9, 2019

Thanks a lot 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment