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

Odd behavior reporting (unchanged) coverage levels #492

Open
ferdnyc opened this issue Oct 16, 2024 · 3 comments
Open

Odd behavior reporting (unchanged) coverage levels #492

ferdnyc opened this issue Oct 16, 2024 · 3 comments

Comments

@ferdnyc
Copy link
Contributor

ferdnyc commented Oct 16, 2024

Just noticed this on a recent PR run:


Coverage report

FileStatementsMissingCoverageCoverage
(new stmts)
  src/pydot
  classes.py
  core.py
  dot_parser.py
  test
  conftest.py
  test_classes.py
  test_pydot.py
Project Total

This report was generated by python-coverage-comment-action


...There were no code changes, so there were no coverage changes, of course. But why does test_pydot.py staying at 93% coverage get a green badge, while classes.py staying at 96% gets an orange badge? Something fishy there.

Possible it has something to do with the branch coverage, though I don't think so.

(Although, speaking of branch coverage, having it included does make those summaries a bit "off". For example, 384 ÷ 402 is actually 95.5%, not 93%. 93% is the correct coverage value, because missing branches lower the total coverage for the file... but it means that displaying both the percentages and the lines-of-coverage fractions is a bit confusing/misleading, since one doesn't directly correspond to the other.)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 16, 2024

Aha. I guess the tooltips explain it:

classes.py: "This PR removes 0.18 percentage points from the coverage rate in src/pydot/classes.py, taking it from 97.05% (50/52) to 96.87% (50/52)."

test_pydot.py: "This PR adds 0.06 percentage points to the coverage rate in test/test_pydot.py, taking it from 93.8% (384/402) to 93.86% (384/402)."

(So, all the more reason not to show the lines-covered/total-lines fractions, since it's confusing if 50/52 is expressed as both 97.05% and 96.87%.)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Oct 16, 2024

What might be better is to display something like this:

...That way, the explanation isn't just in the tooltip.

@ewjoachim
Copy link
Member

I seemed to recall that we weren't supposed to display lines that weren't modified, I wonder why this didn't work (at least for lines with absolutely no changes) (maybe it's a branch coverage issue too)

Yeah, I think the issue is that branch coverage makes it less clear what exactly the displayed number is. That said, I think there's more value in seeing if 93% is 93/100 or 9300/10000 than seeing more digits.

Finally: I don't understand how the PR change even impacts branch 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

No branches or pull requests

2 participants