Skip to content
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

[WFCORE-3201] Add JaCoCo profile #2736

Merged
merged 1 commit into from Feb 11, 2018

Conversation

marekkopecky
Copy link
Contributor

@marekkopecky marekkopecky commented Aug 22, 2017

Add JaCoCo profile to wf-core TS

PR adds new profile, current default behaviour is not changed.

JBEAP jira: https://issues.jboss.org/browse/JBEAP-12795
WFCORE jira: https://issues.jboss.org/browse/WFCORE-3201
WF-CORE 3.0.x PR: https://github.com/jbossas/wildfly-core-eap/pull/406

pom.xml Outdated
</plugins>
</build>
</profile>
<profile> <!-- surefire.jacoco.args property needs to be set correctly, if coverage profile is not enabled -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a separate profile needed for this, instead of what we do with surefire.jpda.args at L160?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Brian.

surefire.jpda.args property is set in jpda profile directly in
https://github.com/wildfly/wildfly-core/blob/master/pom.xml#L1730

surefire.jacoco.args property is set by JaCoCo plugin:
https://github.com/marekkopecky/wildfly-core/blob/WFCORE-3201/pom.xml#L1804

Although echo from maven-antrun-plugin is able to print this property correctly, JaCoCo plugin is unfortunately unable to set surefire.jacoco.args to the rest of maven with <surefire.jacoco.args/> defined like surefire.jpda.args is defined on L160. The consequence is that surefire JVM and EAP JVM is started without jacoco property and coverage is not stored to exec file.

See this experiment: https://paste.fedoraproject.org/paste/6zSiMYxcu5Y9nj73TYdSEw/raw

@bstansberry bstansberry added ready-for-merge This PR is ready to be merged and fulfills all requirements and removed ready-for-merge This PR is ready to be merged and fulfills all requirements labels Sep 12, 2017
pom.xml Outdated
<target>
<echo>Jacoco jvm args: ${surefire.jacoco.args}</echo>
</target>
</configuration>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this, for debugging properties and profiles, use maven help plugin
help:effective-settings should give you what you want there.
adding ant-run-plugin just for that that is just bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

pom.xml Outdated



<!-- JaCoCo test coverage. Will set ${surefire.jacoco.args} to be used in Arquillian config and as surefire jvm argument. -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no arquillian tests in WildFly core
And I don't know if jacoco will work with our WildFly test runner.

Question is should we even have this enabled for testsuite at all, maybe having it for unit tests would be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no arquillian tests in WildFly core, you are right, I update this comment.

Yes, JaCoCo is basically java agent, it works correctly also with WildFly Test Runner.

If we want to know test coverage of this testsuite, we need to enable JaCoCo in this testsuite. If we enable JaCoCo only in unit tests, we don't have test coverage data for testsuite. Testsuite adds more test coverage then unit tests.

Anyway, JaCoCo is disabled by default in this PR, it needs to be activated by -Dcoverage.

@bstansberry
Copy link
Contributor

@ctomc is this ok now?

pom.xml Outdated
@@ -149,6 +149,7 @@
<version.org.wildfly.security.elytron-web.undertow-server>1.0.0.Final</version.org.wildfly.security.elytron-web.undertow-server>
<version.xml-resolver>1.2</version.xml-resolver> <!-- Apache xml-resolver -->
<!-- Non-default maven plugin versions and configuration -->
<version.jacoco.plugin>0.7.9</version.jacoco.plugin>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.8.0 is out

pom.xml Outdated
</build>
</profile>
<profile> <!-- surefire.jacoco.args property needs to be set correctly, if coverage profile is not enabled -->
<id>ts.jacoco.profile.not.enabled</id>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can probably remove this profile completely.
just add <surefire.jacoco.args> to top of pom.xml, similarly as we do add <modular.jdk.args />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If surefire.jacoco.args is defined on the top of pom.xml, jacoco-maven-plugin is not able to replace its value. I check it with <surefire.jacoco.args /> and with <surefire.jacoco.args ></surefire.jacoco.args >

Append property of jacoco-maven-plugin ( https://github.com/wildfly/wildfly-core/pull/2736/files#diff-600376dffeb79835ede4a0b285078036R1795 )is not related with property content. Append=true means, that jacoco adds data to exec, but doesn't replace exec file, see http://www.eclemma.org/jacoco/trunk/doc/prepare-agent-mojo.html

Copy link
Contributor

@ctomc ctomc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the small changes that should be done, this looks fine.

@marekkopecky
Copy link
Contributor Author

retest this please

1 similar comment
@marekkopecky
Copy link
Contributor Author

retest this please

@bstansberry bstansberry merged commit d76a066 into wildfly:master Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants