Permalink
Browse files

Improve testing of cookies in functional tests:

- cookies can be set using string or symbol keys
- cookies are preserved across calls to get, post, etc.
- cookie names and values are escaped
- cookies can be cleared using @request.cookies.clear

[#6272 state:resolved]
  • Loading branch information...
pixeltrix committed Mar 6, 2011
1 parent 2437c78 commit e2523ff68309f444cd052031c1fe8a3030d2865a
@@ -171,6 +171,10 @@ def assign_parameters(routes, controller_path, action, parameters = {})
end
def recycle!
+ write_cookies!
+ @env.delete('HTTP_COOKIE') if @cookies.blank?
+ @env.delete('action_dispatch.cookies')
+ @cookies = nil
@formats = nil
@env.delete_if { |k, v| k =~ /^(action_dispatch|rack)\.request/ }
@env.delete_if { |k, v| k =~ /^action_dispatch\.rescue/ }
@@ -297,7 +301,11 @@ def exists?; true; end
# and cookies, though. For sessions, you just do:
#
# @request.session[:key] = "value"
- # @request.cookies["key"] = "value"
+ # @request.cookies[:key] = "value"
+ #
+ # To clear the cookies for a test just clear the request's cookies hash:
+ #
+ # @request.cookies.clear
#
# == Testing named routes
#
@@ -411,6 +419,7 @@ def process(action, parameters = nil, session = nil, flash = nil, http_method =
Base.class_eval { include Testing }
@controller.process_with_new_base_test(@request, @response)
@request.session.delete('flash') if @request.session['flash'].blank?
+ @request.cookies.merge!(@response.cookies)
@response
end
@@ -22,7 +22,7 @@ def flash
end
def cookies
- @request.cookies.merge(@response.cookies)
+ @request.cookies.merge(@response.cookies).with_indifferent_access
end
def redirect_to_url
@@ -1,5 +1,6 @@
require 'active_support/core_ext/object/blank'
require 'active_support/core_ext/hash/reverse_merge'
+require 'rack/utils'
module ActionDispatch
class TestRequest < Request
@@ -76,10 +77,14 @@ def cookies
private
def write_cookies!
unless @cookies.blank?
- @env['HTTP_COOKIE'] = @cookies.map { |name, value| "#{name}=#{value};" }.join(' ')
+ @env['HTTP_COOKIE'] = @cookies.map { |name, value| escape_cookie(name, value) }.join('; ')
end
end
+ def escape_cookie(name, value)
+ "#{Rack::Utils.escape(name)}=#{Rack::Utils.escape(value)}"
+ end
+
def delete_nil_values!
@env.delete_if { |k, v| v.nil? }
end
@@ -94,6 +94,30 @@ def delete_cookie_with_domain
cookies.delete(:user_name, :domain => :all)
head :ok
end
+
+ def symbol_key
+ cookies[:user_name] = "david"
+ head :ok
+ end
+
+ def string_key
+ cookies['user_name'] = "david"
+ head :ok
+ end
+
+ def symbol_key_mock
+ cookies[:user_name] = "david" if cookies[:user_name] == "andrew"
+ head :ok
+ end
+
+ def string_key_mock
+ cookies['user_name'] = "david" if cookies['user_name'] == "andrew"
+ head :ok
+ end
+
+ def noop
+ head :ok
+ end
end
tests TestController
@@ -291,6 +315,65 @@ def test_deleting_cookie_with_all_domain_option
assert_cookie_header "user_name=; domain=.nextangle.com; path=/; expires=Thu, 01-Jan-1970 00:00:00 GMT"
end
+ def test_cookies_hash_is_indifferent_access
+ [:symbol_key, :string_key].each do |cookie_key|
+ get cookie_key
+ assert_equal "david", cookies[:user_name]
+ assert_equal "david", cookies['user_name']
+ end
+ end
+
+ def test_setting_request_cookies_is_indifferent_access
+ @request.cookies.clear
+ @request.cookies[:user_name] = "andrew"
+ get :string_key_mock
+ assert_equal "david", cookies[:user_name]
+
+ @request.cookies.clear
+ @request.cookies['user_name'] = "andrew"
+ get :symbol_key_mock
+ assert_equal "david", cookies['user_name']
+ end
+
+ def test_cookies_retained_across_requests
+ get :symbol_key
+ assert_equal "user_name=david; path=/", @response.headers["Set-Cookie"]
+ assert_equal "david", cookies[:user_name]
+
+ get :noop
+ assert_nil @response.headers["Set-Cookie"]
+ assert_equal "user_name=david", @request.env['HTTP_COOKIE']
+ assert_equal "david", cookies[:user_name]
+
+ get :noop
+ assert_nil @response.headers["Set-Cookie"]
+ assert_equal "user_name=david", @request.env['HTTP_COOKIE']
+ assert_equal "david", cookies[:user_name]
+ end
+
+ def test_cookies_can_be_cleared
+ get :symbol_key
+ assert_equal "user_name=david; path=/", @response.headers["Set-Cookie"]
+ assert_equal "david", cookies[:user_name]
+
+ @request.cookies.clear
+ get :noop
+ assert_nil @response.headers["Set-Cookie"]
+ assert_nil @request.env['HTTP_COOKIE']
+ assert_nil cookies[:user_name]
+
+ get :symbol_key
+ assert_equal "user_name=david; path=/", @response.headers["Set-Cookie"]
+ assert_equal "david", cookies[:user_name]
+ end
+
+ def test_cookies_are_escaped
+ @request.cookies[:user_ids] = '1;2'
+ get :noop
+ assert_equal "user_ids=1%3B2", @request.env['HTTP_COOKIE']
+ assert_equal "1;2", cookies[:user_ids]
+ end
+
private
def assert_cookie_header(expected)
header = @response.headers["Set-Cookie"]
@@ -36,10 +36,10 @@ class TestRequestTest < ActiveSupport::TestCase
req.cookies["user_name"] = "david"
assert_equal({"user_name" => "david"}, req.cookies)
- assert_equal "user_name=david;", req.env["HTTP_COOKIE"]
+ assert_equal "user_name=david", req.env["HTTP_COOKIE"]
req.cookies["login"] = "XJ-122"
assert_equal({"user_name" => "david", "login" => "XJ-122"}, req.cookies)
- assert_equal %w(login=XJ-122 user_name=david), req.env["HTTP_COOKIE"].split(/; ?/).sort
+ assert_equal %w(login=XJ-122 user_name=david), req.env["HTTP_COOKIE"].split(/; /).sort
end
end

4 comments on commit e2523ff

@raggi

This comment has been minimized.

Show comment
Hide comment
@raggi

raggi Mar 29, 2011

Contributor

Is this reliably backwards compatible? If not, this shouldn't go in a patchlevel release, it may break all user sessions.

Contributor

raggi replied Mar 29, 2011

Is this reliably backwards compatible? If not, this shouldn't go in a patchlevel release, it may break all user sessions.

@NZKoz

This comment has been minimized.

Show comment
Hide comment
@NZKoz

NZKoz Mar 29, 2011

Member

it only changes the test process stuff so it's not likely to break a production app at least.

Is it causing test failures for you guys?

Member

NZKoz replied Mar 29, 2011

it only changes the test process stuff so it's not likely to break a production app at least.

Is it causing test failures for you guys?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Mar 29, 2011

Member

It should make it more like a running the app for real since it's using the HTTP_COOKIE header rather than a cached hash between calls to process.

Member

pixeltrix replied Mar 29, 2011

It should make it more like a running the app for real since it's using the HTTP_COOKIE header rather than a cached hash between calls to process.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Mar 29, 2011

Contributor

I am also -1 on applying this on a patch level release. It may not break an application, but it may break tests on updating, or worse, tools that are built on top of Rails Testing structure (for instance Rspec). Patch level == bug fixes only.

Contributor

josevalim replied Mar 29, 2011

I am also -1 on applying this on a patch level release. It may not break an application, but it may break tests on updating, or worse, tools that are built on top of Rails Testing structure (for instance Rspec). Patch level == bug fixes only.

Please sign in to comment.