Rspec 2.14 + Ruby 2.1.0 + Simplecov 0.8.x doesn't pass along the correct exit status
agrobbin opened this issue · 61 comments
Looks like Simplecov 0.8.x, Ruby 2.1.0, and Rspec 2.14 don't quite work together as you would expect. When running bundle exec rspec in that environment (with Rails 4.0.2), the exit code when tests fail is 0, when it should be 1. However, when I run them on Ruby 2.0.0(-p353) with Simplecov 0.8.x, it correctly exits 1. Even weirder, when running them on Ruby 2.1.0 and Simplecov 0.7.1, it also correctly exits 1.
it seems to happen only when combined with rspec javascript test (capybara+poltergeist).
I've created test case. Perhaps a bit too big though.
Travis report: https://travis-ci.org/edogawaconan/capytestruby21/builds/17829008
(pardon the repo name because initially I thought it's capybara bug)
in my case, ruby 2.0 isn't affected though.
I dug a little deeper.
https://travis-ci.org/BanzaiMan/travis_production_test/jobs/18281155 tests this code base, running on Ruby 2.1.0 and using this Gemfile.lock. As you can see, the combination of Ruby 2.1.0, RSpec 2.14.1 and SimpleCov 0.8.2, is not sufficient to show the problem here.
I also added poltergeist for kicks (https://travis-ci.org/BanzaiMan/travis_production_test/jobs/18284015) but the problem does not manifest. Perhaps it is rspec-rails and/or Rails itself playing a part.
#269 (comment) implicates Rails 4.
@BanzaiMan try enabling poltergeist by adding 'js: true' in the test case.
@edogawaconan I don't know how to exercise JavaScript, I guess. At any rate, it seems to me that developers from poltergeist should be involved to track this down.
when I checked last time, same error also occured when using capybara-webkit driver.
@edogawaconan Could it be capybara, then? It's the common factor here, right?
yeah, probably a combination of simplecov, capybara, and .
So, I've finally also bumped into this at work. Mysteriously, it only manifests when I run the whole test suite, not for individual specs that fail (tried both with capybara acceptance spec and a model unit test). Digging in.
Closed #269 in favor of this - I tried reverting the exit status changes from the 0.8 branch and also did a bit of binding.pry around the behaviour, I can not figure out what is going wrong, the behaviour is seemingly totally random :( This is happening with rspec 3 and rails 4 for me by the way. For now, I'm bailing and will downgrade to 0.7.1 myself...
Same issue for us (Simplecov 0.8.x, Ruby 2.1.0, Rspec 2.14 and Rails 4.0.2), rolling back to 0.7.1 fixes it.
Same issue here. Downgrading simplecov to 0.7.1 fixes it.
- Ruby 2.1.0 / 2.1.1
- rspec (2.14.1)
- rspec-rails (2.14.0)
- capybara (2.1.0)
- poltergeist (1.4.1)
just saw a warning on circle-ci (which is pretty awesome) mentioning this issue. is there any known problem with this version and TestUnit?
It's still unclear to me when exactly this is happening. I did try to debug the issue by comparing the code that changed in comparison to 0.7, but nothing obvious came up. It's possible this also happens with test unit if simplecov is the problem (which the downgrade fix suggests).
Might be Rails 4 specific, "bundle exec rspec" returns exit code properly with the following;
rails 3.2.17
ruby 2.1.1
rspec 2.14.1
rspec-rails 2.14.1
capybara 2.2.1
poltergeist from git revision: 09c6570dea3804cf6ea47821081710b6311be445
👎 Releasing a rolled-back gem with the minor version bumped is more appropriate.
Yanking gems is never the answer.
There's a special place in hell for people who yank gems.
😸 i SO agree to that comment!
If I actually knew what change to roll back, believe me I would have done that. Releasing the last 0.7 as a new 0.9 would remove a lot of actual bug fixes and features added in the 0.8 line, so that does not feel like an option to me. It'd just be nice to get this fixed 😿
I propose to make the simplest test case affected by the bug on a new temporary repo validated with Travis, then to pull each commit since 0.7 and push it so Travis will build each one of them.
As a note by #269, the issues (if it is the same, which I think) it is not Rails specific, not RSpec specific and not Ruby 2.0 specific.
@irrationalfab Yes, that would be a great strategy. Unfortunately, the only time I have bumped into this myself so far was on a client project and the test suite was ~5 minutes, which made the debugging extremely time consuming (also, the bug was only showing when running the full test suite...) - I did some reverts and testing against that repo before making this comment, but wasn't able to find a solution in a timely fashion.
It would be great if we had a open source demo repo with a small test suite that displayed this bug - it should be pretty simple to roll back from there.
Is ~1:20 min still too much? Otherwise Alchemy CMS would be such open source project.
I have setup a minimal test case here: https://travis-ci.org/irrationalfab/simplecov-test/builds
This is the build which should have failed (using the head version of simplecov): https://travis-ci.org/irrationalfab/simplecov-test/builds/25769176
Thanks guys. that will be a great help. I will try these out against the past commits as soon as possible to figure out which one broke the exit status thing.
Cool, that seems legit. I'll revert that one soon!
Anyone has any idea what this change actually broke? Only line that may look offending to me is 5143ee6#diff-99ecb32ffe1919f41b2741f74b68dfa8L82 or am I missing something?
That line indeed looks suspicious.
All other changes are trivial or are JRuby specific and people reported this problem with MRI as well.
As I read this line, the change ensures it always exits with a code, since exit @exit_status if @exit_status would not be executed when @exit_status is 0, would it?
I don't know the details but if there is no failure it should let the hosting script exit with the appropriate code.
In Ruby (at least MRI) 0 evals to true, so this line would not be executed if @exit_status is false or nil.
However after the change it would probably map @exit_code nil -> 0. Hence the error.
In bacon it appears to interfere with its at_exit
This is it https://travis-ci.org/hajder/simplecov-test/builds/25775647
EDIT:
Sorry I was too quick. My bad.
This is such a great Detective thread!
On Thu, May 22, 2014 at 3:38 AM, Mike Szyndel notifications@github.comwrote:
This is it https://travis-ci.org/hajder/simplecov-test/builds/25775647
—
Reply to this email directly or view it on GitHubhttps://github.com//issues/281#issuecomment-43872077
.
Ok, I was wrong before so this time I have been more careful.
I re-applied every change line by line on top of commit 68b1f1e and it turns out that... yes. It turns out problem is in changes lines 60-72, and specifically 60.
if covered_percent < SimpleCov.minimum_coverage
$stderr.puts "Coverage (%.2f%%) is below the expected minimum coverage (%.2f%%)." % \
[covered_percent, SimpleCov.minimum_coverage]
@exit_status = SimpleCov::ExitCodes::MINIMUM_COVERAGE
elsif (last_run = SimpleCov::LastRun.read)
diff = last_run['result']['covered_percent'] - covered_percent
if diff > SimpleCov.maximum_coverage_drop
$stderr.puts "Coverage has dropped by %.2f%% since the last time (maximum allowed: %.2f%%)." % \
[diff, SimpleCov.maximum_coverage_drop]
@exit_status = SimpleCov::ExitCodes::MAXIMUM_COVERAGE_DROP
end
end
If I change it to @exit_status = if covered_percent < SimpleCov.minimum_coverage test provided by @irrationalfab passes, if I remove @exit_status = it fails. (mind that I added the assignments INSIDE the block as per offending commit)
WTF?
Also worth noting that change in line 80 (which I suspected previously) results in warning
'exit': no implicit conversion from nil to integer (TypeError)
Ok, got this one. Opened a pull request, unfortunately I have no idea how to test that so if someone can help me I would be very happy.
This bug is a result of quite interesting side-effect. In old code something like that happened (in pseudo-ruby):
@error_code = 0 # since no previous errors
@error_code = if low_coverage
low_cov_err
elsif cov_drop
cov_drop_err
end
So when coverage neither dropped nor was below threshold this block assigned nil to @error_code and last line didn't return anything. (exit @exit_status if @exit_status)
After the change however assignments were moved into if/ifels blocks so original @error_code = 0 is persisted.
My proposed solution is to check if @error_code is a number bigger than zero before returning.
Why is the error code set to 0 in first place? Can't it be left to nil, and return only unless nil?
Issue #5 is mentioned in comment, didn't dig into. @colszowka can you help?
#302 is merged, though a test would really make sense here. I added a suggestion on that over at the other issue. Everyone else who was seeing this behaviour, could you please give the current master a shot against your repo via gem 'simplecov', require: false, github: 'colszowka/simplecov' and report back?
@colszowka with gem 'simplecov', require: 'false', github: 'colszowka/simplecov', a bundle exec rspec throws ruby-2.1.2/gems/bundler-1.6.2/lib/bundler/runtime.rb:76:in 'require': cannot load such file -- false (LoadError). Removing require: 'false' will resolve the issue.
Do require: false instead of 'false'
φ
On Jun 19, 2014 8:41 AM, "Michael Mell" notifications@github.com wrote:
@colszowka https://github.com/colszowka with gem 'simplecov', require:
'false', github: 'colszowka/simplecov', a bundle exec rspec throws
ruby-2.1.2/gems/bundler-1.6.2/lib/bundler/runtime.rb:76:inrequire':
cannot load such file -- false (LoadError). Removingrequire: 'false, will
resolve the issue.—
Reply to this email directly or view it on GitHub
#281 (comment).
@hajder @colszowka et al: sorry for not being helpful lately. It's been a busy week.
http://www.commitstrip.com/en/2014/05/07/the-truth-behind-open-source-apps/
I have seen this issue on 0.8.*, but cannot replicate reliably. I am now running master branch as advised above, and have not seen this issue yet.
Has anyone been able to reliably replicate this? I wonder if we should change our at_exit handling code in general. Perhaps an END block.
@bf4 maybe it's time to release this fix. I don't think it can be more broken than currently is (in 0.8 version)
@hajder you mean master, right? only @colszowka has gem push
yup. lot's of people waiting for this fix, I don't think my commit broke it more than it was ;)
(actually I believe it fixed it!)
Hello Gentlemen, yes, this needs shipping. I just became a dad and hence am slightly busy, but I'll try to get it out over the weekend!
@colszowka Congrats! Enjoy these moments.
If this is fixed in master, can we close this issue? (And for those of you who have put more time into this than me, is there anything in #273 to consider merging?)
Confirmed fixed in master by
- case reported by @fiedl https://travis-ci.org/bf4/wingolfsplattform/builds
- case reported by @edogawaconanhttps://travis-ci.org/bf4/capytestruby21/builds
I've had trouble reproducing https://travis-ci.org/magiclabs/alchemy_cms/jobs/16516963#L403 2
Were there other reproducible failures we could/should test for fixes?
While looking into this, I came across http://www.davekonopka.com/2013/rspec-exit-code.html by @davekonopka 2, 3 which also looked interesting, in general
if defined?(RUBY_ENGINE) && RUBY_ENGINE == "ruby" && RUBY_VERSION >= "1.9"
module Kernel
alias :__at_exit :at_exit
def at_exit(&block)
__at_exit do
exit_status = $!.status if $!.is_a?(SystemExit)
block.call
exit exit_status if exit_status
end
end
end
endno, 0.9 is out.