Add support for maximum or average values in metrics#669
Add support for maximum or average values in metrics#669akash-manna-sky wants to merge 9 commits intojenkinsci:mainfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
plugin/src/main/java/io/jenkins/plugins/coverage/metrics/model/MetricAggregation.java
Fixed
Show fixed
Hide fixed
uhafner
left a comment
uhafner
left a comment
There was a problem hiding this comment.
Thanks for restoring this functionality. The Jenkins code looks good, but I think the code that computes the actual values (while it looks good) should be moved to the coverage-model, so that it can be used by other backends as well.
I will create an issue there, I need to spend some time on how to design the new API there. If you like, you then can continue there. (But I would fully understand if you would better like me to design the API in the coverage model, then you just need to adapt this PR).
| if (MetricAggregation.isSupported(metric)) { | ||
| this.aggregation = aggregation; | ||
| } |
There was a problem hiding this comment.
I'm not sure if this statement is save: when the metric is set after the aggregation, it will fail. Can't we set the aggregation in any case and ignore it later on?
| var aggregation = qualityGate.getAggregation(); | ||
|
|
||
| Optional<Value> possibleValue; | ||
| if (MetricAggregation.isSupported(metric) && aggregation != MetricAggregation.TOTAL && rootNode != null) { |
There was a problem hiding this comment.
How can the node get null?
| if (MetricAggregation.isSupported(metric) && aggregation != MetricAggregation.TOTAL && rootNode != null) { | |
| if (MetricAggregation.isSupported(metric) && aggregation != MetricAggregation.TOTAL) { |
| <li><strong>MAXIMUM</strong>: Maximum value found in any method or class</li> | ||
| <li><strong>AVERAGE</strong>: Average value across all methods or classes</li> |
There was a problem hiding this comment.
I need to check an example, but shouldn't these options work on the method level only?
| result.add(qualityGate, QualityGateStatus.INACTIVE, "n/a"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The code after this line actually should be part of the coverage model.
…lify aggregation handling
|
Then what should I do now? Should I open a new PR in Coverage Model or should I wait untill you open the issue there? Or anything else? Please suggest me. @uhafner |
|
I think I need more time to investigate on how to integrate this functionality. I started to document it in jenkinsci/coverage-model#270. |
Okay, please take your time. Once everything is sorted out, and if you think it’s appropriate to assign the issue to me, I would be happy to contribute. |
2 participants
Add support for maximum or average values in metrics
Fixes #639
Testing done
Submitter checklist