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

Build-over-build New Feature Proposal #80

Merged
merged 7 commits into from
Mar 16, 2017

Conversation

aditi-rajawat
Copy link
Contributor

Build-over-build is a new feature proposed to eliminate Jenkin's builds with bad code coverage as compared to the last successful build. It allows user to configure delta threshold values for each type of coverage, which are compared to the delta coverage of the current build. Delta coverage of a build is defined as the absolute difference between coverage of last successful build and current build. If the coverage drops more than the user-configured thresholds then the build fails.
Build-over-build New Feature Proposal.pdf

@aditi-rajawat
Copy link
Contributor Author

Hi..my pull request failed checks due to merge conflicts. I suppose only people with write access to the repository can resolve the conflicts. Any estimate when it will be done? Thanks.

@centic9
Copy link
Member

centic9 commented Feb 17, 2017

The conflicts mean that you developed the changes on an older version of the project. You can and should resolve the conflicts yourself by merging the latest changes from origin/master into your feature-branch and then pushing the merge-commit to Github, the PR will be updated with those changes automatically then.

@aditi-rajawat
Copy link
Contributor Author

Ok. Thank you for clarifying. I will resolve the conflicts soon.

@aditi-rajawat
Copy link
Contributor Author

Please review the PR. Thank you.

@aditi-rajawat
Copy link
Contributor Author

@centic9 Hi. Can I get an estimated date for this PR merge and jacoco plugin next release? My team plans to use the next release for our project and we are seeking an estimation. Please let me know. Thank you.

Copy link
Member

@centic9 centic9 left a comment

Choose a reason for hiding this comment

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

I reviewed and verified the changes and it seems to work fine, a few questions/comments, though:

  • Why did you need to change everything to BigDecimal? floats should have enough precision here, or?
  • It is a bit uncommon to leave your name in all the code-places, the Git-history will show who did what anyway. It would be better to only describe why something was done to not clutter the code

@aditi-rajawat
Copy link
Contributor Author

Hi. I changed everything to BigDecimal for more accurate calculations of delta coverage. I agree with your second comment and have removed my name from the source code. Thanks.

@aditi-rajawat
Copy link
Contributor Author

Hi. Can this PR be merged with the master branch if all the changes have been verified satisfactorily? I would also like to know about the next version release date. Please let me know. Thanks.

@centic9
Copy link
Member

centic9 commented Feb 28, 2017

Sorry for taking my time here, but I cannot rush here both because I want to really understand the reason for all the changes and also because I am only working on it in my free time, which is quite limited nowadays.

Do you have an actual example where the precision of float was not enough? As we mostly operate in 0-100% with up to 2 decimal digits I don't immediately see the need here.

What about using double? All the BigIntegers will introduce quite some more complexity and some memory overhead, I only want to introduce it if it is really necessary.

Sorry for being picky, but this plugin is used by a lot of people and I don't want to introduce more complexity/overhead/memory than necessary without a good reason.

@mheinzerling
Copy link
Collaborator

mheinzerling commented Mar 1, 2017

I'm of course a friend of BigDecimals and promote them where ever possible. But I also don't get the reason why we might need them here?
Do we need the PowerMock? I never heard of it (but I also prefer custom mocks), is this the default in Jenkins plugins? We have already EasyMock in the pom file?

JacocoCoverageResultSummary currentBuildCoverage = JacocoLoadData.getResult(run);

JacocoDeltaCoverageResultSummary jacocoDeltaCoverageResultSummary = new JacocoDeltaCoverageResultSummary();
jacocoDeltaCoverageResultSummary.instructionCoverage = lastBuildCoverage.getInstructionCoverage().subtract(currentBuildCoverage.getInstructionCoverage()).abs();
Copy link
Member

Choose a reason for hiding this comment

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

This looks strange to me, why don't we keep the negative values so we know the actual difference?

@centic9 centic9 merged commit 0f1ddd9 into jenkinsci:master Mar 16, 2017
@centic9
Copy link
Member

centic9 commented Mar 16, 2017

With commits 2b0665e, 95983ae and de63db9 I have now applied/merged the changes, but reverted the BigDecimal changes for now. Please test the results as it was a non-primitive merge due to other concurrent changes in the meantime.

I also adjust the handling of abs() somewhat, I hope it still works fine.

Let me know as soon as you have verified the changes so we can prepare an official release.

@aditi-rajawat
Copy link
Contributor Author

@centic9 My apologies for not been able to respond. My other tasks took higher priority. Thank you for reverting BigDecimal changes. I understand it was not a clever choice considering the performance issues it might cause.

I am verifying the code from the master branch.

@centic9
Copy link
Member

centic9 commented Mar 23, 2017

no worries, I just released this as part of 2.2.0, please try that version directly via Jenkins update if possible so we get some more testing done early on and can release fixes quickly if necessary.

@aditi-rajawat
Copy link
Contributor Author

Okay. That's great. I will test the released version then. I just have one question. can we also increase the scale of coverage percentages displayed on Jenkins job page? If yes, I can submit the change. It is important in case of millions of lines of code.

@aditi-rajawat
Copy link
Contributor Author

I have tested the changes using 2.2.0 release version of the plugin from jenkins. It is working as expected. Also checked the source code. Thank you for including this feature and releasing it.

It would be good if we display coverage % with increased scale (approx 6 digits after decimal). Can I add that change?

@centic9
Copy link
Member

centic9 commented Mar 24, 2017

Yes, sure, please create a new PR, but please try to make it an option with the current precision as default as many people don't care about having it in this detail-level, e.g. for me 78 or 79% is usually enough information as I have multiple smaller projects.

@dbyron0
Copy link

dbyron0 commented Apr 25, 2017

Probably breaking the rules by posting here, but the mailing list looks dormant and I don't see a way to report an issue. I enabled the "Record JaCoCo coverage report" post-build task in my jenkins job and it worked great. I then clicked the "Fail the build if coverage degrades more than the delta thresholds" checkbox and ran the job again and got the following stack trace:

[JaCoCo plugin] Overall coverage: class: 97, method: 87, line: 93, branch: 62, instruction: 90
ERROR: Step ‘Record JaCoCo coverage report’ aborted due to exception: 
java.lang.ClassNotFoundException: org.joda.time.ReadablePartial
	at java.lang.ClassLoader.findClass(ClassLoader.java:530)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at hudson.util.MaskingClassLoader.loadClass(MaskingClassLoader.java:75)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at org.apache.tools.ant.AntClassLoader.findBaseClass(AntClassLoader.java:1387)
	at org.apache.tools.ant.AntClassLoader.loadClass(AntClassLoader.java:1080)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
Caused: java.lang.NoClassDefFoundError: org/joda/time/ReadablePartial
	at hudson.plugins.jacoco.portlet.bean.JacocoDeltaCoverageResultSummary.build(JacocoDeltaCoverageResultSummary.java:37)
	at hudson.plugins.jacoco.JacocoPublisher.checkBuildOverBuildResult(JacocoPublisher.java:703)
	at hudson.plugins.jacoco.JacocoPublisher.perform(JacocoPublisher.java:637)
	at hudson.tasks.BuildStepCompatibilityLayer.perform(BuildStepCompatibilityLayer.java:78)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:720)
	at hudson.model.Build$BuildExecution.post2(Build.java:186)
	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:665)
	at hudson.model.Run.execute(Run.java:1753)
	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:405)

I'm using version 2.46.1 of jenkins and version 2.2.0 of the jacoco plugin.

Thanks for your help.

@dbyron0
Copy link

dbyron0 commented Apr 26, 2017

And...happily now there's #87 and https://issues.jenkins-ci.org/browse/JENKINS-43103. Thank you.

@dcalap
Copy link

dcalap commented Oct 30, 2017

Hi, I have a question/problem with this feature. I have this config in my pipeline:
jacoco buildOverBuild: true, deltaMethodCoverage: '1'
And when I run a build log shows:

[JaCoCo plugin] Delta coverage: class: 0.0, method: -5.0, line: -1.989708, branch: 0.0, instruction: -1.413044, complexity: -4.545452
[JaCoCo plugin] Delta thresholds: JacocoHealthReportDeltaThresholds [deltaInstruction=0.0, deltaBranch=0.0, deltaComplexity=0.0, deltaLine=0.0, deltaMethod=1.0, deltaClass=0.0]
[JaCoCo plugin] Results of delta thresholds check: FAILURE

But build not failing and finish successfully. Any idea how to break the build if delta thresholds check FAILS?

Thanks!

@sivajangam
Copy link

screen shot 2017-12-07 at 5 09 15 pm

HI,

I downloaded the plugin and shows different label,please find in my screenshot.
Is this the one with feature Build over Build ? if it is then what are the metrics which I need to define to make the build fail when code coverage degrades compared to last successful build coverage

@centic9
Copy link
Member

centic9 commented Dec 10, 2017

Yes, the items under "fail the build if " are probably what you are looking for.

@sivajangam
Copy link

sivajangam commented Dec 11, 2017

But what should be the ideal metrics which I need to give, as 2.0 for all or any other metrics, as 0 might fail, Could you suggest

@sivajangam
Copy link

Can anyone explain me the scenario, as I kept delta threshold as 2.0,
as per formula D coverage = | coverage of last successful build – coverage of current build |

But for me the build was success in scenario-1 and failing in scenario-2.

Scenario-1

02:16:01 [JaCoCo plugin] Delta coverage: class: 1.900177, method: 2.404953, line: 1.9257431, branch: 0.84700394, instruction: 0.9057579, complexity: 1.597332
02:16:01 [JaCoCo plugin] Delta thresholds: JacocoHealthReportDeltaThresholds [deltaInstruction=2.0, deltaBranch=2.0, deltaComplexity=2.0, deltaLine=2.0, deltaMethod=2.0, deltaClass=2.0]
02:16:01 [JaCoCo plugin] Results of delta thresholds check: SUCCESS

Scenario -2

11:40:54 [JaCoCo plugin] Delta coverage: class: -1.900177, method: -2.404953, line: -1.9257431, branch: -0.84700394, instruction: -0.9057579, complexity: -1.597332
11:40:54 [JaCoCo plugin] Delta thresholds: JacocoHealthReportDeltaThresholds [deltaInstruction=2.0, deltaBranch=2.0, deltaComplexity=2.0, deltaLine=2.0, deltaMethod=2.0, deltaClass=2.0]
11:40:54 [JaCoCo plugin] Results of delta thresholds check: FAILURE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants