Legend in html is wrong if threshold options are 0% or 100% #369

Spacetown · 2020-05-05T18:44:38Z

If --html-high-threshold is 100% in the html the legend is:
high: >= 100%
If --html-medium-threshold is 0% in the html the legend is:
low: <= 0%

The =shall only be printed if threshold isn't 0% or 100%

The text was updated successfully, but these errors were encountered:

Spacetown · 2020-05-05T22:15:54Z

Not needed anymore since #367 is merged one hour ago. :-)

Spacetown · 2020-05-06T08:04:48Z

Checked the test data and saw that the legend is changed only.

latk · 2020-05-06T08:43:41Z

The current template includes this code:

<span class="coverage-low">low: &lt; {{COVERAGE_MED}} %</span>
{% if COVERAGE_MED != COVERAGE_HIGH %}
<span class="coverage-medium">medium: &gt;= {{COVERAGE_MED}} %</span>
{% endif %}
<span class="coverage-high">high: &gt;= {{COVERAGE_HIGH}} %</span>

What specifically do you propose as an alternative?

If I understand correctly the low-coverage should check zero:

{% if COVERAGE_MED == 0 %}
<span class="coverage-low">low: = 0 %</span>
{% else %}
<span class="coverage-low">low: &lt; {{COVERAGE_MED}} %</span>
{% endif %}

And the high coverage should check against 100:

{% if COVERAGE_HIGH < 100 %}
<span class="coverage-high">high: &gt;= {{COVERAGE_HIGH}} %</span>
{% else %}
<span class="coverage-high">high: = 100 %</span>
{% endif %}

Right? Or did I misunderstand?

Spacetown · 2020-05-06T10:26:58Z

That's correct. I'm working on a PR for this.

Spacetown · 2020-05-06T10:44:01Z

After adding tests I think it should be:

{% if COVERAGE_MED > 0 %}
<span class="coverage-low">low: &lt; {{COVERAGE_MED}} %</span>
{% endif %}

Because the medium value is part of the mid range.

latk · 2020-05-06T11:56:37Z

That sounds reasonable, but the legend must match the actual code in the coverage_to_class() function:

gcovr/gcovr/html_generator.py

Lines 77 to 86 in 1f17d44

    
           def coverage_to_class(coverage, medium_threshold, high_threshold): 
        
               if coverage is None: 
        
                   return 'coverage-unknown' 
        
               if coverage == 0: 
        
                   return 'coverage-none' 
        
               if coverage < medium_threshold: 
        
                   return 'coverage-low' 
        
               if coverage < high_threshold: 
        
                   return 'coverage-medium' 
        
               return 'coverage-high'

(coverage-none and coverage-low are effectively the same, could be unified/removed though)

Add feature suggested in issue gcovr#369

Spacetown · 2020-05-06T12:01:42Z

So, the condition if coverage == 0: should be changed to if coverage == 0 and medium_threshold > 0:
Is this correct?

Spacetown · 2020-05-06T12:04:48Z

And alos the if coverage < medium_threshold.

latk · 2020-05-06T15:36:02Z

I'm not sure what would be correct. These are edge cases. We can define them in any way, but should choose that way that looks like the best user experience.

If medium coverage is zero, then arguably medium coverage just means low coverage.

Perhaps we should just borrow the semantics from the HTML5 <meter> element, which involves attributes min, max, low, high. Overview:

region	`<meter>`	current gcovr
low	`0% <= x <= low`	`x == 0% or x < medium`
medium	`low < x < high`	`medium <= x < high`
high	`high <= x <= 100%`	`high <= x <= 100%`

Perhaps we should then deprecate --html-medium-threshold in favour of a --html-low-threshold? Or leave the logic as it is?

I think the logic as it is is reasonable and that only the legend should be updated to reflect it correctly. Thus, the legend would always show an entry for low coverage. But I'm willing to be convinced otherwise.

Spacetown · 2020-05-06T19:37:52Z

I'm also not sure.
I've checked the collors of the meter elment and got following behaviour with Firefox 76.0:

region	`<meter>`
low (red)	`0% <= x < low`
medium (orange)	`low <= x <= high`
high (green)	`high < x <= 100%`

But it is a little bit wired if you only want to have low and high:

<!DOCTYPE html>
<html>
  <body>
    Optimum 99 (current value):
    <meter min="0" max="100" low="90" high="90" optimum="99"  value="89" title="%">89</meter>
    <meter min="0" max="100" low="90" high="90" optimum="99"  value="90" title="%">90</meter>
    <meter min="0" max="100" low="90" high="90" optimum="99"  value="91" title="%">91</meter>
    <hr/>
    Optimum egual to low/high:
    <meter min="0" max="100" low="90" high="90" optimum="90"  value="89" title="%">89</meter>
    <meter min="0" max="100" low="90" high="90" optimum="90"  value="90" title="%">90</meter>
    <meter min="0" max="100" low="90" high="90" optimum="90"  value="91" title="%">91</meter>
    <hr/>
  </body>
</html>

If the optimum is 99 (hardcoded, why not 100?), a value identical to high/low result in a mid color of the gauge.
If the optimum is set to the value of low/high, a same value is shown in the high color and the high and low range are shown in the mid color.

What about adding a new option --html-low-threshold with a default of None. If the option is set, the function coverage_to_class changes to the behaviour of the meter element.
If the low and high are the same, the value of the optimum attribute must depend on the value and the ranges to get the correct colors.
If the option isn't set, we can keep the old behaviour and legend. Maybe we can also print a deprecated warning and a hint to use the new option.

latk · 2020-05-07T17:19:28Z

Upon reading the HTML standards more closely, it seems entirely unspecified what low and high actually mean.

If the optimum is 99 (hardcoded, why not 100?)

Well spotted, we should probably correct that. We were probably worri 8000 ed that optimum=max would break something.

What about adding a new option --html-low-threshold with a default of None. If the option is set, the function coverage_to_class changes to the behaviour of the meter element.

This sounds complicated, and I don't want unnecessary complexity. Either we imitate <meter> entirely, or we stick to the current implementation, but not both.

Of the two variants, I find the current gcovr behaviour more intuitive. As a user story:

As a tester,
I want to highlight files that are well-tested
So that I can have a sense of progress and focus my efforts elsewhere.
- Context: a file is well-tested if it has at least xx% coverage.
- Solution: set --html-high-coverage=xx
As a tester
I want to highlight files that have low coverage
So that I can focus my efforts there.
- Context: a file has low coverage if it has less than yy% coverage.
- Solution: set --html-medium-coverage=yy

If a user has other expectations things get ugly, but this is unavoidable. For example:

As a tester
I want to highlight files that have low coverage
So that I can focus my efforts there.
- Context: a file has low coverage if it has at most 75% coverage.
- Solution: set --html-medium-coverage=75.0000001

This boils down to which relations (less than, at most, at least, more than, …) we expect. But for purposes like quality-gating I expect an inclusive lower bound for medium and high coverage regions, which means an exclusive upper bound for low and medium regions. What HTML <meter> does is, for our purposes, insane.

latk · 2020-05-07T17:23:10Z

I think that the current legend causes confusion because the medium-threshold is shown twice, in the definition of low and medium. Instead, I now think we should only show the lower bounds:

[low: ≥ 0%] [medium: ≥ 75%] [high: ≥ 90%]

(giving a lower bound for low is superfluous because it's always zero, but not giving the bound would look odd.)

I also think that in this definition, it would be fine if the legend for medium is always shown, even when medium=high or medium=0. This would mean that we'd actually delete code, and don't need new test cases :)

I'm ambivalent regarding whether cases like high=100 should be special-cased.

Spacetown · 2020-05-07T18:26:35Z

I think that the current legend causes confusion because the medium-threshold is shown twice, in the definition of low and medium. Instead, I now think we should only show the lower bounds:
[low: ≥ 0%] [medium: ≥ 75%] [high: ≥ 90%]
(giving a lower bound for low is superfluous because it's always zero, but not giving the bound would look odd.)

I fully agree.

I also think that in this definition, it would be fine if the legend for medium is always shown, even when medium=high or medium=0. This would mean that we'd actually delete code, and don't need new test cases :)

Current implementation will not print medium if it is equal to high. This is tested with html-high-75. I've already a test case added on my local branch for medium=0

I'm ambivalent regarding whether cases like high=100 should be special-cased.

I've played a little bit with the ranges and saw high => 100 %. We can also only encapsulate the >.

Spacetown · 2020-05-07T18:55:05Z

Reply to the meter element and the behaviour:

I've played a little bit more with the meter and different browsers and have come to following result for low 80 and high 90:

Value	Firefox 76.0 (MacOs)	Safari (MacOs)	Crome 81.0 (Android)
70	red	red	red
80	yellow	yellow	yellow
85	yellow	yellow	yellow
90	yellow	green	green
100	green	green	green

Result for low 80 and high 80:

Value	Firefox 76.0 (MacOs)	Safari (MacOs)	Crome 81.0 (Android)
70	red	red	red
80	yellow	green	green
85	green	green	green
90	green	green	green
100	green	green	green

You see, the colors depend on the browser and are not reliable.
I suggest to change the stylesheet for the meter and use the class attribute like for table and span elements. With this we'll be independant from the browser implementation of the ranges and the optimum attribut.

I'll try this localy and let you know about the reult.

Spacetown · 2020-05-07T22:14:31Z

I've updated the template and the test data, in my opinion result looks good.
The legend in the form [low: >= 0%] [medium: >= 75.0%] [high: >= 90.0%] as suggested by you is much better to read than the old one low: < 75.0 % medium: >= 75.0 % high: >= 90.0 %.
The result is rendered identical in Firfox and Safari.

- Change legend in html. - --html-medium-threshold must be greater 0.

latk · 2020-05-08T10:07:21Z

looks great, let's continue this as a pull request!

Spacetown · 2020-05-08T11:35:09Z

PR #371

- Change legend in html. - --html-medium-threshold must be greater 0.

latk · 2020-05-27T16:42:47Z

Closed via #371.

Spacetown changed the title ~~Legend in html wrong if threshold options are 0% or 100%~~ Legend in html is wrong if threshold options are 0% or 100% May 5, 2020

Spacetown closed this as completed May 5, 2020

Spacetown reopened this May 6, 2020

Spacetown pushed a commit to Spacetown/gcovr that referenced this issue May 6, 2020

Add feature suggested in issue gcovr#369

a1f3a74

Add feature suggested in issue gcovr#369

Spacetown pushed a commit to Spacetown/gcovr that referenced this issue May 7, 2020

Add feature suggested in issue gcovr#369

fcec568

- Change legend in html. - --html-medium-threshold must be greater 0.

latk pushed a commit that referenced this issue May 27, 2020

Add feature suggested in issue #369

2458c9c

- Change legend in html. - --html-medium-threshold must be greater 0.

latk closed this as completed May 27, 2020

latk added Format: HTML Type: Bug Type: Enhancement labels May 27, 2020

Spacetown mentioned this issue Feb 15, 2021

No longer colored columns, as seen on Chrome #475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Legend in html is wrong if threshold options are 0% or 100% #369

Legend in html is wrong if threshold options are 0% or 100% #369

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Legend in html is wrong if threshold options are 0% or 100% #369

Legend in html is wrong if threshold options are 0% or 100% #369

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!