codeclimate / ruby-test-reporter

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

Feature - run on Codeship out of the box #156

Closed cknowles closed 7 years ago

cknowles commented 7 years ago

I was following on from https://github.com/codeclimate/ruby-test-reporter/issues/115 and going to patch up the reporter so it works on Codeship without Docker host volume mounts/monkey patches.

I had an initial question to get this patch running - what is committed_at used for when pushed to Code Climate? Not many of the CIs supported seem to provide that as a parameter, I checked a few. A build could contain multiple commits so the current Git code grabs the last commit timestamp. Depending on what it's used for we could get rid of it or set it to Time.now or the build time?

maxjacobson commented 7 years ago

HI @c-knowles,

Thanks for opening this issue.

I spent some time today setting up some repos on codeship with codeclimate test coverage reporting, one using the docker system, and one without.

Happy to report that things seem to work for me. Example repo: https://github.com/codeclimate-testing/smashcut-codeship-docker/pull/1

Relevant files:

I suspect I'm doing something different, to avoid that issue you were facing. When you have some time, will you take a look and see if it jumps out at you?

antoniobg commented 7 years ago

I can't find any major difference between your configuration and mine. The only difference I found was that I also had require 'codeclimate-test-reporter' in my spec_helper.rb, but I removed it and still got the same.

2016-12-30T09:43:27.570ZwebCoverage report generated for RSpec to /pocket_change/coverage. 54983 / 57353 LOC (95.87%) covered.
2016-12-30T09:43:38.792Zrabbitmq
2016-12-30T09:43:38.792Zrabbitmq=WARNING REPORT==== 30-Dec-2016::09:43:38 ===
2016-12-30T09:43:38.792Zrabbitmqclosing AMQP connection <0.736.0> (172.17.0.6:57108 -> 172.17.0.5:5672):
2016-12-30T09:43:38.792Zrabbitmqconnection_closed_abruptly
2016-12-30T09:43:39.463ZwebI, [2016-12-30T09:43:39.462356 #2311]  INFO -- : Reporting coverage data to Code Climate.
2016-12-30T09:43:42.151Zwebfatal: Not a git repository: './.git'
2016-12-30T09:43:42.154Zwebfatal: Not a git repository: './.git'
2016-12-30T09:43:42.157Zwebfatal: Not a git repository: './.git'
2016-12-30T09:43:42.158Zwebbundler: failed to load command: codeclimate-test-reporter (/bundle/bin/codeclimate-test-reporter)
2016-12-30T09:43:42.158ZwebCodeClimate::TestReporter::InvalidPayload: A git commit timestamp was not found in the test report payload
2016-12-30T09:43:42.158Zweb  /bundle/gems/codeclimate-test-reporter-1.0.3/lib/code_climate/test_reporter/payload_validator.rb:16:in `validate'
2016-12-30T09:43:42.158Zweb  /bundle/gems/codeclimate-test-reporter-1.0.3/lib/code_climate/test_reporter/payload_validator.rb:11:in `validate'
2016-12-30T09:43:42.158Zweb  /bundle/gems/codeclimate-test-reporter-1.0.3/lib/code_climate/test_reporter/formatter.rb:27:in `format'
2016-12-30T09:43:42.158Zweb  /bundle/gems/codeclimate-test-reporter-1.0.3/lib/code_climate/test_reporter.rb:19:in `run'
2016-12-30T09:43:42.159Zweb  /bundle/gems/codeclimate-test-reporter-1.0.3/bin/codeclimate-test-reporter:16:in `<top (required)>'
2016-12-30T09:43:42.159Zweb  /bundle/bin/codeclimate-test-reporter:23:in `load'
2016-12-30T09:43:42.159Zweb  /bundle/bin/codeclimate-test-reporter:23:in `<top (required)>'
maxjacobson commented 7 years ago

@antoniobg Thanks for sharing. My hypothesis is that this line adds the .git directory into the image, and maybe your Dockerfile doesn't have a similar line. Does that sound plausible?

In the meantime, I'm investigating whether there would be any consequences to adding a fallback so we send a timestamp even when git is not available. If it seems fine, we can do that and avoid the dependency on git

maxjacobson commented 7 years ago

I'm able to reproduce the behavior if I add a .dockerignore file which ignores .git. That's another thing you can check for.

Codeship exposes some information via environment variables, but it doesn't expose a committed_at value.

After some digging, I can confirm that we do use the committed_at value (to help figure out which test report to compare yours to), so I'm not comfortable changing the validation to omit it. We do need to send it.

I'll send an email to Codeship asking about adding that environment variable, but I'm not sure if they'll be able to do so. In the meantime, I believe you should be able to make this work by making sure the .git folder is included in your image by checking your Dockerfile and .dockerignore.

antoniobg commented 7 years ago

Thank you very much @maxjacobson, removing the .git folder from the .dockerignore file fixed the issue.

maxjacobson commented 7 years ago

Glad to hear it! Going to close this for now, but let me know if anything else comes up :)

cknowles commented 7 years ago

Unfortunately removing the ignore of .git along with https://github.com/docker/docker/issues/12886 means that .git will get included in the main runtime container outside of the build system. In our case, we specifically try to minimise any contents of the runtime container (this is quite a common use case though). Since .git does not serve any purpose while the app is running we don't really want it there and it can cause significant bloat. I missed it from the original description but that was where the volume mount part came from - i.e. mounting .git as a volume. So I suppose this problem is caused by multiple things combining.

If we can't omit committed_at there are probably some other options, if there are multiple commits I assume the reporter would need the latest date? It may be the .git mount is the only viable workaround right now. I kind of assume if the majority of our commits are sequential that just using the build date would work so maybe an environment variable override could work.

antoniobg commented 7 years ago

In the end the approach of including the .git folder is not going to work for us. I've forked the repo to accept and env variable COMMITTED_AT that we explicitly pass to the ruby-test-reporter and I've created a PR https://github.com/codeclimate/ruby-test-reporter/pull/168 in case it could be useful

maxjacobson commented 7 years ago

@c-knowles @jobwat @grodowski @polmiro With special thanks to @antoniobg, I'm happy to share that we've merged #168 and included it in v1.0.5. Please give it another try and let us know if anything else comes up.

cknowles commented 7 years ago

@antoniobg can I ask where you retrieve COMMITTED_AT from? I'm considering wiring it up to Codeship CI_TIMESTAMP, although it's not the same timestamp all the time as far as I can tell that should work in most cases.

jobwat commented 7 years ago

@maxjacobson I tried v1.0.5 but no success...

2017-01-20T05:37:21.129ZInstalling codeclimate-test-reporter 1.0.5
...
2017-01-20T05:38:30.565Ztests42 runs, 79 assertions, 0 failures, 0 errors, 0 skips
2017-01-20T05:38:30.646ZtestsCoverage report generated for MiniTest to /home/app/my-project/coverage. 735 / 808 LOC (90.97%) covered.
2017-01-20T05:38:31.462ZtestsI, [2017-01-20T05:38:31.461981 #286]  INFO -- : Reporting coverage data to Code Climate.
2017-01-20T05:38:31.495Ztestsfatal: Not a git repository: './.git'
2017-01-20T05:38:31.497Ztestsfatal: Not a git repository: './.git'
2017-01-20T05:38:31.497Ztestsbundler: failed to load command: codeclimate-test-reporter (/home/app/my-project/vendor/bundle/ruby/2.3.0/bin/codeclimate-test-reporter)
2017-01-20T05:38:31.498ZtestsCodeClimate::TestReporter::InvalidPayload: A git commit timestamp was not found in the test report payload
2017-01-20T05:38:31.498Ztests  /home/app/my-project/vendor/bundle/ruby/2.3.0/gems/codeclimate-test-reporter-1.0.5/lib/code_climate/test_reporter/payload_validator.rb:16:in `validate'
2017-01-20T05:38:31.498Ztests  /home/app/my-project/vendor/bundle/ruby/2.3.0/gems/codeclimate-test-reporter-1.0.5/lib/code_climate/test_reporter/payload_validator.rb:11:in `validate'
2017-01-20T05:38:31.498Ztests  /home/app/my-project/vendor/bundle/ruby/2.3.0/gems/codeclimate-test-reporter-1.0.5/lib/code_climate/test_reporter/formatter.rb:28:in `format'

I then set CI_COMMITED_AT following @c-knowles comment

CI_COMMITED_AT=$CI_TIMESTAMP bundle exec codeclimate-test-reporter

And boom: working!

Thanks guys

antoniobg commented 7 years ago

@c-knowles We set it to the current time, because in our case it doesn't matter, but using CI_TIMESTAMP looks like a better solution.