CodeClimate::TestReporter::Git doesn't use branch_from_git_or_ci

[davetapley]

3efa705 introduced CodeClimate::TestReporter::Git, which includes a pair of methods: branch_from_git and branch_from_git_or_ci.

Curiously however, CodeClimate::TestReporter::Git#info uses branch_from_git, which means it doesn't have the fall back to CI logic which branch_from_git_or_ci provides.

I'm not sure if this is intentional, or an oversight?

---

It should be noted that the current implementation causes a failure when running the parallel test formatter without a .git directory (this was also raised on #38).

Specifically ⬇️

CodeClimate::TestReporter::Formatter.new.format SimpleCov.result

Will cause ⬇️

Coverage = 51.21%. fatal: Not a git repository: './.git'
fatal: Not a git repository: './.git'
fatal: Not a git repository: './.git'

Code Climate encountered an exception: CodeClimate::TestReporter::InvalidPayload
A git commit timestamp was not found in the test report payload

When patching it to use branch_from_git_or_ci the payload validation error goes away and the upload appears to be successful, although you will still see:

Coverage = 51.23%. fatal: Not a git repository: './.git'
fatal: Not a git repository: './.git'                   

Note that in my patch I also implement committed_at_or_now to avoid the problem of not being able to pull a commit time stamp without a .git.

---

So: Is there a reason to prefer branch_from_git over branch_from_git_or_ci?

[pbrisbin]

Hey @dukedave. Unfortunately, I don't know that we have anyone around with the historical context to know if this was an oversight or intentional. However, it does look like an oversight to me, and the fixes you've made sound right. Would you be willing to open them as a PR, then I'll ping some more folks and, if no one can come up with reasons against, we'll merge it?

[davetapley]

@pbrisbin absolutely.

I'm not sure if you saw my related #38 (comment)?

I can make a pull with the patch I made, but I don't have the bandwidth to ensure the full "this should work without a .git" semantics I described there.

[pbrisbin]

I did see your related comment. I thought you had answered yourself and decided to create a new issue and this was it -- is that not right?

this should work without a .git

My understanding is that the patch you've been using makes it work without a .git (but still prints some ignorable stderr). Is that right, or is there more broken aspects that you wouldn't be fixing?

[davetapley]

@pbrisbin ah, yeah I can see the confusion.

In my mind I wanted this issue to be specifically regarding the patch I suggested, then #38 could be re-opened to address the general issue of whether a .git is required 'officially', because:

Yeah, my patch makes it work without a .git in our specific configuration (i.e. using Codeship), but can't make any guarantees about whether it'll work generally with out a .git.

[pbrisbin]

Gotcha. I'm inclined to open a new Issue about the confusing docs.

Your patch sounds like a Good Thing, and it's OK if it's not known to be comprehensive with making a .git no longer a requirement in all configurations, IMO. FWIW, I don't think the code paths around .git are very different between configurations and environments, so it's possible your fix could be enough.

[jclif]

We were wondering what the status was on this. Due to the fact that .git is unavailable during out build, we have been unable to get the coverage report to work (we are currently using circleCI).

[pbrisbin]

@jclif sorry, we haven't been able to work on this ourselves yet , so I don't have any updates.

Can you describe some more about your build env, in case there is a workaround? For example, in our builds we also lost .git when moving to running specs via docker. We were able to work around the issue by mounting it in with --volume though.

[jclif]

@pbrisbin That's an interesting suggestion! We are also building with docker, so I'll give it a shot and see if that resolves things. Thanks!

[pbrisbin]

I'm going to close this as stale. While the issue is still technically present, the change in usage makes the description and reproduction instructions no longer relevant. If someone bumps into this again, they should feel free to re-report and we can discuss a fix.

[cknowles]

Can the patch suggested above be included? I think the older version worked fine without the git directory using CI params so this seems to be a regression as far as I can tell. We're in the same boat using codeship so may submit a patch if I get around to it (but would expect this to be covered by code climate).

[pbrisbin]

Can the patch suggested above be included?

If someone were to take the gisted patch, try it out, and open a PR, that would certainly speed things up. Otherwise, it might take us some time to action that ourselves.

I think the older version worked fine without the git directory using CI params so this seems to be a regression as far as I can tell

I'm not sure. We haven't changed anything about handling CI params (e.g. env vars) in the 1.0 work -- For better or worse, I think this has just been broken this way for long time.

We're in the same boat using codeship so may submit a patch if I get around to it

I'd be happy to work with you on a PR. Unfortunately, we have many priorities so, given the low-severity and known workaround for docker, addressing this bug just isn't on our immediate timeline.