search

codeclimate / ruby-test-reporter

DEPRECATED Uploads Ruby test coverage data to Code Climate
https://codeclimate.com
Other
92 stars 92 forks source link

Prefer CI info over git if present #190

Closed andrewhood125 closed 7 years ago

andrewhood125 commented 7 years ago

Resolves #189

andrewhood125 commented 7 years ago

@maxjacobson thanks for pointing me to that! I'll take a look!

andrewhood125 commented 7 years ago

@maxjacobson updated! 👍

andrewhood125 commented 7 years ago

Hmm.. looking at fails.

andrewhood125 commented 7 years ago

No safe operator. 😢

andrewhood125 commented 7 years ago

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 ?

maxjacobson commented 7 years ago

I think I know what's happening. The expectation that's not met is here:

       -:git => {:head=>"7a36651c654c73e7e9a6dfc9f9fa78c5fe37241e", :committed_at=>1474318896, :branch=>"master"},
       +:git => {:head=>"73a72f2142b1dbba41cefa0181b996d856f7ee4b", :committed_at=>1474318896, :branch=>"master"},

The head is expected to be the latest sha in this compressed repository https://github.com/codeclimate/ruby-test-reporter/blob/master/spec/fixtures/fake_project.tar.gz , but instead, when the test runs on Circle CI, it prefers the value from the environment (because of your change) and fails. When you run the test locally, no CI-specific environment variables are set, so it uses the one from the repository (and passes).

Does that make sense? I think you can probably avoid this failure by stubbing something.

andrewhood125 commented 7 years ago

Ah I see it now. I glazed over those shas thinking they were the same 👍 Thanks for highlighting that.

andrewhood125 commented 7 years ago

@maxjacobson updated.

andrewhood125 commented 7 years ago

@maxjacobson look good to you?

maxjacobson commented 7 years ago

@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 branch and committed_at.

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.

andrewhood125 commented 7 years ago

👌 got the new test reporter working.