-
Notifications
You must be signed in to change notification settings - Fork 76
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
Possible Incompatibility with SimpleCov 0.18+ #413
Comments
Thanks for the heads up @PragTob ! Pulling in a dev colleague to take a look. |
Hi @PragTob thank you for reaching out. We would really like to keep this project without requiring integration. Have you considered for simplecov to support a json output of the results? Other test coverage tools offer different formats for the outputs of the result. We would be happy on helping with writing that. |
Hi @ale7714 ! That's what I thought (although I think it's an integration either way) - you mean without adjusting the user simplecov code in any way, correct? We have different output formats, supported through formatters (https://github.com/colszowka/simplecov/blob/master/doc/alternate-formatters.md, one of them is even a JSON formatter but it looks like it's unmaintained as of now: https://github.com/vicentllongo/simplecov-json ) :) Having a JSON output for simplecov is an idea, but if I understand your application correctly you'd rather that the user doesn't have to set anything - in other words you'd want the JSON report to be written on every run - correct? I see how a JSON report could be useful, I just somewhat worry about the format of it (and then the complexity for you to parse it). Ruby now has 4 coverage criteria (lines, branch, method, oneshot lines) and the vision for SimpleCov is to allow all of them to be run individually or together. I guess a structure like:
could work. But one of the problems for your current way of integrating is that this would mean that you also need to be able to process all n types of coverage data we support. Cheers, |
Hi @PragTob,
XCov does something like this and it works out great. Handling the parsing on the test-reporter is fairly straightforward once we know how the template looks like. We have some tools that do lines and branch coverage and we're able to successfully parse the information we need once we get know how the format looks like. I'll reiterate that we're happy to contribute to get the implementation of the JSON output up and running. Although I would defer to you on the template of the format. In general lines the information, currently provided in the resultset is enough for us but if we were able to add additional information link no-cov that would be awesome. Once we have a new template, on our side we will put together a new parser for it. |
Hi @ale7714, it's nice that you offer help for the JSON formatter and I recognize that. I'd have to think up a suitable format and what to do, right now that's not my focus though. The thing that irks me a bit here though is that in the end that we'd create and write a JSON report on every run while that's only necessary for the minority of runs that run on CI for a code climate customer. I made a quick check with 3 other online tools that provide code coverage and all of them integrate with us through a ruby gem (which imo is the right way to do it). Does Code Climate still have an environment variable like "CODE_CLIMATE_TOKEN" or something that could be checked to only write the JSON report (unless configured in other ways) when that is set? |
Hi @PragTob, Thank you for your response. I appreciate you’re taking the time to discuss this with us. I understand that other tools might do things differently. A couple of years ago, we took the direction of building a test coverage reporter tool that could parse output from different coverage tools with the main goal of having a consistent way to setup coverage reporting with Code Climate across different languages. Before writing the tool, we performed some research and we were able to identify that most coverage tools offer a way to output a file (in JSON, XML, or similar) with the test coverage data that we would be able to parse.
Yes, users are setting CC_TEST_REPORTER_ID on their environment. We would love to setup a time to chat with you to discuss how can we partner and support you on this. Please, let us know your availability over email at hello@codeclimate.com or a DM us in Twitter. Looking forward to hearing back from you. |
Thanks I'll get in touch although I thought communication here was mostly fine. And forgot to update here, the version with the incompatible file format was released 2 days ago so this is now a current issue. |
Hi there, Simplecov 0.18.0 is released and we could not use it as it leads to an error with
|
Hi @maximeg - Emily here with Code Climate Support. Thanks for the report! We're working on this now.
If you have any other questions in the meantime, feel free to reach out to hello@codeclimate.com and we'll be happy to help. |
@maximeg I have the same result. Sadly, but right now possible to use simplecov <= 0.17.x with latest cc-test-reporter |
Because codeclimate doesn't support simplecov 0.18 codeclimate/test-reporter#413
Because codeclimate doesn't support simplecov 0.18 codeclimate/test-reporter#413
I didn't add this information as to my mind we didn't change any API documented anywhere and hence it shouldn't appear in a Changelog. However, this affects people and they have to find out for themselves so notifying them of this might be a good thing. I still couldn't bring myself to put it under breaking changes as technically we haven't broken anything that we ever communicated as working. So a link to codeclimate/test-reporter#413 seems to be helpful.
I didn't add this information as to my mind we didn't change any API documented anywhere and hence it shouldn't appear in a Changelog. However, this affects people and they have to find out for themselves so notifying them of this might be a good thing. I still couldn't bring myself to put it under breaking changes as technically we haven't broken anything that we ever communicated as working. So a link to codeclimate/test-reporter#413 seems to be helpful.
Just a note that the main change is the introduction of 'lines' in the hierarchy of the output data from what I found so it may be an easy bandaid for now. Sadly, I don't have the logs present from when I was looking into this but the rest of the data looked the same. :( Edit: I missed that the original post already mentioned this (oops!) |
@sgnn7 that is correct. See my initial post. The problem though is that the current "integration" also already suffers from multiple problems (such as ignored lines not working) and that the file used is internal and it's format might change soon - even drastically (it's an option that it becomes a YAML due to problems encountered with the branch format in JSON). Sure CodeClimate could patch it up, but it's not the most long term stable solution. We'll see what we can up with, if anything. |
I added bare support for simplecov 0.18 in #420 in case someone wants to give it a try. |
Downgrade simplecov in an effort to get CodeClimate code coverage working. See: codeclimate/test-reporter#413.
commit c922580 Author: Artem Kozaev <artemkozaev@gmail.com> Date: Tue Dec 8 22:58:00 2020 +0300 Add email validation errors via add (lynndylanhurley#1445) commit 72ccd90 Author: Hu Hailin <tony.hu.hailin@gmail.com> Date: Wed Dec 9 04:57:39 2020 +0900 rc causes issue for namespaced class, use mapping directly to keep behavior consistency with devise (lynndylanhurley#1440) commit 8dba30b Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Tue Dec 8 16:57:19 2020 -0300 Enable rails 6.1 (lynndylanhurley#1446) commit 575272a Author: Ali Deishidi <sizief@gmail.com> Date: Fri Dec 4 19:20:56 2020 +0330 Update faq.md (lynndylanhurley#1439) Type was missing commit 2a83ef9 Author: dominikdarnel <dominik.darnel@gmail.com> Date: Sun Nov 8 21:51:36 2020 +0100 Update overrides.md (lynndylanhurley#1437) Overrideable methods RegistrationsController#render_destroy_success and RegistrationsController#render_destroy_error were missing from docs. commit 5ef7197 Author: Avrohom Katz <iambpentameter@gmail.com> Date: Sun Nov 1 10:40:30 2020 -0500 make DTA respect controller overrides it doesn't kow about (lynndylanhurley#1435) commit 24da31e Author: Paulo Ribeiro <plribeiro3000@gmail.com> Date: Sun Nov 1 12:39:35 2020 -0300 feat(Route): Accept override (lynndylanhurley#1432) commit 530b4ba Author: JP Rosevear <jprosevear@gmail.com> Date: Mon Oct 19 17:32:04 2020 -0400 Remove unnecessary require_relative the file should be autoloaded (lynndylanhurley#1433) commit 576d0b0 Author: Martin Jaime <50113670+martinjaimem@users.noreply.github.com> Date: Mon Oct 19 18:31:44 2020 -0300 Enhancement: remove unnecessary statement in destroy session (lynndylanhurley#1431) commit 0c369d6 Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Sun Oct 4 17:34:44 2020 -0300 Serialize updated at without to s (lynndylanhurley#1423) * working 4.2 version * cleanup * Lock simplecov to last supported cc version codeclimate/test-reporter#413 Co-authored-by: Brian Dunn <brianpatrickdunn@gmail.com> commit bb3e3f0 Author: Brent Dearth <brent.dearth@gmail.com> Date: Tue Sep 1 11:05:33 2020 -0600 chore(deps): remove Sprockets requirement (lynndylanhurley#1428) commit bef1b0b Author: ngouy <nathangouy@free.fr> Date: Sat Jul 25 14:43:00 2020 -0400 missing space in migration generator (lynndylanhurley#1422) So everything is nicely aligned if you uncomment this commit b409997 Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Sun Jun 21 21:53:29 2020 -0300 Avoid failing on undefined variable for invalid record (lynndylanhurley#1416) commit 30a1aff Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Sun Jun 21 21:52:43 2020 -0300 Avoid nullyfing token when current_user is called before (lynndylanhurley#1417) commit ae3547c Author: Charlie Hua <compassion.wisdom@gmail.com> Date: Sun Jun 21 04:37:15 2020 +0800 Memorize current_#{group_name} to avoid error (lynndylanhurley#722) Currently current_#{group_name} (ex. current_member) is not memorized as current_#{mapping} (ex. current_user) is. Instead, it calls set_user_by_token every time, and this could cause error if current_member is called after token is changed (maybe by some other request) and return nil commit 849c244 Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Sat Jun 20 00:39:42 2020 -0300 Fix parent class name generator for rails 6 (lynndylanhurley#1414) commit 449a0be Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Fri Jun 19 20:09:26 2020 -0300 Reduce config variables doc table width (lynndylanhurley#1413) commit 4461a09 Author: Hai Phan Nguyen <pnghai@gmail.com> Date: Sat Jun 20 05:27:45 2020 +0700 Fix crash on devise 4.7.2+ (lynndylanhurley#1412) * Ignore sync uid in case confirmable mail changed * Update user_omniauth_callbacks.rb * Update user_omniauth_callbacks.rb * fix crash on devise 4.7.2+ commit bb09616 Author: Hai Phan Nguyen <pnghai@gmail.com> Date: Wed Jun 10 20:56:28 2020 +0700 Ignore sync uid in case confirmable mail changed (lynndylanhurley#1407) * Ignore sync uid in case confirmable mail changed * Update user_omniauth_callbacks.rb * Update user_omniauth_callbacks.rb commit b3d53b6 Author: Maicol Bentancor <maicol.bentancor@gmail.com> Date: Tue Jun 2 15:07:22 2020 -0300 Bump version to 1.1.4 (lynndylanhurley#1406) commit 2a01f4f Author: Jamal Mohammed <jamal@mdjamal.com> Date: Tue Jun 2 22:07:49 2020 +0530 Update faq.md (lynndylanhurley#1401) Updated FAQ to add **allow_password_change** column if not already added in existing User model. commit 387306a Author: Sai Chander <saichander17@gmail.com> Date: Wed Apr 29 08:43:10 2020 +0800 Issue - 1358 Argument error when converting token updated_at using to_time fixed (lynndylanhurley#1388) commit a3595f4 Author: David Loukidelis <david@loukidelis.com> Date: Tue Apr 28 20:41:32 2020 -0400 Update omniauth_callbacks_controller.rb (lynndylanhurley#1398) commit ef965d7 Author: Arun Kumar Mohan <arunmohandm@gmail.com> Date: Tue Apr 28 19:39:44 2020 -0500 Fix grammar (lynndylanhurley#1396) commit ea697f1 Author: H.Sada <47932189+h-sada@users.noreply.github.com> Date: Wed Apr 29 09:39:27 2020 +0900 fixed not_email in ja.yml (lynndylanhurley#1395) commit 275da3c Author: Gabriel Bursztein <gabrielbursztein94@gmail.com> Date: Mon Mar 30 08:23:22 2020 -0300 Fix: Save user authentication token after email confirmation (lynndylanhurley#1391) commit 4a103ed Author: Ahmed Magdy <ahmedmagdy711@users.noreply.github.com> Date: Mon Mar 30 13:19:16 2020 +0200 Validate that token is valid for patch request last token (lynndylanhurley#1386) commit 4d2d702 Author: Leo Kiiski <goalaleo@gmail.com> Date: Mon Mar 30 14:18:54 2020 +0300 Fix token-type header key in testing example (lynndylanhurley#1390) commit ab050c2 Author: Dylan Lederle-Ensign <dylan.lederle.ensign@gmail.com> Date: Mon Mar 30 07:17:57 2020 -0400 Fix broken link (lynndylanhurley#1392) commit 8c6ea5a Author: Olle Jonsson <olle.jonsson@gmail.com> Date: Mon Mar 30 13:16:53 2020 +0200 CI build fix: Pin to pry < 0.13 for 2.3 support, workaround CodeClimate reporter issue (lynndylanhurley#1393) * gemfiles: Pin to < 0.13 for 2.3 support * Workaround: CodeClimate not supporting SimpleCov - 0.18 gives us issues - see codeclimate/test-reporter#418 commit 49e7203 Author: K-Sato <32632542+K-Sato1995@users.noreply.github.com> Date: Sat Feb 29 08:17:59 2020 +0900 Fix docs/usage/reset_password.md (lynndylanhurley#1382) commit 55fcd11 Author: Bartek Fijałkowski <brateq@gmail.com> Date: Thu Jan 30 21:35:50 2020 +0100 Add docs for confirmation endpoint (lynndylanhurley#1365) commit e0d8dfa Author: sugiken <ynmk1995@gmail.com> Date: Fri Jan 31 05:35:31 2020 +0900 Remove Trackable option from generator (lynndylanhurley#1362) commit 8326cc7 Author: Bartek Fijałkowski <brateq@gmail.com> Date: Thu Jan 30 21:33:45 2020 +0100 Add rails 6.0 config to travis (lynndylanhurley#1366) * Add rails 6.0 config to travis * Exclude Travis Rails 6.0 checks for Ruby below 2.5 * Updates tests to be compatible with Rails 6 Rails 6 no longer generates migration with id: :uuid, so I changed the range of two ifs in tests and everything goes well now. * Add Ruby 2.7.0 to Travis * Exclude travis test for Ruby 2.7.0 with Rails 4.2 Ruby 2.7 comes with Bundler 2 which is not compatible with Rails 4 * Add to Travis test for rails 6 + mongoid 7 * Downgrade mongoid-locker for rails 6 gemfile * Allow to change primary key type for Rails 6 commit 880a249 Author: Paweł Łuczak <woochaq@gmail.com> Date: Thu Jan 30 21:32:47 2020 +0100 Fix missing polish and portugese translation errors (lynndylanhurley#1377) commit 3680fd3 Author: Clément Prod'homme <prodhomme.clement@hotmail.fr> Date: Wed Jan 29 05:42:35 2020 +0100 chore(docs): write complete path for authentication_test_spec (lynndylanhurley#1376) commit 00d7abc Author: Nicholas Martin <nicholas.martin@marketdojo.com> Date: Thu Jan 9 13:23:36 2020 +0000 Add case sensitive option required to prevent deprecation warning in rails 6 (lynndylanhurley#1368)
I can see there are numerous PRs that keep using simplecov < 0.18 as the "solution" for this issue. Do we have a better solution here? I see #413 (comment) that removed |
From my experience, CodeClimate basically stopped development on their Quality product a couple years ago. Last I heard from them, about a year ago, I had sent in an email basically saying "What's going on? Everything's broken. Will it be fixed?" and they never got back to me, even with followups. So I wouldn't count on this ever being fixed. |
Can only add my voice to @bmulholland; We canceled our subscription with CodeClimate because of this issue and build our own code analyzer and tooling to run the things like RuboCop, Code Coverage etc. |
Hey @david942j @bmulholland @coding-bunny This issue has been solved months ago. The only |
Hi @fede-moya, it didn't work for my project when upgrading SimpleCov from 0.17 to 0.21: https://github.com/david942j/one_gadget/runs/5685500299?check_suite_focus=true |
Oh so you mean I need to set that variable for both |
@david942j yeap |
For my project this issue got fixed by this PR: david942j/one_gadget#191
|
I wrote a new gem that uses the latest Here's how I did it!
There is no longer a compatibility issue. I wrote a more complete article about the setup on dev.to |
@pboling require "simplecov_json_formatter" |
## 🎫 Ticket https://jira.cms.gov/browse/DPC-3971 ## 🛠 Changes Reverted simplecov gem to 0.17 ## ℹ️ Context for reviewers Sonarqube stopped processing portal code coverage on Mar 5; the day the rails upgrade was merged into master. As part of the upgrade simplecov was not pinned to 0.17. Simplecov 0.18+ breaking ci is a [known issue](codeclimate/test-reporter#413). There was comment (present in the other gemfiles) warning not to upgrade, but it was removed because for some reason, when I run `bundle install` after sshing into the docker container, it will fail if there are too many bytes in the Gemfile. I removed "extraneous" comments, which meant I missed this. ## ✅ Acceptance Validation [Ran the build](https://management.dpc.cms.gov/job/DPC%20-%20Docker%20Build/3666/) and [checked the coverage](https://sonarqube.cloud.cms.gov/dashboard?branch=jd%2Fdpc-3971-portal-sonarqube&id=bcda-dpc-portal). ## 🔒 Security Implications - [ ] This PR adds a new software dependency or dependencies. - [ ] This PR modifies or invalidates one or more of our security controls. - [ ] This PR stores or transmits data that was not stored or transmitted before. - [ ] This PR requires additional review of its security implications for other reasons. If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.
Hi there,
SimpleCov maintainer here.
To the best of my knowledge you're using
resultset.json
to do the code coverage reporting to code climate. Due to the introduction of branch coverage the format of this file will change slightly in the next release (beta release just released) - namely there are now top level "lines" and "branches" keys.However, the file format might be changed further in the future due to problems with the standard branch format (we might even switch to YML format or something else). See simplecov-ruby/simplecov#801 for some information.
I don't want to break code climate test reporting, however:
a.) we need these changes to work correctly
b.) resultset.json was never intended or communicated as an integration point - it's merely the way in which we do merging results from different processes
As a result of this, I can't guarantee stability for
resultset.json
, furthermore as it pretty much just dumps and merges the standard coverage data it also doesn't support a lot of features of simplecov (off the top of my hat)::nocov:
is ignored with simplecov #246My suggested approach would be to write a gem/formatter that writes a file in a format that you control and can continue working. (I know this is against the general "no integration needed" idea of this project) I'm happy to work with you to ensure that the APIs you need are stable. I also still have hopes (:crossed_fingers:) to release a 1.0 version soon-ish (couple of months) and to make another pass through the code to make sure that public vs. private APIs in the code are accurately documented. Happy to provide you with more public APIs if you need them.
Cheers and happy new year,
Tobi
The text was updated successfully, but these errors were encountered: