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

codecov/project fail #796

Closed
nudles opened this issue Sep 15, 2020 · 6 comments
Closed

codecov/project fail #796

nudles opened this issue Sep 15, 2020 · 6 comments
Assignees

Comments

@nudles
Copy link
Member

nudles commented Sep 15, 2020

Pls check https://github.com/apache/singa/pull/795/checks?check_run_id=1115935742

@chrishkchris
Copy link
Contributor

@moazreyad It is very strange that the test analyzes the coverage on tensor.h in this PR which is totally unrelated

@moazreyad
Copy link
Contributor

There is a problem in this PR because it installs singa in python 3.6 and executes the test cases from python3.7/site-packages folder which does not work. Please fix this different python versions problem.

Note also that the project coverage threshold can be set in .codecov.yml like this:

coverage:
  status:
    project:
      default:
        # basic
        target: auto
        threshold: 5%

The value of threshold can be chosen by the development team based on the quality needs. In the best case, every PR should not reduce the coverage because this means it either added code without test case or it removed (or disabled) the test cases for existing code. However, if the team think this is too strict requirement, we may choose the threshold to 5% or even more. But this means that a PR can be allowed to pass even if it reduces the code coverage too much.

The PR #795 reduces the coverage by 4.97%, so setting the threshold to 5% should make it pass. But this is not recommended solution. We can use it only if the team thinks it is too strict to keep the coverage change always zero or positive. In this case, we may allow threshold of 1% or 2% to pass. But 5% seems high to me and it means there is a lot of code that was not tested and there is a problem. And thanks to the strict code coverage, we found the different python versions problem. Without the strict coverage check, may be this problem will be hidden and will cause other problems later.

@nudles
Copy link
Member Author

nudles commented Sep 17, 2020

After fixing the python version issue, code coverage fails again..
I didn't change the source code in this PR #795

@nudles
Copy link
Member Author

nudles commented Sep 17, 2020

Seems it does not provide the suggestions on how to improve the code coverage.
for example, #797

@moazreyad
Copy link
Contributor

After fixing the python version issue, code coverage fails again..

The coverage is now decreased by 0.01% instead of 4.97%. It still fails because we did not specify a threshold, so any decrease any in the coverage will make test fails. To solve this problem, either add a small threshold like 0.1 % to allow small drops in coverage generally in all PRs, or leave the threshold decision to each pull request reviewer to decide if the decrease in the specific PR is fine and it can be merged or it is not fine and must be fixed.

Seems it does not provide the suggestions on how to improve the code coverage.

Yes, it just reports the coverage results without suggestions on how to fix. This is similar to all the test cases which report the error but of course they do not usually suggest how to fix it.

To improve the coverage, the developer needs to investigate more how to create the suitable test cases to cover his new code, or how not to prevent current test cases from running (like the problem that we had in #795).

@nudles
Copy link
Member Author

nudles commented Sep 18, 2020

Thanks for the explanation.
I think we can let the reviewer to determine if the newly added code should be covered by unit tests.

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

3 participants