Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Feb 7, 2018

There are still many parts of Phobos which have a low coverage. Touching them shouldn't make a PR fail.

See also: dlang/phobos#6124

Example from #2079

codecov/patch — 21.052% of diff hit (target 74.653%)

Phobos and DMD already use this behavior.

@wilzbach wilzbach requested a review from andralex as a code owner February 7, 2018 15:28
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@wilzbach wilzbach added the Trivial typos, formatting, comments label Feb 7, 2018
@dlang-bot dlang-bot merged commit 9b97359 into dlang:master Feb 7, 2018
@MartinNowak
Copy link
Member

MartinNowak commented Feb 13, 2018

Why? Patch coverage in #2079 was low due to a very technical reason (#2079 (comment)), which apparently wasn't understood by any of the reviewers. It seems that coverage rightly failed and should have been rightly overriden by a knowledagable reviewer.
I'm a bit picky here because informal usually means tests are ignored and there aren't any good reasons why a refactoring or newly added code would come without coverage.

Phobos and DMD already use this behavior.

Please reconsider, also it should be threshold: 5 + informal, so that the result at least still reflects the tolerance correctly.

@wilzbach wilzbach deleted the codecov-informational branch February 13, 2018 16:24
@wilzbach
Copy link
Contributor Author

It seems that coverage rightly failed and should have been rightly overriden by a knowledagable reviewer.

Yes, but I opened the PR, s.t. we can use auto-merge (failing CI!).

I'm a bit picky here because informal usually means tests are ignored and there aren't any good reasons why a refactoring or newly added code would come without coverage.

In experience with Phobos + DMD, it's very easy to get below the threshold.
The problem is that CodeCov doesn't take the coverage of the previous diff as threshold, but the overall project coverage.
Here's another example:

dlang/phobos#6041 (comment)

So if you touch the wrong file or function which isn't as covered as the overall project CodeCov would mark your PR as failed.

Please reconsider, also it should be threshold: 5 + informal, so that the result at least still reflects the tolerance correctly.

Almost all reviewers have the CodeCov extension installed and see the red lines directly on the diff.
Also as mentioned threshold doesn't work for diffs - seems like a "missing feature" from CodeCov.

@MartinNowak
Copy link
Member

Almost all reviewers have the CodeCov extension installed and see the red lines directly on the diff.

Hope they soon manage to convert their plugin to a Firefox web extension.
codecov/browser-extension#44

Also as mentioned threshold doesn't work for diffs - seems like a "missing feature" from CodeCov.

Mmh, that's annoying.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Trivial typos, formatting, comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants