-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Resolve coverage reporting issues with CodeCov #6290
Comments
@jywarren i think making this as a whole as a hall of fame task is a great idea |
Yes this'd be great!
…On Fri, Dec 13, 2019 at 1:52 PM Sidharth Bansal ***@***.***> wrote:
@jywarren <https://github.com/jywarren> i think making this as a whole as
a hall of fame task is a great idea
(Maybe I am wrong. I think this task is removing code coverage errors from
whole plots2 repo)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6290?email_source=notifications&email_token=AAAF6JZ6GFQLOVD5OKOMUYLQYPKXJA5CNFSM4IWSYACKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG247EY#issuecomment-565563283>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J5WY4G6JM4EFEUBGA3QYPKXJANCNFSM4IWSYACA>
.
|
Published |
Hey, I might work on this but was wondering, how can I test my changes to the code? |
I noticed that for some reason the config file has a section |
Also does the codecov config need to be in the master branch for the reports to work? |
Thanks Uzay-G for your help |
Thans @SidharthBansal. Yeah you can turn it into a hard one instead 👍 |
Now we just need to resolve the conflicts of #5954 and merge into master. |
Please work on any other issue in the meantime. Jeff is on holidays. |
Just referencing a few issues together here. A recent example issue at #6875 (comment) Issues possibly related to parallelization documented at #5931 And follow-up notes and concerns from this process at #5954 ... |
In #6789, coverage was to drop to 50%, but on rebasing, it went up to very little change and all passed. This makes me worry about the parallelization issue? |
OK, coming back here; i think things are OK... notes: https://docs.codecov.io/docs/merging-reports says it can handle parallel reports already with no config needed. https://docs.codecov.io/docs/unexpected-coverage-changes has some useful info too. #7061 was compaining that the diff coverage is not as high as the project (32% vs 80%) so I wonder @Uzay-G if we should give 1% flexibility on this, and how we configure this? |
Maybe we can lower the strictness here? https://docs.codecov.io/docs/codecovyml-reference#section-codecov Also we can ignore some folders if we think that's appropriate... |
Yes, we can set threshold here! https://docs.codecov.io/docs/commit-status#section-threshold |
Yeah I think we need to make it more flexible. I will look at this later tonight when I can! 👍 |
Ok I can make a pull request for the threshold, do you think we should set it around 5-8%? |
Oh, i was thinking more like 1% - isn't it the "wiggle room" for the
overall project coverage to change +/- as a result of the PR? But yes,
let's try it out!
…On Fri, Jan 3, 2020 at 3:51 PM Uzay-G ***@***.***> wrote:
Ok I can make a pull request for the threshold, do you think we should set
it around 5-8%?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6290?email_source=notifications&email_token=AAAF6J3DXFDS24PVGF2MMC3Q36QOTA5CNFSM4IWSYACKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEICBXOY#issuecomment-570694587>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J4OTPO7ASGQCC7H76DQ36QOTANCNFSM4IWSYACA>
.
|
Oh ok yeah alright! I'll make the pull request now for the threshold! 👍 |
After #5931, @kaustubh-nair opened #5954 to switch our test coverage reporting to CodeCov. But there is some issue with coverage esp with the separately threaded tests we have running now. We'd love help debugging/resolving this if anyone has some experience with Travis and coverage monitoring!
The text was updated successfully, but these errors were encountered: