-
Notifications
You must be signed in to change notification settings - Fork 11
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
Coverage below 75% #62
Comments
Apparently that's only for the files that actually have unit tests (there are only 6). This doesn't even cover the files that don't have unit tests written. We need more unit tests. |
Will mark issue as resolved after repository coverage goes above |
@DRMF/developers I was skiing two weeks ago, thus I think the milestone "End of summer" should be reached by now;-) Given the experience that code had to be rewritten "from the ground up", I think we should focus on high quality code rather than on many lines. I would like to advocate for the idea of atomic commits. If you have problems to break down the change into small changes (you should try to make at least one commit every time you are in the office) you should probably go back to the drawing board and discuss with others what you want to achieve and how you can break it down to small changes. During this process, you should also define how you can test that the code you added or changed does what it is supposed to do. While this might seem to be more effort in the first place, it will certainly pay off in the future. |
@physikerwelt I think that one of the reasons coverage percentage has stalled is because we have not merged two of the largest pull requests because they are still incomplete. I think @joonbang and @edward-bian have been increasing coverage on their PRs - when they're done, we will probably approach 80-90% coverage. |
@joonbang @edward-bian @KChen1250 See @physikerwelt comment above. |
I see that there is a pull request which would increate the test rate from 59% to 80%. What is the main blocker for merging the request? |
Actually, I haven't written any unit tests for the code in my pull request yet. However, by removing the old code (and what little unit tests there were, like 10%?), it significantly increased coverage. |
Why can't you keep the old unit tests. In theory the should still work!? |
Well, there honestly wasn't many unit tests, and I completely rewrote the code as well, so the individual functions (both uses and outputs) are different. There are also some minor (formatting-wise) differences in the final output. |
This means that only 17% of the code written is being tested by nosetests. The unit tests should be written so that every possibility of input (going into each possible if statement, etc) is covered.
The text was updated successfully, but these errors were encountered: