Skip to content

Only include React::Rails::TestHelper in test environment #996

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

Merged
merged 1 commit into from
Jul 27, 2019

Conversation

Aesthetikx
Copy link
Contributor

Based on this discussion in the rails repository I determined that the latest version of react-rails (2.5.0, not present in 2.4.7) inadvertently causes action_controller/test_case to be required, which breaks ActionController::Live streaming in development and presumably production environments.

/home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_controller/test_case.rb:23:in `<module:Live>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_controller/test_case.rb:18:in `<module:ActionController>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_controller/test_case.rb:13:in `<top (required)>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `block in require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:257:in `load_dependency'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_dispatch/testing/integration.rb:614:in `<module:Behavior>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_dispatch/testing/integration.rb:610:in `<class:IntegrationTest>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_dispatch/testing/integration.rb:600:in `<module:ActionDispatch>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/actionpack-5.2.3/lib/action_dispatch/testing/integration.rb:12:in `<top (required)>'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `block in require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:257:in `load_dependency'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/activesupport-5.2.3/lib/active_support/dependencies.rb:291:in `require'
  /home/john/.rbenv/versions/2.6.2/lib/ruby/gems/2.6.0/gems/react-rails-2.5.0/lib/react/rails/railtie.rb:61:in `block (2 levels) in <class:Railtie>'

This change only includes React::Rails::TestHelper in the test environment.

Copy link

@pcarn pcarn left a comment

Choose a reason for hiding this comment

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

+1, this fixed my problem

@BookOfGreg
Copy link
Member

Thank you for confirming this does not occur in the 2.4 branch, that helps me know there is no backport needed.
I'll look into why Chrome is breaking the test suite again, Google probably changed how the webdrivers work (again!) and then get this through as a point release as soon as I'm able to test it myself as the code and attached discussion seems sensible.
I remember copying that include code from another gem so yeah this is probably affecting a fair amount of the community.

@BookOfGreg BookOfGreg self-assigned this Jun 20, 2019
@pcarn
Copy link

pcarn commented Jul 24, 2019

@BookOfGreg Can this be merged soon so we can use it? Thanks!

@BookOfGreg
Copy link
Member

I need the ChromeDriver fix first. #1004
The tests won't run without it.
I'd love some help!

@BookOfGreg
Copy link
Member

BookOfGreg commented Jul 27, 2019

@Aesthetikx @pcarn I re-ran the tests with master merged (#1005 , Finished work on #1004 ) and tests passed there.

Released in 2.6.0

@BookOfGreg BookOfGreg merged commit a03a404 into reactjs:master Jul 27, 2019
@kwstannard
Copy link

Noting here for posterity and search crawlers that this was a breaking change for us, causing a NoMethodError #flash on ActionDispatch::Request.

https://github.com/rails/rails/blob/v5.2.6/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L170

We were able to just remove csrf protection on our API controllers to get around the failure as I am under the impression that csrf is not needed for APIs.

I consider this to be a problem with Rails though. It is not reasonable to expect this change to have caused that error.

kwstannard referenced this pull request in rails/rails Nov 30, 2021
this commit removes some direct access to `env`.
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.

5 participants