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

rails5 can't access session in ActionDispatch::IntegrationTest #23386

Closed
rubys opened this Issue Jan 31, 2016 · 9 comments

Comments

Projects
None yet
7 participants
@rubys
Contributor

rubys commented Jan 31, 2016

Reproduction scenario:

rm -rf throwaway
rails new throwaway 
cd throwaway
rails generate scaffold Product name
rails db:migrate
rails test
sed -i -e '/patch product/ i\
    session[:userid] = 0\
' test/controllers/products_controller_test.rb
rails test
BACKTRACE=1 rails test

Test results: throwaway.txt

[this is with rails master]

Note: I reran rails test with BACKTRACE set as the actual error is inside action_dispatch/testing/test_process.

Example with similar code working in Rails 4.2:

Code: http://intertwingly.net/projects/AWDwR4/checkdepot-215-42/section-10.4.html#cmd3
Test: http://intertwingly.net/projects/AWDwR4/checkdepot-215-42/section-10.4.html#cmd8

Results for same example in Rails 5 (master):

http://intertwingly.net/projects/AWDwR4/checkdepot/section-10.4.html#cmd8

@rubys rubys changed the title from rails5 can't assess session in ActionDispatch::IntegrationTest to rails5 can't access session in ActionDispatch::IntegrationTest Jan 31, 2016

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 1, 2016

Member

The session is an internal structure for the controller. Integration test is more black box than controller tests were. I don't think we should be testing internals of the controller like this. I would rework the test to test something visible in the UI instead.

Member

dhh commented Feb 1, 2016

The session is an internal structure for the controller. Integration test is more black box than controller tests were. I don't think we should be testing internals of the controller like this. I would rework the test to test something visible in the UI instead.

@dhh dhh closed this Feb 1, 2016

@rubys

This comment has been minimized.

Show comment
Hide comment
@rubys

rubys Feb 1, 2016

Contributor

I fully understand removing access to low level details. But in this case, that just exposes what is the real problem here: http://api.rubyonrails.org/classes/ActionDispatch/Integration/RequestHelpers.html is woefully deficient. Don't get me wrong, it is fine if you are testing a single controller action in isolation. But once you want to test anything that involves session logic in your controller, you end up needing things like this: https://github.com/jnicklas/capybara#clicking-links-and-buttons and https://github.com/jnicklas/capybara#interacting-with-forms.

I don't think that these methods would be hard to write (Rails already has access to response.body and css_selector logic). I don't think they could make the current Rails 5 schedule. I don't think that access to a session should be removed until the higher level functions are in place.

Looking at the problem I'm seeing, it appears that the issue is that the session isn't created yet. Doing an unrelated action (like a get) would create a session.

Contributor

rubys commented Feb 1, 2016

I fully understand removing access to low level details. But in this case, that just exposes what is the real problem here: http://api.rubyonrails.org/classes/ActionDispatch/Integration/RequestHelpers.html is woefully deficient. Don't get me wrong, it is fine if you are testing a single controller action in isolation. But once you want to test anything that involves session logic in your controller, you end up needing things like this: https://github.com/jnicklas/capybara#clicking-links-and-buttons and https://github.com/jnicklas/capybara#interacting-with-forms.

I don't think that these methods would be hard to write (Rails already has access to response.body and css_selector logic). I don't think they could make the current Rails 5 schedule. I don't think that access to a session should be removed until the higher level functions are in place.

Looking at the problem I'm seeing, it appears that the issue is that the session isn't created yet. Doing an unrelated action (like a get) would create a session.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 2, 2016

Member

We tested all of Basecamp 3 with controller tests driven by ActionDispatch::IntegrationTest, and I didn't find a need for capybara-style button clickers etc. I think it'd be nice to offer this as an additional level-up, but it's not a blocker in my book. If that's something you'd like to take a swing at, please do! Would make a great addition to Rails 5.1.

Member

dhh commented Feb 2, 2016

We tested all of Basecamp 3 with controller tests driven by ActionDispatch::IntegrationTest, and I didn't find a need for capybara-style button clickers etc. I think it'd be nice to offer this as an additional level-up, but it's not a blocker in my book. If that's something you'd like to take a swing at, please do! Would make a great addition to Rails 5.1.

@cannikin

This comment has been minimized.

Show comment
Hide comment
@cannikin

cannikin Mar 5, 2016

@dhh Can you explain your technique for this? Do you actually post to your login page at the top of your single action test in the controller test, then commence with testing the action? Something like:

test "should get new" do
  user = users(:john)
  post login_url, params: { email: user.email, password: user.password }

  get new_post_url
  assert_response :success
end

Sorry to bug you but I haven't found any docs in the wider world that explain this new recommended controller testing paradigm.

cannikin commented Mar 5, 2016

@dhh Can you explain your technique for this? Do you actually post to your login page at the top of your single action test in the controller test, then commence with testing the action? Something like:

test "should get new" do
  user = users(:john)
  post login_url, params: { email: user.email, password: user.password }

  get new_post_url
  assert_response :success
end

Sorry to bug you but I haven't found any docs in the wider world that explain this new recommended controller testing paradigm.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 6, 2016

Member

That's exactly it. We've wrapped that into a helper method that we usually then use in the setup of a controller. So:

  # helper method
  def sign_in_as(name)
    post login_url, params: { sig: users(name).perishable_signature )
  end

class SomeControllerTest
  setup { sign_in_as 'david' }

  test 'the truth' do
  ..
Member

dhh commented Mar 6, 2016

That's exactly it. We've wrapped that into a helper method that we usually then use in the setup of a controller. So:

  # helper method
  def sign_in_as(name)
    post login_url, params: { sig: users(name).perishable_signature )
  end

class SomeControllerTest
  setup { sign_in_as 'david' }

  test 'the truth' do
  ..
@cannikin

This comment has been minimized.

Show comment
Hide comment
@cannikin

cannikin Mar 7, 2016

Thank you sir! I'm liking the feel of these new controller-integration hybrid tests so far.

cannikin commented Mar 7, 2016

Thank you sir! I'm liking the feel of these new controller-integration hybrid tests so far.

@nikolaokonesh

This comment has been minimized.

Show comment
Hide comment
@nikolaokonesh

nikolaokonesh Jun 15, 2016

@dhh Hi
how to test omniauth "should get new" do ? and "create"...
` helper_method :authenticate_user!

nikolaokonesh commented Jun 15, 2016

@dhh Hi
how to test omniauth "should get new" do ? and "create"...
` helper_method :authenticate_user!

@ream88

This comment has been minimized.

Show comment
Hide comment
@ream88

ream88 Jan 18, 2017

Contributor

If you still need this (I did 😕), this is the solution I came up for (which is just copied from ActionDispatch::Cookies):

def session_header(session)
  session = session.reverse_merge(session_id: SecureRandom.hex)

  key_generator = ActiveSupport::KeyGenerator.new(ENV.fetch('SECRET_KEY_BASE'), iterations: 1000)
  secret = key_generator.generate_key('encrypted cookie')
  sign_secret = key_generator.generate_key('signed encrypted cookie')
  encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, digest: 'SHA1', serializer: ActiveSupport::MessageEncryptor::NullSerializer)

  { 'Cookie' => "_your_app_session=#{encryptor.encrypt_and_sign(session.to_json)}" }
end

You can use it like so:

get '/needs/session', headers: session_header('some' => 'data')

This works with the current 5.0 versions, but can break any time for newer versions. Use at your own risk! The better way would be to follow @dhh's advise:

I would rework the test to test something visible in the UI instead.

Contributor

ream88 commented Jan 18, 2017

If you still need this (I did 😕), this is the solution I came up for (which is just copied from ActionDispatch::Cookies):

def session_header(session)
  session = session.reverse_merge(session_id: SecureRandom.hex)

  key_generator = ActiveSupport::KeyGenerator.new(ENV.fetch('SECRET_KEY_BASE'), iterations: 1000)
  secret = key_generator.generate_key('encrypted cookie')
  sign_secret = key_generator.generate_key('signed encrypted cookie')
  encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, digest: 'SHA1', serializer: ActiveSupport::MessageEncryptor::NullSerializer)

  { 'Cookie' => "_your_app_session=#{encryptor.encrypt_and_sign(session.to_json)}" }
end

You can use it like so:

get '/needs/session', headers: session_header('some' => 'data')

This works with the current 5.0 versions, but can break any time for newer versions. Use at your own risk! The better way would be to follow @dhh's advise:

I would rework the test to test something visible in the UI instead.

@evgenyfadeev

This comment has been minimized.

Show comment
Hide comment
@evgenyfadeev

evgenyfadeev Apr 23, 2017

When authentication depends on an external service, sometimes it might not be available for proper integration tests; for instance, it might be behind some firewall.

Therefore - one has to either stub the authentication service or create a fake test login controller. The second will need to assess the session and that would run contrary to the idea that session is not available for the purposes of running integration tests. Stubbing the service is also extra work.

Perhaps adding an official login method, available from the test cases and not giving full access to session would be a good compromise. Otherwise, we just have an extra stumbling block in the way of testing.

evgenyfadeev commented Apr 23, 2017

When authentication depends on an external service, sometimes it might not be available for proper integration tests; for instance, it might be behind some firewall.

Therefore - one has to either stub the authentication service or create a fake test login controller. The second will need to assess the session and that would run contrary to the idea that session is not available for the purposes of running integration tests. Stubbing the service is also extra work.

Perhaps adding an official login method, available from the test cases and not giving full access to session would be a good compromise. Otherwise, we just have an extra stumbling block in the way of testing.

cflipse added a commit to cflipse/loudoun_codes that referenced this issue May 20, 2017

Add a request level spec for admin testing
This operates more at the API level, but it appears that Rails5.1 has
taken away access to the session variable, so it becomes necessary to
interact with multiple different controller endpoints, instead of simply
preconfiguring a value in the session.

Apparently, DHH thinks this is an "implementation detail", and that it's
better to require hitting multiple endpoints for any single test (Or,
authenticate through header values).  The man does not care about test
efficency, or any of the myriad corner cases in a typical session-based
app, in the slightest.

rails/rails#23386

cflipse added a commit to cflipse/loudoun_codes that referenced this issue May 21, 2017

Add a request level spec for admin testing
This operates more at the API level, but it appears that Rails5.1 has
taken away access to the session variable, so it becomes necessary to
interact with multiple different controller endpoints, instead of simply
preconfiguring a value in the session.

Apparently, DHH thinks this is an "implementation detail", and that it's
better to require hitting multiple endpoints for any single test (Or,
authenticate through header values).  The man does not care about test
efficency, or any of the myriad corner cases in a typical session-based
app, in the slightest.

rails/rails#23386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment