Explicitly print the exit status when SimpleCov fails the build #688
Conversation
lib/simplecov.rb
Outdated
| @@ -203,7 +203,10 @@ def run_exit_tasks! | |||
|
|
|||
| # Force exit with stored status (see github issue #5) | |||
| # unless it's nil or 0 (see github issue #281) | |||
| Kernel.exit exit_status if exit_status && exit_status > 0 | |||
| if exit_status && exit_status > 0 | |||
| $stderr.printf("SimpleCov failed build with exit %d", exit_status) | |||
There was a problem hiding this comment.
Since this affects all failure modes, it should probably be tested at a more fundamental level than the minimum_coverage.feature cuke. Will be glad to take advice from maintainers on testing this
There was a problem hiding this comment.
I don't think we need to test all permutations, that we hit this branch once and that it prints seems a good enough test for me 👍
There was a problem hiding this comment.
A line feed would have been prettier
lib/simplecov.rb
Outdated
| @@ -203,7 +203,10 @@ def run_exit_tasks! | |||
|
|
|||
| # Force exit with stored status (see github issue #5) | |||
| # unless it's nil or 0 (see github issue #281) | |||
| Kernel.exit exit_status if exit_status && exit_status > 0 | |||
| if exit_status && exit_status > 0 | |||
| $stderr.printf("SimpleCov failed build with exit %d", exit_status) | |||
There was a problem hiding this comment.
Not sure I like the wording here - it's not always a build I run simplecov locally all the time. How about we remove the build and jsut say SimpleCov failed with exit %d ?
lib/simplecov.rb
Outdated
| @@ -203,7 +203,10 @@ def run_exit_tasks! | |||
|
|
|||
| # Force exit with stored status (see github issue #5) | |||
| # unless it's nil or 0 (see github issue #281) | |||
| Kernel.exit exit_status if exit_status && exit_status > 0 | |||
| if exit_status && exit_status > 0 | |||
| $stderr.printf("SimpleCov failed build with exit %d", exit_status) | |||
There was a problem hiding this comment.
I don't think we need to test all permutations, that we hit this branch once and that it prints seems a good enough test for me 👍
|
Thanks for the review, I updated the wording @PragTob |
|
@daemonsy I think you forgot to adjust the test with the new message :) |
|
😅 we know the tests work. Updated them. If you feel this is good enough to merge, feel free to squash the commits |
|
Just a ping @PragTob in case the previous message didn't trigger a notification |
|
Hello, just a bump on this. |
|
Hi @PragTob just doing my rounds to clear pending OS PRs. |
|
Hi, anyone got a chance to look at this yet? thanks 🙏 |
|
Hi hi, just one last bump on this. |
|
👋 Hi @daemonsy - sorry I took a bit of a break from simplecov as there was a lot going on in my life et. al. Also you're not an idiot (commit message) you're a nice person helping in open source! 💚 |
|
No problem @PragTob ! I hope everything is going well. (P.S. thanks for the nice words, one can be both at the same time :P) |
PR #688 added functionality that broke the [`JacobEvelyn/friends`](https://github.com/JacobEvelyn/friends) build (see https://travis-ci.com/JacobEvelyn/friends/jobs/238680478), which runs many CLI processes and parses the output. Some CLI processes are expected to have non-success status codes, but these tests also check that our CLI wrote the correct message to STDERR, and with #688 the output to STDERR is now changed. This commit adds a SimpleCov configuration flag to disable the message added in #688.
Update ruby-simplecov to 0.17.1. 0.17.1 (2019-09-16) =================== Bugfix release for problems with ParallelTests. ## Bugfixes * Avoid hanging with parallel_tests. See [#746](simplecov-ruby/simplecov#746) (thanks [@annaswims](https://github.com/annaswims)) 0.17.0 (2019-07-02) =================== Maintenance release with nice convenience features and important bugfixes. Notably this **will be the last release to support ruby versions that have reached their end of life**. Moving forward official CRuby support will be 2.4+ and JRuby support will be 9.1+. Older versions might still work but no guarantees. ## Enhancements * Per default filter hidden files and folders. See [#721](simplecov-ruby/simplecov#721) (thanks [Renuo AG](https://www.renuo.ch)) * Print the exit status explicitly when it's not a successful build so it's easier figure out SimpleCov failed the build in the output. See [#688](simplecov-ruby/simplecov#688) (thanks [@daemonsy](https://github.com/daemonsy)) ## Bugfixes * Avoid a premature failure exit code when setting `minimum_coverage` in combination with using [parallel_tests](https://github.com/grosser/parallel_tests). See [#706](simplecov-ruby/simplecov#706) (thanks [@f1sherman](https://github.com/f1sherman)) * Project roots with special characters no longer cause crashes. See [#717](simplecov-ruby/simplecov#717) (thanks [@deivid-rodriguez](https://github.com/deivid-rodriguez)) * Avoid continously overriding test results with manual `ResultMergere.store_results` usage. See [#674](simplecov-ruby/simplecov#674) (thanks [@tomeon](https://github.com/tomeon))
What's up?
My team uses SimpleCov at work. Thank you for the work. We have a Jenkins setup that runs specs using parallel_tests.
Recently we turned on
SimpleCov.minimum_coverageand sometimes we started getting random failures. Eventually, we found out that it was because of the way minimum coverage is calculated, certainparallel_testsgroupings had coverage below the minimum. We got final output like this:With parallel_tests and our Jenkins setup, the message about "expected minimum coverage..." wasn't at the end, so it's isn't clear.
During the debugging process, I spent some time looking for words like
failed,failure,exitto try to see if the build was failed by a non exit 0.What this does
With this, I thought it might be a good addition to SimpleCov to explicitly print out exit statuses and that it is failing the build. When there is a non-zero exit, this message is printed during the
run_exit_tasks!method.It would have helped me discover earlier that SimpleCov was exiting non zero. So hopping that this default message will help other users spot the cause of an exit earlier.