rycus86 / githooks

Githooks: per-repo and global Git hooks with version control
MIT License
382 stars 20 forks source link

Bugfix/fix coverage #78

Closed gabyx closed 4 years ago

gabyx commented 4 years ago

Some fixes.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 524


Changes Missing Coverage Covered Lines Changed/Added Lines %
base-template.sh 5 7 71.43%
uninstall.sh 4 10 40.0%
<!-- Total: 25 33 75.76% -->
Files with Coverage Reduction New Missed Lines %
uninstall.sh 1 83.94%
install.sh 15 87.55%
<!-- Total: 16 -->
Totals Coverage Status
Change from base Build 515: 3.05%
Covered Lines: 1953
Relevant Lines: 2241

💛 - Coveralls
gabyx commented 4 years ago

I try to correctly mock the download but test 094 still downloads the master I think. pretty weird. will work on it later

gabyx commented 4 years ago

I try to correctly mock the download but test 094 still downloads the master I think. pretty weird. will work on it later

rycus86 commented 4 years ago

Thanks a lot for looking into this! Last time I tried to update kcov, I found that lots have changed in the next version, so I gave up on updating it at the time.

gabyx commented 4 years ago

Last time I tried to update kcov, I found that lots have changed in the next version, so I gave up on updating it at the time.

Tried it too, but lets do that in another PR maybe...

gabyx commented 4 years ago

Now, all coverage works except 116 and I still dont know why, strange... kcov is doing strange things... even the output is scrambled, :-|. Really cumbersome...

gabyx commented 4 years ago

Is there any consense about using git rev-parse --commo-git-dir versus --git-dir. Whould we use everywhere --commo-git-dir Git has to many behaviors :-), not a simple tool :-P

rycus86 commented 4 years ago

Now, all coverage works except 116 and I still dont know why, strange... kcov is doing strange things... even the output is scrambled, :-|. Really cumbersome...

Yeah, that output gets scrambled unfortunately, never really worked out how to fix it properly. If you look at the generated coverage report in HTML, you can see which lines were executed vs which ones did you expect to be executed.

Is there any consense about using git rev-parse --commo-git-dir versus --git-dir. Whould we use everywhere --commo-git-dir Git has to many behaviors :-), not a simple tool :-P

Nope, I have no personal preference, except whichever existed first, so we don't limit compatibility with older Git versions if not necessary.

gabyx commented 4 years ago

Ok. I think this PR is good to go. 116 is still a mystery. Kcov is strange because sometimes not the whole output is shown and it stops suddenly (or just because we cannot see the whole output) and its pretty unobvious where it stopped. I will see if I can detect that in the report. A set -x lets everything go nuts :-)... Updating is an option? If I have time I will look into this.

gabyx commented 4 years ago

https://travis-ci.org/rycus86/githooks/builds/645188064

gabyx commented 4 years ago

Maybe we can leave test 116 out of the coverage, just to make it build and let the users know the build is fine. 116 should be fixed, will look into it soon hopefully with more details.

rycus86 commented 4 years ago

Added some comments, but looks good in general. We can get this in and leave test 116 as is for now, just look at the comments please. Thanks!

gabyx commented 4 years ago

I did some review cleanup, and changed every relevant point to --git-common-dir as its the right thing to do. And I have seen you already did some tests for git worktrees :+1:

Could you please review the last changeset. If you are happy, maybe you can directly merge it, without having me to squash it :-)

gabyx commented 4 years ago

I will also try to replace all cd "path" && git ... to git -C "path" quickly.

gabyx commented 4 years ago

All tests work. Covarage still the same.

rycus86 commented 4 years ago

Thanks a lot!