New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock simplecov between 0.10 and 0.13 version #181

Merged
merged 4 commits into from Mar 20, 2017

Conversation

Projects
None yet
5 participants
@bliof
Contributor

bliof commented Mar 18, 2017

Done because the 0.14.0 version introduces some changes that break the
CodeClimate::TestReporter::Formatter#merge_results
colszowka/simplecov@a7747c1

Check also:

@bliof

This comment has been minimized.

Show comment
Hide comment
@bliof

bliof Mar 18, 2017

Contributor

@gordondiggs follow-up from #180

Contributor

bliof commented Mar 18, 2017

@gordondiggs follow-up from #180

Lock simplecov between 0.10 and 0.13 version
Done because the 0.14.0 version introduces some changes that break the
`CodeClimate::TestReporter::Formatter#merge_results`
colszowka/simplecov@a7747c1

Check also:
  * SimpleCov::Result - https://github.com/colszowka/simplecov/blob/v0.14.0/lib/simplecov/result.rb
  * SimpleCov::RawCoverage - https://github.com/colszowka/simplecov/blob/v0.14.0/lib/simplecov/raw_coverage.rb
@maxjacobson

Hi @bliof, thanks for this PR and very good catch.

Show outdated Hide outdated lib/code_climate/test_reporter/version.rb Outdated
Show outdated Hide outdated codeclimate-test-reporter.gemspec Outdated

maxjacobson added some commits Mar 20, 2017

Undo version change
Our process is to make changes like this while preparing a new release.

@maxjacobson maxjacobson merged commit 9971c7e into codeclimate:master Mar 20, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate no new or fixed issues
Details
codeclimate/coverage 88.27% (-0.8%)
Details

@bliof bliof deleted the bliof:lock-simplecov branch Mar 20, 2017

@franklinyu

This comment has been minimized.

Show comment
Hide comment
@franklinyu

franklinyu Mar 27, 2017

Will v1.1.x branch in the future support SimpleCov v0.14.x?

franklinyu commented Mar 27, 2017

Will v1.1.x branch in the future support SimpleCov v0.14.x?

@colszowka

This comment has been minimized.

Show comment
Hide comment
@colszowka

colszowka Apr 6, 2017

Hey there, It'd be very nice if this bug could be actually fixed instead of pinning to an outdated version of SimpleCov ❤️

The underlying change in SimpleCov is the replacement of Hash and Array core extensions with utility methods that do this, see the related PR that introduced the change It should be fairly easy to change this gem's code to use the new API, swapping the old call to original_result.merge_resultset (although I am not quite sure why this gem needs to deal with resultset merging in the first place, as this is a low-level functionality of simplecov and I'd expect code climate would only need to consume the final (merged) result)

As a side note, this issue only arises when running multiple test suites (i.e. rspec and cucumber). On an app I am currently working on, we had no problems running codeclimate with SimpleCov 0.14.

colszowka commented Apr 6, 2017

Hey there, It'd be very nice if this bug could be actually fixed instead of pinning to an outdated version of SimpleCov ❤️

The underlying change in SimpleCov is the replacement of Hash and Array core extensions with utility methods that do this, see the related PR that introduced the change It should be fairly easy to change this gem's code to use the new API, swapping the old call to original_result.merge_resultset (although I am not quite sure why this gem needs to deal with resultset merging in the first place, as this is a low-level functionality of simplecov and I'd expect code climate would only need to consume the final (merged) result)

As a side note, this issue only arises when running multiple test suites (i.e. rspec and cucumber). On an app I am currently working on, we had no problems running codeclimate with SimpleCov 0.14.

oboxodo added a commit to Quimbee/ruby-test-reporter that referenced this pull request Apr 8, 2017

@ale7714

This comment has been minimized.

Show comment
Hide comment
@ale7714

ale7714 Apr 10, 2017

Member

Hi @colszowka, thank you for your feedback! Currently we're working on a new unified test reporter that will support multiple coverage report formats and parallelized reporting. This new reporter don't rely on low-level functionality of SimpleCov and instead consumes the final result.

We're focusing most of our development efforts on this new tool. It's currently on alpha and we're currently using it with the last version of SimpleCov successfully. I recommend users of SimpleCov v0.14.x to review and try our new test reporter.

Member

ale7714 commented Apr 10, 2017

Hi @colszowka, thank you for your feedback! Currently we're working on a new unified test reporter that will support multiple coverage report formats and parallelized reporting. This new reporter don't rely on low-level functionality of SimpleCov and instead consumes the final result.

We're focusing most of our development efforts on this new tool. It's currently on alpha and we're currently using it with the last version of SimpleCov successfully. I recommend users of SimpleCov v0.14.x to review and try our new test reporter.

@franklinyu

This comment has been minimized.

Show comment
Hide comment
@franklinyu

franklinyu Apr 10, 2017

@ale7714 The manual page indicates that the unified reporter gets the data from ./coverage/.resultset.json in Ruby projects. Would that JSON format change over time? Should we pin the SimpleCov version? We don't need to worry about that if we're using a gem.

franklinyu commented Apr 10, 2017

@ale7714 The manual page indicates that the unified reporter gets the data from ./coverage/.resultset.json in Ruby projects. Would that JSON format change over time? Should we pin the SimpleCov version? We don't need to worry about that if we're using a gem.

@ale7714

This comment has been minimized.

Show comment
Hide comment
@ale7714

ale7714 Apr 10, 2017

Member

@franklinyu that's correct. We're parsing .resulset.json from SimpleCov to a Code Climate payload. For now, it's not needed to pin SimpleCov to a particular version. If we find some issue it require to use a particular version, we will update our docs.
Let me know if you have any other question, also feel free to open issues on the test reporter repo if you find any because we're still on an alpha stage.

Member

ale7714 commented Apr 10, 2017

@franklinyu that's correct. We're parsing .resulset.json from SimpleCov to a Code Climate payload. For now, it's not needed to pin SimpleCov to a particular version. If we find some issue it require to use a particular version, we will update our docs.
Let me know if you have any other question, also feel free to open issues on the test reporter repo if you find any because we're still on an alpha stage.

@colszowka

This comment has been minimized.

Show comment
Hide comment
@colszowka

colszowka Apr 11, 2017

Thanks for the pointer @ale7714, I will try out the new reporter.

I would not advise towards using the resultset.json file since it's an internal cache only meant for simplecov's merging mechanism and you're therefore betting yourselves on an internal data structure. I also would really not like pinning to a particular version of simplecov again later down the road when the approach of using an internal cache file turns out to fail.

I would strongly recommend that you build your own simplecov formatter (it's easy). I don't know if it's fully functional, but https://github.com/vicentllongo/simplecov-json might be a good starting point. This way you can vendorize your dependency on json output from simplecov and guard your customers against pinning potentially buggy and outdated versions.

Disclaimer: I'm the author of simplecov.

colszowka commented Apr 11, 2017

Thanks for the pointer @ale7714, I will try out the new reporter.

I would not advise towards using the resultset.json file since it's an internal cache only meant for simplecov's merging mechanism and you're therefore betting yourselves on an internal data structure. I also would really not like pinning to a particular version of simplecov again later down the road when the approach of using an internal cache file turns out to fail.

I would strongly recommend that you build your own simplecov formatter (it's easy). I don't know if it's fully functional, but https://github.com/vicentllongo/simplecov-json might be a good starting point. This way you can vendorize your dependency on json output from simplecov and guard your customers against pinning potentially buggy and outdated versions.

Disclaimer: I'm the author of simplecov.

@franklinyu

This comment has been minimized.

Show comment
Hide comment
@franklinyu

franklinyu Apr 11, 2017

I agree with @colszowka. Depending on private interface is always fragile, and I don't think customers would be happy about that. There may be a time gap between SimpleCov release a new version and your team update the documentation; if a customer updates his SimpleCov during this gap, he/she is under risk that he/she can't be aware of. Since the unified reporter is still in early stage (there's only a single alpha release), it's time to make the switch before it's too late.

franklinyu commented Apr 11, 2017

I agree with @colszowka. Depending on private interface is always fragile, and I don't think customers would be happy about that. There may be a time gap between SimpleCov release a new version and your team update the documentation; if a customer updates his SimpleCov during this gap, he/she is under risk that he/she can't be aware of. Since the unified reporter is still in early stage (there's only a single alpha release), it's time to make the switch before it's too late.

@ale7714

This comment has been minimized.

Show comment
Hide comment
@ale7714

ale7714 Apr 11, 2017

Member

@colszowka thank you for your input! I appreciate it. I will review more about implementing our own simplecov formatter. I will bring your recommendations to the team.

Member

ale7714 commented Apr 11, 2017

@colszowka thank you for your input! I appreciate it. I will review more about implementing our own simplecov formatter. I will bring your recommendations to the team.

@franklinyu

This comment has been minimized.

Show comment
Hide comment
@franklinyu

franklinyu Apr 12, 2017

The current (low-level) workflow of the unified reporter, for non-parallel build, is

./cc-test-reporter format-coverage
./cc-test-reporter upload-coverage

It seems easy to just replace the first step with a Rake task provided by cc-simplecov-json (the imaginary formatter name):

bundle exec rake cc:coverage:format
./cc-test-reporter upload-coverage

franklinyu commented Apr 12, 2017

The current (low-level) workflow of the unified reporter, for non-parallel build, is

./cc-test-reporter format-coverage
./cc-test-reporter upload-coverage

It seems easy to just replace the first step with a Rake task provided by cc-simplecov-json (the imaginary formatter name):

bundle exec rake cc:coverage:format
./cc-test-reporter upload-coverage

timbru31 added a commit to timbru31/ruby-test-reporter that referenced this pull request Apr 18, 2017

chore: remove simplecov version pinning
as per codeclimate#181 (comment), the issue only occurs when using multiple test suites

glebm added a commit to glebm/i18n-tasks that referenced this pull request Jan 18, 2018

Lock simplecov to work around CodeClimate issue
They depend on internal APIs, see:
codeclimate/ruby-test-reporter#180
codeclimate/ruby-test-reporter#181

This is a second rewrite of their test reporter but they don't plan on
fixing this bug because they are working on the third rewrite...

I'm tired of having to deal with CodeClimate breakages on all my
projects so I'll be moving away from it. If you know a good alternative,
**well-written**, with support for aggregating multiple test suites,
please comment on this commit message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment