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

New features in 2.7.0 break my tests #773

Closed
Ch4s3 opened this issue Oct 10, 2017 · 11 comments
Closed

New features in 2.7.0 break my tests #773

Ch4s3 opened this issue Oct 10, 2017 · 11 comments

Comments

@Ch4s3
Copy link

Ch4s3 commented Oct 10, 2017

The functionality in controller_transaction.rb seems to blow up my tests with the following error:

ArticlesController GET #show returns http success
     Failure/Error: get :show, params: { id: article.uuid }

     LocalJumpError:
       no block given (yield)
     # /Users/chase/.rvm/gems/ruby-2.3.3/gems/sentry-raven-2.7.0/lib/raven/integrations/rails/controller_transaction.rb:8:in `block (2 levels) in included'
     # /Users/chase/.rvm/gems/ruby-2.3.3/gems/devise-4.3.0/lib/devise/test/controller_helpers.rb:33:in `block in process'
     # /Users/chase/.rvm/gems/ruby-2.3.3/gems/devise-4.3.0/lib/devise/test/controller_helpers.rb:100:in `catch'
     # /Users/chase/.rvm/gems/ruby-2.3.3/gems/devise-4.3.0/lib/devise/test/controller_helpers.rb:100:in `_catch_warden'
     # /Users/chase/.rvm/gems/ruby-2.3.3/gems/devise-4.3.0/lib/devise/test/controller_helpers.rb:33:in `process'
     # ./spec/controllers/articles_controller_spec.rb:45:in `block (3 levels) in <top (required)>'
     # ./spec/rails_helper.rb:49:in `block (2 levels) in <top (required)>'

2.6.3 works just fine. @nateberkopec it looks like someone else mentioned this in the comments on PR #743, so I thought I'd open an issue.

@nateberkopec
Copy link
Contributor

Ah crap I forgot this didn't work correctly, I'll push a fix tomorrow.

@Ch4s3
Copy link
Author

Ch4s3 commented Oct 11, 2017

Awesome! Thanks Nate.

@RavenXce
Copy link

This breaks for me on a normal production GET request.
The update should probably be yanked?

@roodion
Copy link

roodion commented Oct 11, 2017

That is strange that build has passed all 1760 test examples at CI and fails on simple run within controller. Shouldn't this default use-case been covered with test?

@greysteil
Copy link
Contributor

greysteil commented Oct 11, 2017

If it's helpful to have a link to an open-source project with failing specs, there's one here.

Looks like tests are failing on 60% of the PRs Dependabot created for this update this morning, so I'll comment in on all of those and let people know you've got a fix incoming.

@nateberkopec
Copy link
Contributor

Given all the feedback here, yanking 2.7.0.

@frodsan
Copy link

frodsan commented Oct 11, 2017

This change worked for my project:

diff --git a/lib/raven/integrations/rails/controller_transaction.rb b/lib/raven/integrations/rails/controller_transaction.rb
index da12a46..23506ba 100644
--- a/lib/raven/integrations/rails/controller_transaction.rb
+++ b/lib/raven/integrations/rails/controller_transaction.rb
@@ -2,12 +2,10 @@ module Raven
   class Rails
     module ControllerTransaction
       def self.included(base)
-        base.class_eval do
-          around_action do |controller|
-            Raven.context.transaction.push "#{controller.class}##{controller.action_name}"
-            yield
-            Raven.context.transaction.pop
-          end
+        base.around_action do |controller, block|
+          Raven.context.transaction.push "#{controller.class}##{controller.action_name}"
+          block.call
+          Raven.context.transaction.pop
         end
       end
     end

I haven't find a way to reproduce it in a test case though :/

nateberkopec added a commit that referenced this issue Oct 11, 2017
There were a few messups here:

* The Rails controller tests never tested a "happy case" (that is, no exceptions)
* The Rails controller tests didn't check to see the exception they were actually catching - so the LocalJumpError went unnoticed!
* You can't `yield` from the block form of `around_action`
* I'm not sure why I thought class_eval was necessary here - it isn't.

Fix #773
@nateberkopec
Copy link
Contributor

I'm 99.8% sure I got this, but can someone confirm that their applications test's pass on #774?

@greysteil
Copy link
Contributor

Sure thing - checking now.

nateberkopec added a commit that referenced this issue Oct 11, 2017
There were a few messups here:

* The Rails controller tests never tested a "happy case" (that is, no exceptions)
* The Rails controller tests didn't check to see the exception they were actually catching - so the LocalJumpError went unnoticed!
* You can't `yield` from the block form of `around_action`
* I'm not sure why I thought class_eval was necessary here - it isn't.

Fix #773
costis added a commit to reevoo/reevoo_sapience-rb that referenced this issue Oct 11, 2017
There was an issue with gem raven-sentry v2.7.0 so we better pin it to
2.6.x for now.

getsentry/sentry-ruby#773
@nateberkopec
Copy link
Contributor

Fixed in 2.7.1.

@Ch4s3
Copy link
Author

Ch4s3 commented Oct 12, 2017

Thanks Nate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants