-
Notifications
You must be signed in to change notification settings - Fork 551
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
Fix at_exit hook to cooperate with Minitest #756
Conversation
Fine with me. I never got minitest to work satisfactorily with simplecov. Sounds like you did, so kudos |
Would be nice to have some sort of regression test on this, though I'm not quite sure how. start a new process? and shell out to a file the opens with a required minitest/autorun ? |
Leave it with me. Maybe we can shell out and test the Simplecov output matches an expected coverage. |
Ironically once I've started testing it I can't seem to get it to reproduce. Running it against a real test suite it's fine tho. Going to keep looking at it. |
6d66944
to
c960b1c
Compare
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.
a999aab
to
77fd1d2
Compare
rubocop is complaining: spec/faked_project/minitest/test_helper.rb:8:3: C: Style/StderrPuts: Use warn instead of $stderr.puts to allow such output to be disabled. $stderr.puts "No SimpleCov config file found!" I might get to fixing this up myself tomorrow or so but if you get to it before I do, ofc feel free! |
I have a fix locally but left it broken intentionally on remote as I’m still not 100% sure my fix works in all scenarios and didn’t want a premature merge.
Leave it with me :) going to get to the bottom of it eventually.
…Sent from my iPhone
On Dec 1, 2019, at 9:31 AM, Tobias Pfeiffer ***@***.***> wrote:
rubocop is complaining: spec/faked_project/minitest/test_helper.rb:8:3: C: Style/StderrPuts: Use warn instead of $stderr.puts to allow such output to be disabled.
$stderr.puts "No SimpleCov config file found!"
I might get to fixing this up myself tomorrow or so but if you get to it before I do, ofc feel free!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
I'm still exploring if this solution is right in all cases. I had to re-work the original patch when writing the feature specs, but the original patch indeed worked locally when I used it on a project. I suspect that there's an issue with load order; when Minitest is loaded, the hook can be set correctly and everything functions well. If Minitest isn't loaded, the original A Minitest plugin might work better here, but that still needs to be explored. |
@adam12 thanks a lot for all the work you put into this! |
f1a25d2
to
945afed
Compare
All the tests except for the newest ruby version are failing with a slightly different coverage... which is an interesting edge case 🤔 Not sure what to do about it though... making it conditional, a range, trying to find a bug (I suspect it's in ruby itself...) - thougs @adam12 ? Also, thanks a lot! |
Yeah, I'm not too sure what to make of it right now. I've been pondering it all weekend. I'll probably have to establish which value I believe is correct and then work backwards from there. I'm guessing it's from changes I made because CI for master tests those older versions with correct coverage. I'll keep poking at it. |
@adam12 the failure is from your newly introduced feature test (well done for introducing it and thank you! 💚 ) - so in that sense it is from changes you made, but the actually measurement might always have been broken :) I'd suggest looking at the HTML report and see what's correct |
@adam12 Hey, how's it going? :) Do you wanna keep poking at it or should I see if I could/should take over? It'd be no bother for me, you already did a lot 💚 If you wanna keep poking at it I also understand and will do things elsewhere. I really want to have this as I believe it'll fix quite some issues for people 🚀 |
Hello @PragTob! I haven't forgotten about it but my free time ebbs and flows. Hoping to look at it again this week. If you're interested in taking a poke at the coverage values and seeing if you can spot the discrepancy I'd definitely not oppose :) Cheers. |
Hi @adam12 thank you! I'll first stick with the branch coverage so we can get that relased as a preview and then if you haven't fixed it I'll take a poke at it. Thanks so much! 👌 |
Looks like branch coverage might be our issue here as well? Also, I'll be rebasing against master unless you've started working on this branch? LMK. |
@adam12 nope haven't started working on it. Rebase away! Na that's not branch coverage/branch coverage doesn't seem to be enabled. However, the number of relevant lines seems to have decreased and somewhat rightfully so. I wouldn't think |
#855 is there now trying to get this in. |
And #855 appears to be ready. Closing this in favor of it. |
Update ruby-simplecov to 0.18.5. 0.18.5 (2020-02-25) =================== Can you guess? Another bugfix release! ## Bugfixes * minitest won't crash if SimpleCov isn't loaded - aka don't execute SimpleCov code in the minitest plugin if SimpleCov isn't loaded. Thanks to [@edariedl](https://github.com/edariedl) for the report of the peculiar problem in [#877](simplecov-ruby/simplecov#877). 0.18.4 (2020-02-24) =================== Another small bugfix release 🙈 Fixes SimpleCov running with rspec-rails, which was broken due to our fixed minitest integration. ## Bugfixes * SimpleCov will run again correctly when used with rspec-rails. The excellent bug report [#873](simplecov-ruby/simplecov#873) by [@odlp](https://github.com/odlp) perfectly details what went wrong. Thanks to [@adam12](https://github.com/adam12) for the fix [#874](simplecov-ruby/simplecov#874). 0.18.3 (2020-02-23) =========== Small bugfix release. It's especially recommended to upgrade simplecov-html as well because of bugs in the 0.12.0 release. ## Bugfixes * Fix a regression related to file encodings as special characters were missing. Furthermore we now respect the magic `# encoding: ...` comment and read files in the right encoding. Thanks ([@Tietew](https://github.com/Tietew)) - see [#866](simplecov-ruby/simplecov#866) * Use `Minitest.after_run` hook to trigger post-run hooks if `Minitest` is present. See [#756](simplecov-ruby/simplecov#756) and [#855](simplecov-ruby/simplecov#855) thanks ([@adam12](https://github.com/adam12)) 0.18.2 (2020-02-12) =================== Small release just to allow you to use the new simplecov-html. ## Enhancements * Relax simplecov-html requirement so that you're able to use [0.12.0](https://github.com/colszowka/simplecov-html/blob/master/CHANGELOG.md#0120-2020-02-12)
Ensures that the
at_exit
hook is triggered at the right time when run under Minitest, by using the Minitestafter_run
hook for execution.Basically verbatim of @zenspider's suggested changes.