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
Polishing of JaCoCo coverage validation changes #1091
Conversation
We can fully rely on Groovy SAM here.
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.
Thanks @bmuschko.
JacocoCoverageVerification
should be added to the DSL guide- Release notes misspell the new task (
JacocoCoverageVerification
) - In
JacocoReportBase
, it probably makes sense to inline thefileExistsSpec
in the two places it's used instead of making it part of the public API. - WRT documenting the setters, we should. The other classes weren't documented because they used to be Groovy and the documentation was only on the property.
- WRT our conversation on Slack, if we make the verification check a part of the
check
task, we need to make the verification task behave in an incremental way (only run when something has changed). - It would be really nice if the failure message from JaCoCo was in the failure at the end of the build.
Throws any failure as GradleException instead of letting Ant take charge of it.
@Override | ||
protected void configureReport(final GroovyObjectSupport antBuilder, final JacocoViolationRulesContainer violationRules) { | ||
if (!violationRules.getRules().isEmpty()) { | ||
Map<String, Object> checkArgs = ImmutableMap.<String, Object>of("failonviolation", !violationRules.isFailOnViolation()); | ||
Map<String, Object> checkArgs = ImmutableMap.<String, Object>of("failonviolation", violationRules.isFailOnViolation(), "violationsproperty", VIOLATIONS_ANT_PROPERTY); |
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.
Couldn't this always be 'false' and then we wouldn't have to catch the exception above?
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.
An exception would still be thrown from Ant if you provide invalid validation rules and set failonviolation = false
. For more info see test case JacocoPluginCoverageVerificationIntegrationTest.Ant task reports error for unknown field value
.
lgtm |
Addresses review comments made by @big-guy after merging the pull request #983.