Prefer CI info over git if present #190
Conversation
andrewhood125
changed the title
.git if present
There was a problem hiding this comment.
Hi @andrewhood125. I think this change is reasonable, and thanks for contributing.
I flagged two things I'd like you to change, and then I'm happy to merge and cut a new release.
I'd also like to let you know about our new test-reporter which has some additional features and is where we're currently focusing more of our development effort.
| it 'returns the head sha from ci if git is not available' do | ||
| expect(Git).to receive(:git).with("log -1 --pretty=format:'%H'").and_return("") | ||
| expect(Ci).to receive(:service_data).and_return({commit_sha: "4567"}) | ||
| it 'returns the head sha from git if ci is not available' do |
There was a problem hiding this comment.
it looks like we're missing a spec it 'returns the head from CI when both git and ci are available'
| @@ -44,7 +44,8 @@ def head_from_git | |||
| end | |||
|
|
|||
| def head_from_ci | |||
| Ci.service_data[:commit_sha] | |||
| sha = Ci.service_data[:commit_sha] | |||
| return nil if sha.empty? | |||
There was a problem hiding this comment.
as-written, this will always return nil
There was a problem hiding this comment.
Right you are. So weird.. thought I ran this from pry and it returned the sha. I'll switch it up.
|
@maxjacobson thanks for pointing me to that! I'll take a look! |
|
@maxjacobson updated! 👍 |
|
Hmm.. looking at fails. |
|
No safe operator. 😢 |
|
Looks like for some reason the ordering of the formatter spec was switched. I wonder if a less strict compare would be better. Is this a flakey spec? Thoughts @maxjacobson ? |
|
I think I know what's happening. The expectation that's not met is here: The Does that make sense? I think you can probably avoid this failure by stubbing something. |
|
Ah I see it now. I glazed over those shas thinking they were the same 👍 Thanks for highlighting that. |
|
@maxjacobson updated. |
|
@maxjacobson look good to you? |
|
@andrewhood125 hey, thanks for the ping and for working on this. I was about to merge this, but then noticed that my colleague was making a similar change in our new test reporter. After talking it through, I believe this change isn't as thorough as it needs to be. Before merging, we would need to also update this to prefer the CI information for I do appreciate your work, and if you'd like to make that change I'll be happy to merge it. But if you're interested, what do you think about trying out our new test reporter (it's in beta and has some additional cool features)? As of today's release, it should already work for your use-case. |
|
👌 got the new test reporter working. |
Resolves #189