Fix issue with submodules looking at other submodule class files #31

rrourke · 2022-11-08T21:32:43Z

This fixes a problem with submodules where if it was previously built even using mvn clean it will look in other sub modules for their class files. Class files should be isolated to the module that the diff is being performed on.
This will also skip running if a jacoco file is not found.

SurpSG

Hi @rrourke

Thank you for your contribution.
I leaved a few comments in your PR. Just trying to understand what issue(s) you want to fix

SurpSG · 2022-11-09T08:04:36Z

src/main/kotlin/com/github/surpsg/DiffCoverageMojo.kt

        return reactorProjects.asSequence()
            .map { project -> File(project.build.outputDirectory) }
-            .filter { outputDirectory -> outputDirectory.exists() }
+            .filter { outputDirectory -> outputDirectory.exists() && outputDirectory.path.contains(rootProjectDir.path)}


Cannot understand why do we need this check. Could you explain?

Might not need this, was from before. I'll remove

Just tested it, this is still needed. Without this the class files are not filtered and all classes for all modules are exposed.

Sorry, still cannot get it. Could you provide a sample project or integration tests, so I can see a project setup?

Existing IT here: https://github.com/SurpSG/diff-coverage-maven-plugin/tree/master/src/it

It is going to take me a while to make to make a project that hits this criteria but think of it like this.
project demo
Sub modules:

A

B

C
Module build order is project demo -> a -> b ->c.
Code changes are made to sub module a, unit tests are added passes code coverage.
B now runs and it detects the classes from module a, its new classes from module b(self) and will pass jacoc but will fail code coverage because there are no tests for classes from a in its jacoco that only ran on b.

if you print https://github.com/SurpSG/diff-coverage-maven-plugin/blob/master/src/main/kotlin/com/github/surpsg/DiffCoverageMojo.kt#L91

you will see:

Classes [INFO] [/Users/.../demo/target/classes, /Users/.../demo/target/a/classes, /Users/.../demo/b/target/classes, /Users/.../demo/b/target/classes]

so when the code coverage runs it will look at the classes in a see that no tests were run in jacoco and then fail.
My change makes it so what is seen is this

Classes [INFO] [/Users/.../demo/b/target/classes]

@SurpSG https://github.com/rrourke/mvn-example/tree/quickTest here is an example repo. Checkout the quickTest repo and run mvn install clean you will see it fail for the reasons i mentioned above. You can then use my code for diff-covrage-maven-plugin and you will see it pass.

Now I got it, thank you!

Np, just let me know what else I need to do to get this merged

It would be nice to add several integration tests

https://github.com/SurpSG/diff-coverage-maven-plugin/tree/master/src/it

SurpSG · 2022-11-09T08:10:31Z

src/main/kotlin/com/github/surpsg/DiffCoverageMojo.kt

+        }
+        if (!found) {
+            log.info("No exec files found skipping")
+            return


Could you explain your motivation?

Exec files are always appear if tests are run. From my experience, missed exec files it's misconfiguration.

Not all modules might have unit tests, this might be redundant since I changed how it scans.

SurpSG · 2022-11-09T08:12:10Z

src/main/kotlin/com/github/surpsg/DiffCoverageMojo.kt


    private val rootProjectDir: File
-        get() = reactorProjects[0].basedir
+        get() = project.basedir


Why do we need this change?

When there are sub modules it scans for all class files or if using the -rf flag it will use the base dir of the first module ran. In either case the class files it scans for are either just from the first module or it looks at all modules. This causes problems as if you have sub module a, b, and c if you make changes to b than when c runs the code change was in b but the jacoco for c doesn't have the unit tests and thus fails. Making this change makes it so it only looks at the class files for the module that is running the tests.

rrourke · 2022-11-16T18:41:33Z

@SurpSG I've made some changes let me know what you think

rrourke · 2022-11-17T18:54:57Z

@SurpSG saw the test failed added missing path files that were not added

SurpSG · 2022-11-21T22:00:20Z

Hi @rrourke

Thank you for this awesome work!

Still investigating your PR.

rrourke · 2022-11-22T16:44:14Z

@SurpSG looks like it failed on itself, what do you want to do from here?

SurpSG · 2022-11-22T21:26:48Z

The plugin is applied to itself. It reported that coverage is not enough.
But please, don't increase the coverage ritght now. Wait until I finish my investigation.
Seems, Maven doesn't provide a clear way to solve this issue.

Suppose, we have a structure:

root
  |__ module_1
  |__module_2
  |__pom.xml

What I expect:

Jacoco maven plugin is aplied to submodules
Diff Coverage is applied to the root only
When we run install or verify:
** tests are run in module_1(coverage is collected by jacoco)
** tests are run in module_2(coverage is collected by jacoco)
** Diff Coverage is run:
*** collected .exec files from all projects
*** collected classes from all projects
*** Run diff coverage analyze. Finally, receive aggregated diff coverage report from entire project

Currently, it has a different behavior.

How it was implemented in jacoco maven plugin to receive the aggregated report? Recommendations from the jacoco team:

create a separate subproject(reporting-module) that depends on all another projects
apply Jacoco aggregate to the reporting-module
When we call install:
all tests are run(coverage collected)
at the end reporting-module is invoked with aggregation task

DiffCoverage maven plugin currently can work in the same way. See integration test: (multi-module-reports-generation-check)[https://github.com/SurpSG/diff-coverage-maven-plugin/tree/master/src/it/multi-module-reports-generation-check]
But I don't like such approach. We have a separate module for reporting needs.

What we can do:

declare the DiffCoverageMojo.kt as aggregator(see about aggregator here)
But the verification on root is invoked earlier than on subpojects, so we don't have coverage info at that moment. Somehow we should force invoking diff coverage that applied to the root after subproject's tests.
Another solutiong - convert DiffCoverageMojo to report plugin. I have a draft code for this approach and seems it works as I expect. I need more time to investigate my approach and your solution as well

rrourke added 3 commits November 8, 2022 12:41

Fix issue with submodules looking at other submodule class files

c16cb9a

use the maven project that is running base url

22572a5

remove semicolon

57fd06a

SurpSG reviewed Nov 9, 2022

View reviewed changes

rrourke added 2 commits November 15, 2022 15:01

fix tests

db7d5c9

revert maven changes

7e0eb8c

add missing path files

b07799c

fixes

0422816

Fix issue with submodules looking at other submodule class files #31

Are you sure you want to change the base?

Fix issue with submodules looking at other submodule class files #31

Uh oh!

Conversation

rrourke commented Nov 8, 2022

Uh oh!

SurpSG left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrourke commented Nov 16, 2022

Uh oh!

rrourke commented Nov 17, 2022

Uh oh!

SurpSG commented Nov 21, 2022

Uh oh!

rrourke commented Nov 22, 2022

Uh oh!

SurpSG commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SurpSG commented Nov 22, 2022 •

edited

Loading