Closed veskoo93 closed 7 years ago
@veskoo93 Thanks. Give me a little bit to review.
Merging #28 into master will increase coverage by
20.7%
. The diff coverage isn/a
.
@@ Coverage Diff @@
## master #28 +/- ##
============================================
+ Coverage 52.42% 73.13% +20.7%
- Complexity 275 350 +75
============================================
Files 29 29
Lines 1381 1381
Branches 277 277
============================================
+ Hits 724 1010 +286
+ Misses 488 194 -294
- Partials 169 177 +8
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 6adf8a6...5b382f4. Read the comment docs.
@veskoo93 check out this: https://codecov.io/gh/Syncleus/Ferma/tree/884fc17c1ce984cdf541286c4547bfd49a38fd1a/src/main/java/com/syncleus/ferma
coverage is currently around only 73%. looks like there is still some room for improvement, would you agree?
Yes, it seems like percentages differ. I tracked code coverage via the generated reports from jacoco. Jacoco seems to calculate percentage based on covered instructions (however jacoco defines this term) and codecov calculates percentage based on covered lines hence the difference. I saw a few spots that I missed to test. I will write these tests tomorrow and notify you. I will also make sure there are minimum issues according to codacy. If you would like me to test something specific please do tell.
@veskoo93 the key is just to make sure any lines that codecov says are not covered are impossible to cover. Most of the stuff being reported as not covered right now would be possible, though I recognize there are a few that arent.
I think codecov actually just reports via jacoco, so should be the same, but it has been a while since i set up the services. Anyway let me know if you hit a deadend on covering what remains and i can try to give you advice on how to cover anything you might have missed.
@veskoo93 also make sure that the unit tests actually check assertions (make sure the values returned from the test are what was expected). There shouldnt be any unit tests that just run code without asserting that the results are sane.
@veskoo93 also dont set tests to ignore. If it is failing due to a legitimate bug let it run and fail. That means I need to fix it (and will).
@veskoo93 I created a new PR and a new branch where i fixed the failing tests and squashed all your commits. Lets work off that one moving forward so I'm going to close this PR, dont worry I really like your work so far.
Multiple tests were added to test code that was not previously reached. There are a few ignored tests that fail (mostly passing null key/values array when creating edge or vertex).