sdboyer / gps

your dependencies have arrived
MIT License
270 stars 24 forks source link

Check that git, bazaar, and mercurial are installed before running tests that require them #186

Closed spenczar closed 7 years ago

spenczar commented 7 years ago

This is just a quality-of-life thing as a contributor.

Fixes #184.

sdboyer commented 7 years ago

Perfect :)

codecov[bot] commented 7 years ago

Codecov Report

Merging #186 into master will decrease coverage by 0.21%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
- Coverage   79.49%   79.27%   -0.22%     
==========================================
  Files          24       24              
  Lines        3804     3798       -6     
==========================================
- Hits         3024     3011      -13     
- Misses        586      589       +3     
- Partials      194      198       +4
Impacted Files Coverage Δ
cmd.go 73.68% <0%> (-3.59%) :x:
deduce.go 78.09% <0%> (-1.67%) :x:
source_manager.go 91.38% <0%> (-0.75%) :x:
solver.go 83.6% <0%> (+0.41%) :white_check_mark:

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 9a583ef...249129b. Read the comment docs.

sdboyer commented 7 years ago

I'm kinda surprised and concerned that this would've resulted in a coverage decrease - that would seem to suggest more effects than this should really have. And this is based on the merge result of #185, which is more reasonable to have had those effects...hmm.

Well, not the end of the world.

One nit, though, before I merge - please limit the first line of git commit messages to 50 chars. I'm not as exacting as Tim Pope about his rules, but I do prefer following them.

spenczar commented 7 years ago

Yeah, the coverage decrease seems pretty weird. I've never used codecov, but I'm trying to figure out where the coverage decrease could have come from.

If you like, I'd be happy to amend the commit message and make a new PR to keep the history clean, I'd prefer to be a good citizen 😄

sdboyer commented 7 years ago

I appreciate it 😄 - just amending the commit and force-pushing to this PR would be fine. No need for a new one.

spenczar commented 7 years ago

This looks it could be a flaky test failure, maybe?


--- FAIL: TestSignalHandling (0.01s)
    manager_test.go:809: time to send signal: 13.449µs
    manager_test.go:812: time to return from Release(): 96.1µs
    manager_test.go:815: finished too fast (96.1µs); the necessary network request could not have completed yet
    manager_test.go:850: Expected error on statting what should be an absent lock file```
sdboyer commented 7 years ago

It is - it's a really hacky way of trying to approximate whether or not the signal handling is occurring correctly. Fails maybe 1/50 times. (I had to move on to other things, so went with the crappy heuristic)

Kicking it, should work on the next run...

sdboyer commented 7 years ago

OK, we're good.