Rspec 2.14 + Ruby 2.1.0 + Simplecov 0.8.x doesn't pass along the correct exit status #281

Closed
agrobbin opened this Issue Jan 22, 2014 · 61 comments

Projects

None yet
@agrobbin

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.

@nanaya
nanaya commented Jan 29, 2014

it seems to happen only when combined with rspec javascript test (capybara+poltergeist).

@nanaya
nanaya commented Jan 29, 2014

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)

@BanzaiMan

Is this a dupe of #269?

@nanaya
nanaya commented Jan 29, 2014

in my case, ruby 2.0 isn't affected though.

@BanzaiMan

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.

@BanzaiMan

#269 (comment) implicates Rails 4.

@nanaya
nanaya commented Feb 5, 2014

@BanzaiMan try enabling poltergeist by adding 'js: true' in the test case.

@BanzaiMan

@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.

@nanaya
nanaya commented Feb 6, 2014

when I checked last time, same error also occured when using capybara-webkit driver.

@BanzaiMan

@edogawaconan Could it be capybara, then? It's the common factor here, right?

@nanaya
nanaya commented Feb 6, 2014

yeah, probably a combination of simplecov, capybara, and .

@colszowka
Owner

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.

@colszowka
Owner

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...

@colszowka colszowka added a commit that referenced this issue Feb 12, 2014
@colszowka Added notice about #281 5e2b5b3
@thibaudgg

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.

@tmikoss
tmikoss commented Mar 13, 2014

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)
@jcoyne jcoyne added a commit to projectblacklight/spotlight that referenced this issue Mar 13, 2014
@jcoyne jcoyne Pin simplecov to 0.7.1 81fd992
@jcoyne jcoyne referenced this issue in projectblacklight/spotlight Mar 13, 2014
Merged

Pin simplecov to 0.7.1 #531

@alakra alakra added a commit to alakra/ndfd-weather-forecast-client that referenced this issue Mar 13, 2014
@alakra alakra Fixing several items for 1.1.0 release
  * Added NDFD::DWML class to handle parsing of XML responses
  * Added Travis CI testing on 2.1.1
  * Explicitly changed to simplecov 0.7.1 because of outstanding bug (see colszowka/simplecov#281)
  * Refreshed VCR cassettes
  * Removed GML api methods (the endpoints on the service don't work as expected)
  * Removed XSLT translation (it did not handle responses with multiple lat/longs) in favor of pure ruby parsing
  * Removed vendor folder (previous reference to noaa-dwml-to-json-xslt)
  * Updated documentation regarding GML methods
39f17ff
@suryagaddipati suryagaddipati added a commit to suryagaddipati/simplecov that referenced this issue Mar 13, 2014
@colszowka @suryagaddipati + suryagaddipati Added notice about #281 6684740
@phoet
phoet commented Apr 19, 2014

just saw a warning on circle-ci (which is pretty awesome) mentioning this issue. is there any known problem with this version and TestUnit?

@colszowka
Owner

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).

@zaburt
zaburt commented Apr 21, 2014

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 
@ugoa ugoa referenced this issue in mtchavez/circleci Apr 30, 2014
Closed

res.body returns a String instead of a Array #7

@bf4 bf4 added a commit to bf4/simplecov that referenced this issue May 2, 2014
@colszowka @bf4 + bf4 Added notice about #281 1f986e6
@afeld
afeld commented May 22, 2014

Thoughts about yanking the affected gem versions?

@BM5k
BM5k commented May 22, 2014

👎 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.

@phoet
phoet commented May 22, 2014

😸 i SO agree to that comment!

@colszowka
Owner

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 😿

@fabiopelosin

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.

@fabiopelosin

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.

@colszowka
Owner

@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.

@tvdeyen
tvdeyen commented May 22, 2014

Is ~1:20 min still too much? Otherwise Alchemy CMS would be such open source project.

https://github.com/magiclabs/alchemy_cms

@fabiopelosin

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

@colszowka
Owner

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.

@colszowka
Owner

Cool, that seems legit. I'll revert that one soon!

@hajder
Contributor
hajder commented May 22, 2014

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?

@fabiopelosin

That line indeed looks suspicious.

@hajder
Contributor
hajder commented May 22, 2014

All other changes are trivial or are JRuby specific and people reported this problem with MRI as well.

@rafaelgonzalez

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?

@fabiopelosin

I don't know the details but if there is no failure it should let the hosting script exit with the appropriate code.

@hajder
Contributor
hajder commented May 22, 2014

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.

@fabiopelosin

In bacon it appears to interfere with its at_exit

@hajder
Contributor
hajder commented May 22, 2014

This is it https://travis-ci.org/hajder/simplecov-test/builds/25775647

EDIT:
Sorry I was too quick. My bad.

@pcreux
pcreux commented May 22, 2014

This is such a great Detective thread!

φ http://pcreux.com

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/colszowka/simplecov/issues/281#issuecomment-43872077
.

@hajder
Contributor
hajder commented May 22, 2014

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?

@hajder
Contributor
hajder commented May 22, 2014

Also worth noting that change in line 80 (which I suspected previously) results in warning

'exit': no implicit conversion from nil to integer (TypeError)

@hajder
Contributor
hajder commented May 22, 2014

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.

@fabiopelosin

Why is the error code set to 0 in first place? Can't it be left to nil, and return only unless nil?

@hajder
Contributor
hajder commented May 22, 2014

Issue #5 is mentioned in comment, didn't dig into. @colszowka can you help?

@Zauvohi Zauvohi pushed a commit to Zauvohi/app_radio that referenced this issue May 30, 2014
Zauvohoi Downgrade simplecov to 0.7.1due to a bug fix that affects exit codes …
…to avoid issues. More info about the bug colszowka/simplecov#281
7d28dcb
@mattbostock mattbostock added a commit to gds-operations/vcloud-net_launcher that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
c817755
@mattbostock mattbostock added a commit to gds-operations/vcloud-launcher that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
230a995
@mattbostock mattbostock added a commit to gds-operations/vcloud-walker that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
495efa5
@mattbostock mattbostock added a commit to gds-operations/vcloud-walker that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
67b4a65
@mattbostock mattbostock added a commit to gds-operations/vcloud-core that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
02acf35
@mattbostock mattbostock added a commit to gds-operations/vcloud-core that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
387171f
@mattbostock mattbostock added a commit to gds-operations/vcloud-tools-tester that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
47bf7c7
@mattbostock mattbostock added a commit to gds-operations/vcloud-edge_gateway that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
90ae404
@mattbostock mattbostock added a commit to gds-operations/vcloud-edge_gateway that referenced this issue Jun 13, 2014
@mattbostock mattbostock Pin SimpleCov to version 0.7.1
Pin SimpleCov to version 0.7.1 to avoid the bug in SimpleCov 0.8.x
affecting exit codes:

colszowka/simplecov#281

Note that I had to change `profiles` to `adapters`, as the naming
changed in version 0.8.0[1].

[1]:
colszowka/simplecov@60f4717#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR16
41d716d
@hajder
Contributor
hajder commented Jun 15, 2014

@bf4 since you are maintainer now can you take a look at this?, especially pr #302

@colszowka
Owner

#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?

@mmell
mmell commented Jun 19, 2014

@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.

@pcreux
pcreux commented Jun 19, 2014

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).

@bf4
Collaborator
bf4 commented Jun 20, 2014

@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/

@xaviershay
Collaborator

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.

@bf4
Collaborator
bf4 commented Jul 11, 2014

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.

@hajder
Contributor
hajder commented Jul 11, 2014

@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)

@bf4
Collaborator
bf4 commented Jul 11, 2014

@hajder you mean master, right? only @colszowka has gem push

@hajder
Contributor
hajder commented Jul 11, 2014

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

@colszowka
Owner

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!

@BanzaiMan

@colszowka Congrats! Enjoy these moments.

@colszowka colszowka added a commit that referenced this issue Jul 13, 2014
@colszowka @bf4 + bf4 Added notice about #281 bbc7da0
@bf4
Collaborator
bf4 commented Jul 13, 2014

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?)

@hajder
Contributor
hajder commented Jul 13, 2014

#273 is pretty much the same fix. I think both can be closed.

@bf4 bf4 closed this Jul 14, 2014
@bf4
Collaborator
bf4 commented Jul 17, 2014

Confirmed fixed in master by

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
end
@hajder
Contributor
hajder commented Aug 17, 2014

@bf4 so is this released as a gem in 0.8 branch?

@xaviershay
Collaborator

no, 0.9 is out.

@tkrajcar tkrajcar added a commit to tkrajcar/carbonmu that referenced this issue Aug 28, 2014
@tkrajcar tkrajcar Specify simplecov version ~> 0.9 (colszowka/simplecov#281) e9cdd3c
@paulgoetze paulgoetze added a commit to paulgoetze/ruby-band that referenced this issue Oct 21, 2014
@paulgoetze paulgoetze Update simplecov dependency
Due to issue given here:
colszowka/simplecov#281
5454b25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment