Coverage minima: add more fine-grained control #55

kitbellew · 2018-02-18T19:58:33Z

Along with existing statement minimum, add branch minimum. Also, include
this pair of control at the package and file level.

kitbellew · 2018-03-10T02:29:05Z

@gslowikowski friendly reminder to review.

kitbellew · 2019-04-10T22:38:34Z

@gslowikowski are you interested in pursuing this still?

gslowikowski · 2019-04-15T18:24:36Z

Yes, I am. I didn't have enough time, when you created your pulls.

I've created some simple test projects to compare SCoverage with Cobertura and Jacoco for similar syntax constructs and it looks very different. I wasn't sure if SCoverage properly reports overall coverage.

Now I will try to find time and return to this problem. I will publish my test projects, and we will see if something in current code base should be changed before merging your pulls, or not.

kitbellew · 2021-05-04T22:26:36Z

@ckipp01 rebased

jozic · 2023-12-23T17:25:51Z

Hey @kitbellew
would you be interested in rebasing this?

kitbellew · 2023-12-23T17:45:09Z

Hey @kitbellew would you be interested in rebasing this?

and them what will happen?

jozic · 2023-12-23T20:34:29Z

I can review and if all is good merge and release it.
I know this PR is super old, but I've only recently got the member/release permissions from @ckipp01.
@gslowikowski seems inactive, at least in this repo, and I'm willing to give back to the community by keeping this plugin maintained. At least for now. So I'm offering some help with moving this forward.
If you don't need/want this anymore, I can totally understand it.

kitbellew · 2023-12-23T21:14:38Z

@jozic ah, no problem, will do. (did it once before, and nothing happened subsequently, hence my question.)

kitbellew · 2023-12-25T21:20:16Z

@jozic сделано.

jozic · 2023-12-26T02:32:30Z

thanks @kitbellew
checking it now
would you mind adding an integration test to it folder following the pattern there?

kitbellew · 2023-12-26T21:20:59Z

would you mind adding an integration test to it folder following the pattern there?

done.

kitbellew · 2023-12-26T21:21:54Z

@jozic it might be easier to review one commit at a time, they are structured logically.

jozic

looks very nice, left some questions/suggestions

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

jozic · 2023-12-26T22:04:59Z

src/main/java/org/scoverage/plugin/SCoverageReportMojo.java

@@ -547,6 +551,32 @@ else if ( !module.getPackaging().equals( "pom" ) )
        getLog().info( "Coverage aggregated reports completed." );
    }

+    private MavenProject findAncestorModuleWithShortestBasedir()


seems like this is not really related to minimums we can set, right?
seems like it's similar to #104
could you please clarify why we need this here? Can we extract that to a separate PR?

ah, correct. the code is what we used in our production for many years, and it's likely that there were two unrelated changes to make everything work. will remove from this pr (and the linked one might be a better implementation).

Just a heads-up, i've merged #104, so dropping this particular change should be safe now.

jozic · 2023-12-26T22:26:08Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+        {
+            if ( !checkCoverage( getLog(), "Package:" + pkgCoverage.name(), pkgCoverage,
+                                 minimumCoverageStmtPerPackage, minimumCoverageBranchPerPackage, false ) )
+                ok = false;


seems like this will continue even after after first false
should we instead use something like
CollectionConverters.asJava( coverage.packages() ).stream().allMatch?
or do you want it to run for each package no matter what?
same question for files

good question. we can either report until first failure (in which case the user would fix something and then find out there's more), or report everything. options:

report everything

report only first failure

put the choice under yet another parameter.

I personally generate a report on a failure, to see what's not covered. So the first failure seems fine to me.
But I can also see a point of having all the packages/files checked at once.

How about we do fail-fast in this PR and then we can do a separate PR to add an extra flag, like failFast with true by default?

@jozic good morning and happy new year.

i finally got around to working on this, and here's what i realized: we will have reporting on all packages and files if it's successful but will stop at the first error if it happens (with a single [ERROR] message after possibly a bunch of [INFO]/[DEBUG]). which is a bit weird since the opposite is probably of more interest.

so, i see several alternatives:

report everything, just like now

report everything, except use [DEBUG] for second and subsequent errors

collect all info/debug messages until the first error; if there are no errors, only then log the info/debug.

thoughts?

Thanks @kitbellew, happy New Year to you as well!

Good point, i guess reporting packages and files as debug for success and error for failure is a fair game. So we should probably leave it as you have it )

@jozic all done and pushed.

jozic · 2023-12-26T22:27:49Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+    private static boolean checkCoverage( Log logger, String metric, CoverageMetrics metrics,
+                                          double minimumStmt, double minimimBranch, boolean logSuccessInfo )


typo minimumBranch

jozic · 2023-12-26T22:33:06Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+        if ( is100( minimum ) && is100( actual ) )
+        {
+            logSuccess( logger, String.format( "Coverage is 100%: %s!", metric ), logSuccessInfo );


Suggested change

logSuccess( logger, String.format( "Coverage is 100%: %s!", metric ), logSuccessInfo );

logSuccess( logger, String.format( "Coverage is 100%%: %s!", metric ), logSuccessInfo );

otherwise it fails with java.util.UnknownFormatConversionException: Conversion = ':'

would be nice to add module3 to your awesome tests checking this branch )

jozic · 2023-12-26T22:42:50Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+
+        boolean ok = checkCoverage( getLog(), "Total", coverage,
+                                    minimumCoverage, minimumCoverageBranchTotal, true );
+        for ( MeasuredPackage pkgCoverage : CollectionConverters.asJava( coverage.packages() ) )


this could be a large list, right? for files it will be even larger
how about wrapping this into if(minimumCoverageStmtPerFile > 0.0 && minimumCoverageBranchPerPackage > 0.0) to avoid checking every package/file with default/0 minimum?

jozic · 2023-12-26T22:56:42Z

src/it/test_coverage_minima/validate.groovy

@@ -0,0 +1,38 @@
+def checkModule(logText, module, coverageLog) {


Awesome tests! Thank you.

you added the tests, i just duplicated :)

kitbellew · 2023-12-31T19:22:57Z

perhaps windows tests will now succeed. added .replaceAll("\r\n", "\n").

jozic

Almost there, let's make the test maven 3.9 compatible and we should be good to merge and then hopefully release soon after

jozic · 2023-12-31T21:24:52Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+
+        boolean ok = checkCoverage( getLog(), "Total", coverage,
+                                    minimumCoverage, minimumCoverageBranchTotal, true );
+        ok = checkCoverage( getLog(), "Package:", coverage.packages(), x -> x.name(),


minor, we can do MeasuredPackage::name instead of x -> x.name() and MeasuredFile::filename instead of x -> x.filename()

will do. not really familiar with new java syntax, more of a scala guy myself :)

same here, I just don't like x variables and Intellij was kind/smart enough to suggest those :)

jozic · 2023-12-31T22:54:07Z

src/main/java/org/scoverage/plugin/SCoverageCheckMojo.java

+            {
+                ok = false;
+            }
+        }


minor, seems like this can be just ok = predicate.test(elem) && ok;

will do, thank you, aligns well with what i did above.

jozic · 2024-01-01T00:52:07Z

src/it/test_coverage_minima/validate.groovy

+    assert new File(basedir, module + "/target/scoverage.xml").exists()
+    assert new File(basedir, module + "/target/site/scoverage/index.html").exists()
+    def entry = logText.find {
+        it.startsWith("scoverage-maven-plugin:") &&


i've noticed that with maven 3.9.6 ( i have that one locally) this check fails, as seems like maven 3.9 changed format from [INFO] --- scoverage-maven-plugin:2.0.1-SNAPSHOT:check (default-cli) @ module01 ---
to [INFO] --- scoverage:2.0.1-SNAPSHOT:check (default-cli) @ module01 ---
so changing to it.startsWith("scoverage") && seems fine for both 3.9.x and 3.8.x

Previously, it was only reported as such for min = 100.

Also, extract a coverage computation and logging method.

Along with existing overall coverage, add parameters for statement and branch minima at the package and file level.

kitbellew · 2024-01-01T01:23:59Z

@jozic all done, ptal. happy new year :)

jozic

lgtm

@kitbellew thanks for your contribution and for your patience during reviews and those almost 6 years :)

I see that @gslowikowski wanted to do some extra checks, but as he's no longer active we gonna move forward and merge/release this. We can always revisit things later if needed.

Happy New Year!

jozic · 2024-01-06T18:59:32Z

@kitbellew this is released in 2.0.1

kitbellew force-pushed the covmin branch 2 times, most recently from 44ab980 to b61c01c Compare February 19, 2018 03:19

kitbellew mentioned this pull request Feb 22, 2018

Coverage minima: add more fine-grained control scoverage/sbt-scoverage#253

Merged

kitbellew force-pushed the covmin branch from b61c01c to c282961 Compare May 4, 2021 22:25

kitbellew force-pushed the covmin branch from c282961 to ce6ee05 Compare December 25, 2023 21:18

kitbellew force-pushed the covmin branch from ce6ee05 to 73e91d1 Compare December 26, 2023 21:20

kitbellew force-pushed the covmin branch 2 times, most recently from dfd41a6 to 85c5796 Compare December 26, 2023 21:59

jozic reviewed Dec 26, 2023

View reviewed changes

This was referenced Dec 27, 2023

Skip compilation when skip scoverage to speed up build #178

Closed

how to switch between statement or branch coverage as default #109

Closed

kitbellew force-pushed the covmin branch 2 times, most recently from 2fbd487 to 3d9c712 Compare December 31, 2023 19:22

jozic reviewed Jan 1, 2024

View reviewed changes

kitbellew added 3 commits December 31, 2023 17:19

Coverage minima: Add integration tests to check it

f20affd

Coverage minima: report 100% coverage if min < 100

86f51a3

Previously, it was only reported as such for min = 100.

Coverage minima: add parameter for branch minima

6f2b217

Also, extract a coverage computation and logging method.

Coverage minima: add more fine-grained control

6e10469

Along with existing overall coverage, add parameters for statement and branch minima at the package and file level.

kitbellew force-pushed the covmin branch from 3d9c712 to 6e10469 Compare January 1, 2024 01:23

jozic approved these changes Jan 1, 2024

View reviewed changes

jozic merged commit 1f37dd0 into scoverage:master Jan 1, 2024
4 checks passed

kitbellew deleted the covmin branch January 1, 2024 02:13

		private static boolean checkCoverage( Log logger, String metric, CoverageMetrics metrics,
		double minimumStmt, double minimimBranch, boolean logSuccessInfo )

	logSuccess( logger, String.format( "Coverage is 100%: %s!", metric ), logSuccessInfo );
	logSuccess( logger, String.format( "Coverage is 100%%: %s!", metric ), logSuccessInfo );

		@@ -0,0 +1,38 @@
		def checkModule(logText, module, coverageLog) {

Coverage minima: add more fine-grained control #55

Coverage minima: add more fine-grained control #55

Conversation

kitbellew commented Feb 18, 2018

kitbellew commented Mar 10, 2018

kitbellew commented Apr 10, 2019

gslowikowski commented Apr 15, 2019

kitbellew commented May 4, 2021

jozic commented Dec 23, 2023

kitbellew commented Dec 23, 2023

jozic commented Dec 23, 2023

kitbellew commented Dec 23, 2023

kitbellew commented Dec 25, 2023

jozic commented Dec 26, 2023

kitbellew commented Dec 26, 2023

kitbellew commented Dec 26, 2023

jozic 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

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

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

kitbellew commented Dec 31, 2023

jozic 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitbellew commented Jan 1, 2024

jozic left a comment • edited Loading

Choose a reason for hiding this comment

jozic commented Jan 6, 2024

jozic left a comment •

edited

Loading