Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Do not use color for unchanged coverage #777 #789

Merged
merged 5 commits into from
Oct 16, 2023

Conversation

Amanjain4269
Copy link
Contributor

@Amanjain4269 Amanjain4269 commented Oct 11, 2023

Changes the coloring of the delta in the overview: small changes < 0.01 percent will be not color. All other changes will be colored as before (positive: green, negative: red).

Bildschirmfoto 2023-10-18 um 14 55 32

The isPostiveTrend() is changed to getTrend() and corresponding file changes are made as per the PR - Do not use color for unchanged coverage #777
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks! Please also check the CI warnings.

void shouldReturnPositiveTrendForLineMetric() {
CoverageBuildAction action = createCoverageBuildActionWithDelta(Metric.LINE);
int trend = action.getTrend(Baseline.PROJECT, Metric.LINE);
assertThat(trend).isEqualTo(1);
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the delta values visible here? Otherwise it is hard to understand the test. It would also make sense to have in one test the boundaries used that return 0, >0, and <0

Some changes are made again as per the feedback in the draft PR Do not use color for unchanged coverage #777
@Amanjain4269
Copy link
Contributor Author

Thanks for the reviews.

I made the changes in another commit. Can you please check now?

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Almost ready for merge. Thanks again!

Comment on lines 126 to 128
CoverageBuildAction action = createCoverageBuildActionWithDelta();
double trend = action.getTrend(Baseline.PROJECT, Metric.LINE);
assertThat(trend).isEqualTo(0.3); // deltaValue = 0.3
Copy link
Member

Choose a reason for hiding this comment

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

Almost: what I actually meant is something like:

Suggested change
CoverageBuildAction action = createCoverageBuildActionWithDelta();
double trend = action.getTrend(Baseline.PROJECT, Metric.LINE);
assertThat(trend).isEqualTo(0.3); // deltaValue = 0.3
CoverageBuildAction action = createCoverageBuildActionWithDelta(Baseline.PROJECT, Metric.LINE, Fraction.of("0.001");
assertThat(action.getTrend(Baseline.PROJECT, Metric.LINE)).isPositive();

and then

        CoverageBuildAction action = createCoverageBuildActionWithDelta(Baseline.PROJECT, Metric.LINE, Fraction.of("0.0009");
        assertThat(action.getTrend(Baseline.PROJECT, Metric.LINE)).isZero();

So move the interesting parts to the focus of the reader.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be helpful to add here:

assertThat(action.formatDelta(Baseline.PROJECT, Metric.LINE)).isEqualTo("+0.10%");

@@ -544,18 +544,25 @@ public String formatDelta(final Baseline baseline, final Metric metric) {
* @param metric
* the metric to check
*
* @return {@code true} if the trend is positive, {@code false} otherwise
* @return {@code roundedDeltaValue} if the trend is positive or negative, {@code 0} otherwise
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return {@code roundedDeltaValue} if the trend is positive or negative, {@code 0} otherwise
* @return a positive value if the trend is positive, a negative value if the trend is negative, or {@code 0} if there is no significant change in the trend

double deltaValue = delta.get().doubleValue();
// to apply rounding off for boundary delta values
double roundedDelta = Math.round(deltaValue * 100.0) / 100.0;
if (-0.1 < roundedDelta && roundedDelta < 0.1) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my fault: percentages are stored in the interval [0,1], so it makes sense to compare 0.001. I also do not see the reason why you are rounding?

@uhafner uhafner added the enhancement Enhancement of existing functionality label Oct 12, 2023
This is regarding the PR Do not use color for unchanged coverage #777
@Amanjain4269
Copy link
Contributor Author

I made a new commit. Please check now.

Also, there is one more thing I want to mention. I think there are errors in the changes I made in the coverage-summary.jelly file, as the checks are failing. I tried another approach in this new commit but still the same.
Can you help me out with this?

@@ -60,11 +60,14 @@
<li>${formatter.formatValueWithMetric(value)}
<j:if test="${it.hasDelta(baseline, value.metric)}">
<j:choose>
<j:when test="${it.isPositiveTrend(baseline, value.metric)}">
<j:if test="${it.getTrend(baseline, value.metric) > 0}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<j:if test="${it.getTrend(baseline, value.metric) > 0}">
<j:when test="${it.getTrend(baseline, value.metric) > 0}">

</j:when>
<j:otherwise>
</j:if>
<j:if test="${it.getTrend(baseline, value.metric) < 0}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<j:if test="${it.getTrend(baseline, value.metric) < 0}">
<j:when test="${it.getTrend(baseline, value.metric) < 0}">

void shouldReturnZeroWhenDeltaIsNotPresentForGivenMetric() {
CoverageBuildAction action = createCoverageBuildActionWithDelta(Baseline.PROJECT, Metric.LINE, Optional.empty());
assertThat(action.getTrend(Baseline.PROJECT, Metric.LINE)).isZero();
}
Copy link
Member

Choose a reason for hiding this comment

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

Tests look good now!

Just a couple of analysis warnings remaining...

Final chages are made as per the reviews and warnings.
@Amanjain4269
Copy link
Contributor Author

@uhafner

Thanks for your valuable insights.

I made the final changes.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

There is a compile error in the Jelly file.

@@ -60,11 +60,14 @@
<li>${formatter.formatValueWithMetric(value)}
<j:if test="${it.hasDelta(baseline, value.metric)}">
<j:choose>
<j:when test="${it.isPositiveTrend(baseline, value.metric)}">
<j:when test="${it.getTrend(baseline, value.metric) > 0}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<j:when test="${it.getTrend(baseline, value.metric) > 0}">
<j:when test="${it.getTrend(baseline, value.metric) gt 0}">

<j:set var="color" value="var(--green)"/>
</j:when>
<j:otherwise>
<j:when test="${it.getTrend(baseline, value.metric) < 0}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<j:when test="${it.getTrend(baseline, value.metric) < 0}">
<j:when test="${it.getTrend(baseline, value.metric) lt 0}">

@@ -30,6 +31,25 @@
*/
@DefaultLocale("en")
class CoverageBuildActionTest {
@SuppressWarnings("unused")
private CoverageBuildAction createCoverageBuildActionWithDelta(final Baseline baseline, final Metric metric, final Optional<Fraction> delta) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems that baseline is not used at all:

Suggested change
private CoverageBuildAction createCoverageBuildActionWithDelta(final Baseline baseline, final Metric metric, final Optional<Fraction> delta) {
private CoverageBuildAction createCoverageBuildActionWithDelta(final Metric metric, final Optional<Fraction> delta) {

@@ -544,18 +544,23 @@ public String formatDelta(final Baseline baseline, final Metric metric) {
* @param metric
* the metric to check
*
* @return {@code true} if the trend is positive, {@code false} otherwise
* @return a positive value if the trend is positive, a negative value if the trend is negative, or {@code 0} if there is no significant change in the trend
Copy link
Member

Choose a reason for hiding this comment

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

There is a not required import in line 21

@uhafner uhafner marked this pull request as ready for review October 16, 2023 14:44
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #789 (c0a2b1c) into master (8106422) will increase coverage by 0.11%.
Report is 7 commits behind head on master.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #789      +/-   ##
============================================
+ Coverage     74.14%   74.26%   +0.11%     
- Complexity     1705     1710       +5     
============================================
  Files           130      130              
  Lines          6301     6302       +1     
  Branches        677      675       -2     
============================================
+ Hits           4672     4680       +8     
+ Misses         1407     1399       -8     
- Partials        222      223       +1     
Files Coverage Δ
...ns/coverage/metrics/steps/CoverageBuildAction.java 74.28% <100.00%> (+5.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Amanjain4269
Copy link
Contributor Author

@uhafner
Is everything OK now?

@uhafner
Copy link
Member

uhafner commented Oct 16, 2023

Thanks for your patience!

@uhafner uhafner merged commit 1c4eb14 into jenkinsci:master Oct 16, 2023
25 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants