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

Branch counter of reporter may have a bug #695

Open
KiritaniAyaka opened this issue Sep 23, 2022 · 16 comments · May be fixed by #699
Open

Branch counter of reporter may have a bug #695

KiritaniAyaka opened this issue Sep 23, 2022 · 16 comments · May be fixed by #699

Comments

@KiritaniAyaka
Copy link

KiritaniAyaka commented Sep 23, 2022

Recap

I'm using codecov for code coverage. Codecov marked some lines as Partial that means one of branches is not covered, but they are marked as covered in HTML reports.

I think codecov is using clover.xml as the coverage information source. Then I checked my clover.xml, the values of truecount and falsecount seems are wrong, please check my repro and screenshots below.

Repro

I created a repo for repro this. My lib is using vitest (vitest depends istanbul indirectly) so I created 2 branches for repro of istanbul and vitest.

HTML Reports

image

clover.xml

...
<line num="1" count="1" type="stmt"/>
<line num="3" count="2" type="cond" truecount="2" falsecount="0"/>
<line num="5" count="1" type="stmt"/>
<line num="9" count="1" type="stmt"/>
<line num="12" count="2" type="stmt"/>
...

Line 5, 9 are marked as covered in HTML reports, but the falsecount of branch at line 3 is 0 in xml reports.

Codecov

image

Codecov think one of branches are not covered.

Possible expected output of clover.xml

...
<line num="1" count="1" type="stmt"/>
- <line num="3" count="2" type="cond" truecount="2" falsecount="0"/>
+ <line num="3" count="2" type="cond" truecount="1" falsecount="1"/>
<line num="5" count="1" type="stmt"/>
<line num="9" count="1" type="stmt"/>
<line num="12" count="2" type="stmt"/>
...

I'm new to code coverage, so I'm not sure whether it is a bug of istanbul, or other lib, or codecov?

@KiritaniAyaka KiritaniAyaka changed the title Branch counts of reporter may have a mistake Branch counter of reporter may have a bug Sep 23, 2022
@mvorisek
Copy link

mvorisek commented Oct 7, 2022

Probably very related issue with clover renderer - never reached condition is rendered with falsecount = 2:

coverage data from instrumented JS code:

image

currently rendered clover line:

<line num="63" count="0" type="cond" truecount="0" falsecount="2"/>

expected rendered clover line:

<line num="63" count="0" type="cond" truecount="0" falsecount="0"/>

this problem is causing CodeCov to think the line is partially covered:

image

the fix should be easy, as the coverage data are correct, probably one condition/line needs to be fixed in the clover renderer code

place where rendered:

attrs.falsecount = branchDetail.total - branchDetail.covered;

@KiritaniAyaka
Copy link
Author

Probably very related issue with clover renderer - never reached condition is rendered with falsecount = 2:

Right.
In clover, truecount means there are truecount branches are covered in this condition, and falsecount means "not covered".
CodeCov think truecount means the times that condition is judged as true, and falsecount means times of false.

But which practice of clover and CodeCov is more common and standard?

@KiritaniAyaka
Copy link
Author

In clover, truecount means there are truecount branches are covered in this condition, and falsecount means "not covered".

Addition for this:

Test this function

export default function (a: number) {
	switch (a) {
		case 1:
			console.log(1)
			break
		case 2:
			console.log(2)
			break
		case 3:
			console.log(3)
			break
		default:
			console.log('default')
	}
	return a
}

Testing file:

it("index", () => {
	func(1);
});

Clover output:

...
<line num="1" count="1" type="stmt"/>
<line num="2" count="1" type="cond" truecount="1" falsecount="3"/>
<line num="4" count="1" type="stmt"/>
<line num="5" count="1" type="stmt"/>
...

Then we change testing file:

it("index", () => {
	func(1);
	func(1);
	func(2);
	func(99);
});

re-run test and check clover.xml again:

...
<line num="1" count="1" type="stmt"/>
<line num="2" count="4" type="cond" truecount="3" falsecount="1"/>
<line num="4" count="2" type="stmt"/>
...

In clover, truecount means there are truecount branches are covered in this condition, and falsecount means "not covered".

Changing testing file and source file to other situation, it could be confirmed too.

I already add this case at latest commit of my repro.

@mvorisek
Copy link

mvorisek commented Oct 11, 2022

truecount means there are truecount branches are covered in this condition, and falsecount means "not covered". CodeCov think truecount means the times that condition is judged as true, and falsecount means times of false.

@KiritaniAyaka do you have any reference for it?

I would say count should represent how many times the coverage expression (statement, branch, ...) was reached.

And if a binary expression, the truecount should represent how many times it evaluated to true, falsecount how many times it evaluated to false. truecount + falsecount should always equals to count for every <line... clover element.

here is some clover output example from our CI:

        <line num="171" count="56" type="cond" truecount="3" falsecount="1"/>
        <line num="177" count="56" type="cond" truecount="1" falsecount="1"/>
        <line num="178" count="56" type="stmt"/>
        <line num="181" count="0" type="stmt"/>
        <line num="185" count="112" type="cond" truecount="2" falsecount="0"/>

truecount + falsecount simply does not equal to count

@KiritaniAyaka
Copy link
Author

do you have any reference for it?

No reference, just my personal infer. You can check my repro and its CodeCov reports to confirm it.

CodeCov and istanbul-reporter has different understanding to truecount and falsecount. Need a generic standard that most reporter are following to judge which went wrong.

@mvorisek
Copy link

your example with case is great, for KiritaniAyaka/istanbul-coverage-report@6a301a5 commit the clover output is:

<?xml version="1.0" encoding="UTF-8"?>
<coverage generated="1665465748533" clover="3.2.0">
  <project timestamp="1665465748533" name="All files">
    <metrics statements="23" coveredstatements="18" conditionals="8" coveredconditionals="5" methods="4" coveredmethods="4" elements="35" coveredelements="27" complexity="0" loc="23" ncloc="23" packages="1" files="3" classes="3"/>
    <file name="index.ts" path="/home/runner/work/istanbul-coverage-report/istanbul-coverage-report/src/index.ts">
      <metrics statements="5" coveredstatements="5" conditionals="2" coveredconditionals="2" methods="1" coveredmethods="1"/>
      <line num="1" count="1" type="stmt"/>
      <line num="3" count="2" type="cond" truecount="2" falsecount="0"/>
      <line num="5" count="1" type="stmt"/>
      <line num="9" count="1" type="stmt"/>
      <line num="12" count="2" type="stmt"/>
    </file>
    <file name="index2.ts" path="/home/runner/work/istanbul-coverage-report/istanbul-coverage-report/src/index2.ts">
      <metrics statements="8" coveredstatements="8" conditionals="2" coveredconditionals="2" methods="2" coveredmethods="2"/>
      <line num="1" count="1" type="stmt"/>
      <line num="3" count="2" type="stmt"/>
      <line num="5" count="2" type="cond" truecount="2" falsecount="0"/>
      <line num="7" count="1" type="stmt"/>
      <line num="11" count="1" type="stmt"/>
      <line num="15" count="2" type="stmt"/>
      <line num="17" count="2" type="stmt"/>
      <line num="21" count="4" type="stmt"/>
    </file>
    <file name="index3.ts" path="/home/runner/work/istanbul-coverage-report/istanbul-coverage-report/src/index3.ts">
      <metrics statements="10" coveredstatements="5" conditionals="4" coveredconditionals="1" methods="1" coveredmethods="1"/>
      <line num="1" count="1" type="stmt"/>
      <line num="2" count="1" type="cond" truecount="1" falsecount="3"/>
      <line num="4" count="1" type="stmt"/>
      <line num="5" count="1" type="stmt"/>
      <line num="8" count="0" type="stmt"/>
      <line num="9" count="0" type="stmt"/>
      <line num="12" count="0" type="stmt"/>
      <line num="13" count="0" type="stmt"/>
      <line num="16" count="0" type="stmt"/>
      <line num="18" count="1" type="stmt"/>
    </file>
  </project>
</coverage>

It seems you are right about the current output format. The output is following:

  • count - how many times the expression was reached, no matter the binary result
  • truecount - number of reached branches
  • falsecount - number of non-reached branches

the CodeCov should then process the coverage like:

  • if count == 0, then line is uncovered
  • else when truecount is present, line is fully covered if falsecount == 0, otherwise line is only partially covered

@thomasrockhu-codecov can you please comment on this problem?

@thomasrockhu-codecov
Copy link

thomasrockhu-codecov commented Oct 15, 2022

@mvorisek I need to dig in further here, but at a glance, Codecov does not consider partial lines as covered.

We defined it as hits / (sum of hit + partial + miss)
Source

To comment on

if count == 0, then line is uncovered
else when truecount is present, line is fully covered if falsecount == 0, otherwise line is only partially covered
The logic we take is basically

if count == 0: uncovered
if truecount == 0 and falsecount == 0: uncovered
elif truecount == 0 or falsecount == 0: partial
else: covered

@mvorisek
Copy link

thank you for the reply, the problem is the falsecount definition

as shown on a real istanbuljs/nyc clover output data in #695 (comment)

istanbuljs/nyc defines falsecount as number of non-reached branches (ie. falsecount == 0 means full coverage)

CodeCov, per your response, per issue description and also my experience, processes it like number of times the expression has evaluated to false (ie. falsecount == 0 means partial coverage)

as there is no official clover standard, I cannot tell which definition is correct, but both make sense

so the question is, what CodeCov "usually" receives, are there any other clover reporters that uses the definition implemented by CodeCov?

@KiritaniAyaka
Copy link
Author

KiritaniAyaka commented Oct 15, 2022

Thanks for replying. The logic of CodeCov looks excellent and has no problem.

In semantics, I think we should use truecount and falsecount to indicate times of condition was judged as true and false. but istanbul didn't do so.

We can be sure it is a bug of istanbul if there is documentation supporting this.

Do you have documentation about Clover.xml? I can't find any guidelines on search engines about how Clover.xml should be rendered.


BTW, we can't learn from other JavaScript testing libraries because I found almost all of them are using istanbul-reports to create coverage reports. In other words, they are making the same mistakes as istanbul-reports.


are there any other clover reporters that uses the definition implemented by CodeCov?

Well, the implementation of other reporters is also worthy of ref ✨

@drazisil-codecov
Copy link

drazisil-codecov commented Oct 17, 2022

Interesting. I'm aware of istanbuljs/v8-to-istanbul#158, but this is the first I've seen mention of a clover issue. Let me hunt around...

edit: On the subject of a schema, I believe that https://gist.github.com/aik099/8556655 is the closest that exists to a documented "standard"

@KiritaniAyaka
Copy link
Author

edit: On the subject of a schema, I believe that https://gist.github.com/aik099/8556655 is the closest that exists to a documented "standard"

Thanks for the information, it's very pivotal!

<xs:documentation>
...
    @count - only applicable if @type == 'stmt' or 'method'; the number of times the construct was executed
    @truecount - only applicable if @type == 'cond'; the number of times the true branch was executed
    @falsecount - only applicable if @type == 'cond'; the number of times the false branch was executed
...
</xs:documentation>

Source

According to this, we can almost know this is a bug of istanbul.

@KiritaniAyaka
Copy link
Author

Excuse me, I want to discuss further on this.

I'm working on this issue but uncertain how to deal with the switch.

The switch is currently considered a "condition" (cond). We know the result of the switch is not binary, so how to decide the truecount and falsecount of it?

I think there are two practices:

  • considering switch as a "statement" (stmt)
  • keeping the truecount and falsecount to a truthy (always 1)

Do you have any advice or better practice?

@silverbackdan
Copy link

silverbackdan commented Dec 8, 2022

Hi, I have another reproduction where it is very simple and shows that the truecount and falsecount do not match the total of count.
vitest-dev/vitest#2456 (comment)

I have a branch and am adding lots of testing, there are a number of cases where I get incorrect partial matches, this thought best to start with this issue and give another reproduction for you with 1 very simple file and test.

Once the patch is merged I will see how it affects my tests and raise more reproductions of other partial matches should they still exist.

I get lots of partial matches on class function calls for example. Thanks :)

@lorenzogrv
Copy link

Dropping a comment to get notified on updates

@bcoe
Copy link
Member

bcoe commented Jan 30, 2023

I created a repo for repro this. My lib is using vitest (vitest depends istanbul indirectly) so I created 2 branches for repro of istanbul and vitest.

In your example, are you expecting truecount = 1 and falsecount = 1, because both the if and else branches are executed once?

Presumably, if the if branch alone was executed twice, we'd expect truecount = 2, falsecount = 1 (it sounds like this isn't the case in your setup though.

@JavanShen
Copy link

have same probleam

lars-reimann added a commit to Safe-DS/DSL that referenced this issue Jul 4, 2023
### Summary of Changes

If the `clover.xml` report is sent to Codecov lines with conditionals
are incorrectly marked as only partially covered (see [this
issue](istanbuljs/istanbuljs#695)). We are now
send the JSON report instead to get a more accurate picture of the code
coverage.
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 a pull request may close this issue.

8 participants