Skip to content

Conversation

@elek
Copy link
Member

@elek elek commented Jun 3, 2020

What changes were proposed in this pull request?

HDDS-3635 started to archive the jacoco coverage data for each of the unit and integration tests (unit, it-*).

This patch introduces a new build step to combine all of them together and archive the coverage report in HTML as a build artifact.

Notes:

  1. acceptance test coverage is not yet included

  2. I decided to do it only for master (branch) builds as it requires a new build which adds ~15 minutes to the full build. As the coverage data is not (yet) used for PR we don't need to enable it (yet)

  3. We can further improve it to upload the merged data to somewhere (sonar?) Can be done in the next Jira

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3710

How was this patch tested?

I run it on my local fork. The final version expected to be here:

https://github.com/elek/hadoop-ozone/actions?query=branch%3AHDDS-3710

Download the coverage artifact and check the index.html.

@elek
Copy link
Member Author

elek commented Jun 3, 2020

FYI @arp7 @vivekratnavel

@vivekratnavel: this patch creates an XML report which can be used to upload the results to any services. @arp7 suggested updating the Sonar, we can move the sonar execution to the coverage step which can use the report

@elek
Copy link
Member Author

elek commented Jun 3, 2020

Updated the sonar report. Will see how does it work after the next build on apache repo:

https://github.com/apache/hadoop-ozone/actions?query=branch%3AHDDS-3710

@nandakumar131 nandakumar131 added the build Pull request that modifies the build process label Jun 4, 2020
Copy link
Contributor

@vivekratnavel vivekratnavel left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@elek
Copy link
Member Author

elek commented Jun 4, 2020

Finally, it's ready to merge.

You can see the results here: https://sonarcloud.io/dashboard?branch=HDDS-3710&id=hadoop-ozone (see the coverage numbers)

The most tricky part: We create one big coverage report file (all it-* + unit test for all subproject), but sonar plugin requires one for each subproject. Creating one merged for each subproject (one file for each subproject which includes unit + it-*) seems to be complex, but fortunately sonar works well if the one big merged is copied to all the sub-projects.

@elek
Copy link
Member Author

elek commented Jun 5, 2020

Thanks @vivekratnavel the review. I am merging it now. Sonar master will be updated with coverage data after the next full green build.

@elek elek merged commit a12565f into apache:master Jun 5, 2020
@adoroszlai
Copy link
Contributor

@elek post-commit checks on master started failing with the error below since this PR was merged:

2020-06-05T09:11:19.4479655Z [INFO] Loading execution data file /home/runner/work/hadoop-ozone/hadoop-ozone/.github/../target/coverage/jacoco-all.exec.
2020-06-05T09:11:22.5622500Z Exception in thread "main" java.io.IOException: Error while analyzing target/coverage-classes/org/apache/logging/log4j/core/util/SystemClock.class.
2020-06-05T09:11:22.5626525Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzerError(Analyzer.java:162)
2020-06-05T09:11:22.5627923Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:134)
2020-06-05T09:11:22.5629079Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:157)
2020-06-05T09:11:22.5630207Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:193)
2020-06-05T09:11:22.5631470Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:226)
2020-06-05T09:11:22.5632560Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5633642Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5635181Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5636463Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5637598Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5639213Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5640400Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:221)
2020-06-05T09:11:22.5641966Z 	at org.jacoco.cli.internal.commands.Report.analyze(Report.java:110)
2020-06-05T09:11:22.5643631Z 	at org.jacoco.cli.internal.commands.Report.execute(Report.java:84)
2020-06-05T09:11:22.5645243Z 	at org.jacoco.cli.internal.Main.execute(Main.java:90)
2020-06-05T09:11:22.5647492Z 	at org.jacoco.cli.internal.Main.main(Main.java:105)
2020-06-05T09:11:22.5650142Z Caused by: java.lang.IllegalStateException: Can't add different class with same name: org/apache/logging/log4j/core/util/SystemClock
2020-06-05T09:11:22.5652279Z 	at org.jacoco.cli.internal.core.analysis.CoverageBuilder.visitCoverage(CoverageBuilder.java:106)
2020-06-05T09:11:22.5653435Z 	at org.jacoco.cli.internal.core.analysis.Analyzer$1.visitEnd(Analyzer.java:99)
2020-06-05T09:11:22.5654588Z 	at org.jacoco.cli.internal.asm.ClassVisitor.visitEnd(ClassVisitor.java:326)
2020-06-05T09:11:22.5655781Z 	at org.jacoco.cli.internal.core.internal.flow.ClassProbesAdapter.visitEnd(ClassProbesAdapter.java:100)
2020-06-05T09:11:22.5656912Z 	at org.jacoco.cli.internal.asm.ClassReader.accept(ClassReader.java:692)
2020-06-05T09:11:22.5658019Z 	at org.jacoco.cli.internal.asm.ClassReader.accept(ClassReader.java:400)
2020-06-05T09:11:22.5659133Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:116)
2020-06-05T09:11:22.5661518Z 	at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeClass(Analyzer.java:132)
2020-06-05T09:11:22.5662765Z 	... 14 more
2020-06-05T09:11:22.5854728Z ##[error]Process completed with exit code 1.

https://github.com/apache/hadoop-ozone/runs/741621135

@elek
Copy link
Member Author

elek commented Jun 5, 2020

@adoroszlai Debugging right now. I think it's a conflict with HDDS-3627 / #975
Coverage report requires to unzip all the related classes. legacy/current jar files were excluded, but they are replaced with hadoop2/hadoop3

@elek
Copy link
Member Author

elek commented Jun 5, 2020

Tried to fix with addendum, but didn't work. I will revert it and merge it back when stability is proved on a separated branch.

elek added a commit that referenced this pull request Jun 5, 2020
@adoroszlai
Copy link
Contributor

Thanks @elek for taking care of the failure.

elek added a commit to elek/ozone that referenced this pull request Jun 6, 2020
@elek
Copy link
Member Author

elek commented Jun 7, 2020

Addendum was almost good, will commit the fixed version:

This is almost the same as the original one (just with fixed exclude rule to be compatible with HDDS-3627) therefore I don't think that we need an other review round. And I would prefer to push it on the weekend (It should work, but if it doesn't it can be fixed until Monday)

On the fork branch it worked well:

https://github.com/elek/hadoop-ozone/actions/runs/127489033

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

Labels

build Pull request that modifies the build process

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants