[FLINK-5092] Add maven profile with code coverage report generation #2836

BorisOsipov · 2016-11-21T05:48:07Z

Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.

General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
Tests & Build
- Functionality added by the pull request is covered by tests
- mvn clean verify has been executed successfully locally or a Travis build has passed

I hope it's nice to have ability to collect test coverage.

mxm

Thanks for your contribution @BorisOsipov! Looks good. I would prefer is we changed the argLine definitions as little as possible. That's why I would like to just use the late property evaluation once in the parent POM and let that be inherited by all Surefire configurations.

mxm · 2016-11-22T14:47:46Z

pom.xml

 						<log4j.configuration>${log4j.configuration}</log4j.configuration>
 					</systemPropertyVariables>
-					<argLine>-Xms256m -Xmx800m -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>
+					<!-- Do not define argLine here. See http://www.eclemma.org/jacoco/trunk/doc/prepare-agent-mojo.html -->


That's bound to cause problems because a lot of code assumes these arguments here. We shouldn't touch them and instead do as the documentation you linked suggested:

One of the ways to do this in case of maven-surefire-plugin - is to use syntax for late property evaluation:

<argLine>@{argLine} -Xms256m -Xmx800m -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>

mxm · 2016-11-22T14:49:03Z

pom.xml

@@ -93,6 +93,7 @@ under the License.
 		<flink.forkCount>1C</flink.forkCount>
 		<flink.reuseForks>true</flink.reuseForks>
 		<log4j.configuration>log4j-test.properties</log4j.configuration>
+		<argLine>-Xms256m -Xmx800m -XX:-UseGCOverheadLimit</argLine>


I would prefer if the argline configuration stayed as is. It is missing the fork number here. This requires us to manually insert it for all modules which rely on it.

mxm · 2016-11-22T14:50:39Z

flink-streaming-connectors/flink-connector-cassandra/pom.xml

-					<argLine>-Xms256m -Xmx2800m -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>
+					<systemPropertyVariables>
+						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
+					</systemPropertyVariables>


We can just remove all argLine definition here along with the forkNumber definition. They are already defined in the parent pom.

mxm · 2016-11-22T14:50:48Z

flink-streaming-connectors/flink-connector-kafka-0.10/pom.xml

-					<argLine>-Xms256m -Xmx1000m -Dlog4j.configuration=${log4j.configuration} -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>
+					<systemPropertyVariables>
+						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
+					</systemPropertyVariables>


mxm · 2016-11-22T14:50:56Z

flink-streaming-connectors/flink-connector-kafka-0.9/pom.xml

-					<argLine>-Xms256m -Xmx1000m -Dlog4j.configuration=${log4j.configuration} -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>
+					<systemPropertyVariables>
+						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
+					</systemPropertyVariables>


And here (see above).

mxm

A few minor comments. Then we should be ready to merge.

mxm · 2016-11-23T09:50:30Z

pom.xml

@@ -93,7 +93,7 @@ under the License.
 		<flink.forkCount>1C</flink.forkCount>
 		<flink.reuseForks>true</flink.reuseForks>
 		<log4j.configuration>log4j-test.properties</log4j.configuration>
-		<argLine>-Xms256m -Xmx800m -XX:-UseGCOverheadLimit</argLine>
+		<argLine> </argLine>


This is not needed anymore.

mxm · 2016-11-23T09:53:31Z

pom.xml

@@ -996,10 +996,9 @@ under the License.
 					<reuseForks>${flink.reuseForks}</reuseForks>
 					<systemPropertyVariables>
 						<forkNumber>0${surefire.forkNumber}</forkNumber>
-						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>


Actually, this is probably fine. Could you re-add this line and remove the parameter from the the argLine?

mxm · 2016-11-23T10:02:52Z

flink-streaming-connectors/flink-connector-kafka-0.9/pom.xml

-					<systemPropertyVariables>
-						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
-					</systemPropertyVariables>
+					<argLine>-Xms256m -Xmx1000m -Dlog4j.configuration=${log4j.configuration} -XX:-UseGCOverheadLimit</argLine>


I think this can be reduced to

<argLine>@{argLine} -Xmx1000m</argLine>

mxm · 2016-11-23T10:03:29Z

flink-streaming-connectors/flink-connector-kafka-0.10/pom.xml

-					<systemPropertyVariables>
-						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
-					</systemPropertyVariables>
+					<argLine>-Xms256m -Xmx1000m -Dlog4j.configuration=${log4j.configuration} -XX:-UseGCOverheadLimit</argLine>


I think this can be reduced to

<argLine>@{argLine} -Xmx1000m</argLine>

mxm · 2016-11-23T10:03:43Z

flink-streaming-connectors/flink-connector-cassandra/pom.xml

-					<systemPropertyVariables>
-						<mvn.forkNumber>0${surefire.forkNumber}</mvn.forkNumber>
-					</systemPropertyVariables>
+					<argLine>-Xms256m -Xmx2800m -Dmvn.forkNumber=${surefire.forkNumber} -XX:-UseGCOverheadLimit</argLine>


I think this can be reduced to

<argLine>@{argLine} -Xmx2800m</argLine>

BorisOsipov · 2016-11-23T10:08:43Z

@mxm Thank you for review. I ran into some issues with collecting coverage data.
I am going to do additional tests and come back to you.

BorisOsipov · 2016-11-25T08:22:11Z

@mxm
I tried to use late property evaluation approach in argLine.
Unfortunately, it doesn't work well with different profiles. In some cases it breaks test run by maven without a coverage profile. In other case it breaks run tests from IDE.
Finally, I came back to the first version with argLine as property. I tested it with\without coverage profile. I didn't find any problems or changes in tests run.
There is no problem with inheritance of Surefire configuration.
I didn't change JVM memory options in argLine to keep the current behavior as is.
I introduced mvn.forkNumber as a system property instead of the parameter in argLine.(this property is used to correct logging)

zentol · 2016-11-25T08:33:04Z

Has anyone actually looked in detail into the report that is generated?

BorisOsipov · 2016-11-25T09:54:56Z

@zentol I aggregated results by SonarQube.
I can share some overall report.
flink.pdf

zentol · 2016-11-25T10:36:08Z

If i ran this myself, could i see which lines are not covered?

BorisOsipov · 2016-11-25T10:45:00Z

You can find report in ${project.basedir}/target/jacoco-report folder.

mxm

If late property binding does not work, let's use the property. Could you add a comment for the property? It seems to be an undocumented feature of Surefire. It is hard to guess that setting a property influences the configuration of a Maven plugin?

By the way, the forkNumber is used for logging but also other purposes (using it as an offset for port bindings to prevent collisions).

zentol · 2016-11-25T12:38:45Z

The plugin cannot detect cross-module coverage, correct? as in, all the tests in flink-tests will not contribute in any way to the coverage of other modules?

mxm · 2016-11-25T12:58:00Z

It does not: https://github.com/jacoco/jacoco/wiki/MavenMultiModule That's a bummer but apparently there are some workarounds: http://www.thinkcode.se/blog/2012/02/18/test-coverage-in-a-multi-module-maven-project

BorisOsipov · 2016-12-14T11:54:00Z

@zentol Yes plugin cannot detect out of box cross-module coverage, but there are some tricks how it could be done.

I've implemented and tested one of them.
I squash older commits and split my works to two commits.
The first one adds coverage plugin and change surefire configuration.
The second one adds:

new maven submodule for coverage report aggregation.
change shade maven plugin configuration. When tests run with coverage profile shaded jar doesn't replace original jar(shadedArtifactAttached=true). It needs because maven-shade-plugin changes class files and it makes impossible coverage reports aggregations, because jacoco requires the same classes on data collecting and report phases.

Changes in shade configuration made some test broken:

ClassLoaderITCase
ZooKeeperLeaderElectionITCase
JobManagerHACheckpointRecoveryITCase

I investigate that they cannot run correctly without shading and there is no way to fix it.
I think it is not a big problem.

So you can get full coverage report by running command mvn clean install -Pcoverage -Dmaven.test.failure.ignore=true and open flink/flink-test-utils-parent/flink-test-utils-coverage/target/site/jacoco-aggregate/index.html

What do you think about these changes?
I glad to answer you questions if you have

zentol · 2016-12-16T12:46:05Z

I will try this out on the weekend and report back :)

BorisOsipov · 2016-12-16T13:05:31Z

Pay attention that Jacoco does not support parallel test execution. Your experiment will be longer than usual full tests execution

BorisOsipov · 2017-01-17T18:13:55Z

@zentol Hi. Any news?

zentol · 2017-01-17T18:32:02Z

@BorisOsipov i haven't found time yet to try this out :(

The long running time makes it tricky since you devote the machine completely to tests for hours. I could let it run over night though ... frankly, i haven't considered that option yet for some reason.

Will try that tonight.

zentol · 2017-01-18T08:46:24Z

I let it run overnight but a test failure caused the build to lockup :(
Will disable the test and try again this evening.

kenmy · 2017-01-18T12:28:12Z

I have tryed to collect coverage by this approach.
During my attempt I rebased this PR into e187b5e (master in apach/flink at the yesterday evening). And share result via gdrive.
My branch is here. I am not sure that did anything right because haven't checked added projects since this PR. I hope is will help to resolve conflicts faster.

zentol · 2017-01-18T13:18:04Z

@kenmy Thank you, this helped me a lot :)

@BorisOsipov From a quick glance the aggregation appears to be working.

This is really cool stuff. We'll have to go through the pom changes again, maybe @rmetzger can help here. But i wanna get this in. :)

Some thoughts:

Is there a way to exclude certain files? We have some generated code (Tuple classes), and it's a bit funky for scala code (specifically case classes).
Is there a way to see how often a given line was tested? (Could maybe be useful for test deduplication)

BorisOsipov · 2017-01-18T13:34:23Z

@zentol

Is there a way to exclude certain files?

Sure. It setups like in other plugins by include\exclude section in plugin configuration.

Is there a way to see how often a given line was tested?

Unfortunately no. All available coverage counters described here http://www.eclemma.org/jacoco/trunk/doc/counters.html

zentol · 2017-01-18T13:44:28Z

Ok, cool. We should certainly exclude classes under "org.apache.flink.runtime.messages".

I'll have to dig through the report some more to find others, but i guess we can add them later on too.

zentol · 2017-01-25T14:27:48Z

flink-contrib/flink-storm-examples/pom.xml

@@ -364,6 +363,18 @@ under the License.
 							</transformers>
 						</configuration>
 					</execution>
+					<execution>
+						<id>shade-flink</id>


With this we execute the shade-plugin also for other build-profiles, correct? (same for flink-examples/flink-examples-streaming) It would be cool if this would only be done when creating the coverage report, so that the normal build-time is unaffected.

The maven's philosophy is 'one artifact per project'. These projects don't follow this way and produce several jar's artifacts :)
Shade plugin should produce flink-storm-streaming_2.10-1.2-SNAPSHOT-shaded.jar but it doesn't do it. It is a problem for maven install plugin that wants to have such jar by default. And it leads to failure on install phase without such configuration changes. For this reason I added such changes.

So I propose don't collect test coverage for these projects (flink-storm and streaming examples) at all. What do you think?

- add maven profile 'coverage' with maven jacoco plugin - extract argLine from surefire configuration to project properties

- add module for tests coverage report aggregating - add option for disabling replacing original jar with shaded one

…e report

BorisOsipov · 2017-02-04T07:54:00Z

@zentol I've rebased pr and excluded mentioned above flink-examples-streaming and flink-storm-examples modules.

rmetzger · 2017-02-13T20:21:21Z

flink-connectors/flink-connector-cassandra/pom.xml

@@ -65,6 +68,7 @@ under the License.
 							<goal>shade</goal>
 						</goals>
 						<configuration combine.self="override">
+							<shadedArtifactAttached>${shade.shadedArtifactAttached}</shadedArtifactAttached>


Why can't we define this in the parent pom globally for all shade plugin instances?

rmetzger · 2017-02-13T20:22:09Z

flink-examples/pom.xml

+		<profile>
+			<id>coverage</id>
+			<modules>
+				<module>flink-examples-batch</module>


Why are the streaming examples not in the module list?

rmetzger · 2017-02-13T20:22:33Z

flink-test-utils-parent/flink-test-utils-coverage/pom.xml

+	There is a separate "flink-test-utils-coverage" package that expects the all flink packages
+	have built before.
+	We need to depend on all modules if we want to get full coverage report
+    including coverage from tests in flink-tests module.


tabs vs spaces for indentation

mxm reviewed Nov 22, 2016

View reviewed changes

mxm reviewed Nov 23, 2016

View reviewed changes

mxm reviewed Nov 25, 2016

View reviewed changes

BorisOsipov force-pushed the JacocoCoverage branch from b1ce36a to 5fc5d95 Compare December 14, 2016 11:23

zentol reviewed Jan 25, 2017

View reviewed changes

Boris Osipov added 3 commits February 3, 2017 11:52

[FLINK-5092] Add maven profile with code coverage report generation

23f343f

- add maven profile 'coverage' with maven jacoco plugin - extract argLine from surefire configuration to project properties

[FLINK-5092] Add maven profile with code coverage report generation

f00387c

- add module for tests coverage report aggregating - add option for disabling replacing original jar with shaded one

[FLINK-5092] Rebase work to master. Exclude some modules from coverag…

c1eb73e

…e report

BorisOsipov force-pushed the JacocoCoverage branch from 5fc5d95 to c1eb73e Compare February 4, 2017 07:49

BorisOsipov closed this Feb 4, 2017

[FLINK-5092] Update plugin version

64575fc

BorisOsipov reopened this Feb 4, 2017

rmetzger reviewed Feb 13, 2017

View reviewed changes

BorisOsipov closed this Apr 13, 2017

rmetzger added the component=Tests label Mar 14, 2019

[FLINK-5092] Add maven profile with code coverage report generation #2836

[FLINK-5092] Add maven profile with code coverage report generation #2836

Conversation

BorisOsipov commented Nov 21, 2016 • edited

mxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisOsipov commented Nov 23, 2016

BorisOsipov commented Nov 25, 2016

zentol commented Nov 25, 2016

BorisOsipov commented Nov 25, 2016

zentol commented Nov 25, 2016

BorisOsipov commented Nov 25, 2016 • edited

mxm left a comment

Choose a reason for hiding this comment

zentol commented Nov 25, 2016

mxm commented Nov 25, 2016 via email • edited

BorisOsipov commented Dec 14, 2016

zentol commented Dec 16, 2016

BorisOsipov commented Dec 16, 2016

BorisOsipov commented Jan 17, 2017

zentol commented Jan 17, 2017

zentol commented Jan 18, 2017

kenmy commented Jan 18, 2017

zentol commented Jan 18, 2017

BorisOsipov commented Jan 18, 2017

zentol commented Jan 18, 2017 • edited

Choose a reason for hiding this comment

BorisOsipov Jan 25, 2017 • edited

Choose a reason for hiding this comment

BorisOsipov commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BorisOsipov commented Nov 21, 2016 •

edited

BorisOsipov commented Nov 25, 2016 •

edited

mxm commented Nov 25, 2016 via email •

edited

zentol commented Jan 18, 2017 •

edited

BorisOsipov Jan 25, 2017 •

edited