Skip to content
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

Explicitly print the exit status when SimpleCov fails the build #688

Merged
merged 4 commits into from
Jun 25, 2019

Conversation

daemonsy
Copy link
Contributor

@daemonsy daemonsy commented Aug 10, 2018

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_coverage and sometimes we started getting random failures. Eventually, we found out that it was because of the way minimum coverage is calculated, certain parallel_tests groupings had coverage below the minimum. We got final output like this:

13752 examples, 0 failures, 120 pendings
Tests have failed for a parallel_test group.

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, exit to 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.

SimpleCov failed build with exit

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.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A line feed would have been prettier

Copy link
Collaborator

@PragTob PragTob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, only fairly minor comments 👌

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@daemonsy
Copy link
Contributor Author

Thanks for the review, I updated the wording @PragTob

@PragTob
Copy link
Collaborator

PragTob commented Aug 12, 2018

@daemonsy I think you forgot to adjust the test with the new message :)

@daemonsy
Copy link
Contributor Author

😅 we know the tests work. Updated them.

If you feel this is good enough to merge, feel free to squash the commits

@daemonsy
Copy link
Contributor Author

Just a ping @PragTob in case the previous message didn't trigger a notification

@daemonsy
Copy link
Contributor Author

Hello, just a bump on this.

@daemonsy
Copy link
Contributor Author

Hi @PragTob just doing my rounds to clear pending OS PRs.

@daemonsy
Copy link
Contributor Author

Hi, anyone got a chance to look at this yet? thanks 🙏

@daemonsy
Copy link
Contributor Author

Hi hi, just one last bump on this.

@PragTob
Copy link
Collaborator

PragTob commented Jun 25, 2019

👋

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! 💚

@PragTob PragTob merged commit 7770ce8 into simplecov-ruby:master Jun 25, 2019
@daemonsy
Copy link
Contributor Author

No problem @PragTob ! I hope everything is going well.
simplecov is still helping us on every, single, build, so thank you.

(P.S. thanks for the nice words, one can be both at the same time :P)

PragTob pushed a commit that referenced this pull request Sep 29, 2019
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.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 2, 2019
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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants