coverage-v8 issues with calculation of branch coverage

stereobooster commented 1 month ago

Describe the bug

I have following code

export function comp(a, b) {
  if (a === b) {
    return 0
  } else if (a > b) {
    return 1
  } else {
    return -1
  }
}

and following test

test("compares", () => {
  expect(comp(1, 1)).toBe(0);
  expect(comp(1, 0)).toBe(1);
});

it shows branch coverage 2/3 - which is correct. But If I add missing test

expect(comp(0, 1)).toBe(-1);

It shows branch coverage 4/4. Which seems to be incorrect

If I change code to

export function comp(a, b) {
  if (a === b) {
    return 0
  } else if (a > b) {
    return 1
  } 
  return -1
}

It shows branch coverage 5/5. Which seems to be incorrect.

PS 1 I checked it with istanbul, and istanbul numbers are more consistent. So I guess this is the issue with c8

PS 2 I checked using c8 (^10.1.2) directly and results are different, so there is a bug

Reproduction

https://github.com/stereobooster/vitest-coverage-v8

System Info

System:
    OS: macOS 14.1.1
    CPU: (8) x64 Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
    Memory: 196.43 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.17.1 - ~/.nvm/versions/node/v18.17.1/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.6.7 - ~/.nvm/versions/node/v18.17.1/bin/npm
    pnpm: 9.2.0 - ~/Library/pnpm/pnpm
  Browsers:
    Chrome: 127.0.6533.99
    Safari: 17.1
  npmPackages:
    @vitest/coverage-v8: ^2.0.3 => 2.0.3
    @vitest/ui: ^2.0.3 => 2.0.3
    vite: ^5.3.4 => 5.3.4
    vitest: ^2.0.3 => 2.0.

Used Package Manager

pnpm

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

stereobooster commented 1 month ago

AriPerkkio commented 1 month ago

This is same as https://github.com/bcoe/c8/issues/172, also mentioned in the description of https://github.com/vitest-dev/vitest/pull/4309.

Here's reproduction with just Node without Vitest, c8 or any other npm package:

import { writeFileSync } from "node:fs";
import inspector from "node:inspector";

writeFileSync(
  "./source-file.mjs",
  `
export function comp(a, b) {
  if (a === b) {
    return 0;
  } else if (a > b) {
    return 1;
  } else {
    return -1;
  }
}
`.trim(),
  "utf8"
);

const session = new inspector.Session();

session.connect();
session.post("Profiler.enable");
session.post("Profiler.startPreciseCoverage", {
  callCount: true,
  detailed: true,
});

const { comp } = await import("./source-file.mjs");
comp(1, 1);
comp(1, 0);
// comp(0, 1);

const coverage = await collectCoverage("source-file.mjs");
console.log(JSON.stringify(coverage, null, 2));

async function collectCoverage(filename) {
  return new Promise((resolve, reject) => {
    session.post("Profiler.takePreciseCoverage", async (error, coverage) => {
      if (error) return reject(error);

      const result = coverage.result.filter((entry) =>
        entry.url.includes(filename)
      );

      resolve({ result });
    });
  });
}

stereobooster commented 1 month ago

I got a hint in different thread. It seems https://github.com/cenfun/monocart-coverage-reports can handle those cases correctly

AriPerkkio commented 1 month ago

Yup, it does this by AST analysis. Similar how @vitest/coverage-istanbul works, but they do that also for V8 coverage.

There's still a bug/feature in V8/Node that causes this branch count issue.

stereobooster commented 1 month ago

I created extended comparison https://github.com/stereobooster/test-coverage-calculation if anybody is curios

vitest-dev / vitest