-
Notifications
You must be signed in to change notification settings - Fork 12
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
Group support #2
Conversation
Nicer dev experience, to avoid using docker for a relatively simple gem.
@fede-moya any tricks to triggering the CI, it seems it doesn't like me and there is some chance I broke the docker tests but I don't think so. |
Hi @PragTob I will be reviewing this one later today. About,
I guess CircleCi isn't configured to analysis contributions I will try to fix that. |
spec/fixtures/sample.json
Outdated
}, | ||
"coverage": { | ||
"/gem/spec/fixtures/sample.rb": { | ||
"/stub_working_directory/spec/fixtures/sample.rb": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
@@ -14,14 +14,14 @@ | |||
end | |||
|
|||
describe 'format' do | |||
context 'whit line coverage' do | |||
context 'with line coverage' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PragTob Thanks for the house cleaning 🧹 . About your comment around integrations tests, looks like you have a point. Do you think worths adding some integration tests here right ? I could work on that to make this library more reliable. About the CI, CircleCi is now configure to analysis PRs from forked repos so if you trigger the CI with a new commit it should run. Holding approval till we are sure tests pass.
@fede-moya it's still my plan to take this and move it entirely to a simplecov mono repo :) Imo it makes it much easier for everyone if simplecov(core), simplecov-html and simplecov-json (yes I'd probably rename and re release it for consistency) lived in the same repo. Like, top level goes into the three projects, just some top level shenanigans to run all tests etc. - so I don't think it needs to be handled here unless you wanna take a stab at that unification over in the simplecov repo :) |
for reference, added them over here: simplecov-ruby/simplecov#1015 |
No, I don't. I would take that as a compliment 😃 . Go for it, ping me if you think I can assist in any way, I would be happy to help. |
@fede-moya any updates on the CI? I'd love to get this merged and released without completely moving the project 😁 |
@PragTob yeap, as I pointed on #2 (review)
I believe that's accurate. Can you try that ? |
@fede-moya ah, scuzi somehow I missed that 🤦 Pusching new commit now |
yup it's running, beautiful - thank you! also stupid me, I have push rights I could have just pushed to the repo itself from the beginning. |
@PragTob Nice 👍🏼 . I saw it's failing due to rubocop complaining, feel free to make changes to rubocop's rules. |
all good just missed that rubocop was configured :) |
adds the most basic support for groups, along with some fixes. I'm opening another PR on simplecov in a second to also integration test this, as that was quite tough around here.