Permalink
Browse files

Refactor ActionController::TestCase cookies

Assigning cookies for test cases should now use cookies[], e.g:

  cookies[:email] = 'user@example.com'
  get :index
  assert_equal 'user@example.com', cookies[:email]

To clear the cookies, use clear, e.g:

  cookies.clear
  get :index
  assert_nil cookies[:email]

We now no longer write out HTTP_COOKIE and the cookie jar is
persistent between requests so if you need to manipulate the environment
for your test you need to do it before the cookie jar is created.
  • Loading branch information...
1 parent 64325a8 commit d4658d86fe7b037f2105e798a5717e22ca55d61d @pixeltrix pixeltrix committed Mar 29, 2011
View
@@ -1,5 +1,23 @@
*Rails 3.1.0 (unreleased)*
+* Refactor ActionController::TestCase cookies [Andrew White]
+
+ Assigning cookies for test cases should now use cookies[], e.g:
+
+ cookies[:email] = 'user@example.com'
+ get :index
+ assert_equal 'user@example.com', cookies[:email]
+
+ To clear the cookies, use clear, e.g:
+
+ cookies.clear
+ get :index
+ assert_nil cookies[:email]
+
+ We now no longer write out HTTP_COOKIE and the cookie jar is
+ persistent between requests so if you need to manipulate the environment
+ for your test you need to do it before the cookie jar is created.
+
* Added 'ActionView::Helpers::FormHelper.fields_for_with_index', similar to fields_for but allows to have access to the current iteration index [Jorge Bejar]
* Warn if we cannot verify CSRF token authenticity [José Valim]
@@ -175,17 +175,14 @@ 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/ }
@symbolized_path_params = nil
@method = @request_method = nil
@fullpath = @ip = @remote_ip = nil
@env['action_dispatch.request.query_parameters'] = {}
+ cookie_jar.reset!
end
end
@@ -301,18 +298,17 @@ def exists?
# For redirects within the same controller, you can even call follow_redirect and the redirect will be followed, triggering another
# action call which can then be asserted against.
#
- # == Manipulating the request collections
+ # == Manipulating session and cookie variables
#
- # The collections described above link to the response, so you can test if what the actions were expected to do happened. But
- # sometimes you also want to manipulate these collections in the incoming request. This is really only relevant for sessions
- # and cookies, though. For sessions, you just do:
+ # Sometimes you need to set up the session and cookie variables for a test.
+ # To do this just assign a value to the session or cookie collection:
#
- # @request.session[:key] = "value"
- # @request.cookies[:key] = "value"
+ # session[:key] = "value"
+ # cookies[:key] = "value"
#
- # To clear the cookies for a test just clear the request's cookies hash:
+ # To clear the cookies for a test just clear the cookie collection:
#
- # @request.cookies.clear
+ # cookies.clear
#
# == \Testing named routes
#
@@ -450,7 +446,6 @@ def process(action, parameters = nil, session = nil, flash = nil, http_method =
@controller.process_with_new_base_test(@request, @response)
@assigns = @controller.respond_to?(:view_assigns) ? @controller.view_assigns : {}
@request.session.delete('flash') if @request.session['flash'].blank?
- @request.cookies.merge!(@response.cookies)
@response
end
@@ -185,6 +185,11 @@ def delete(key, options = {})
value
end
+ # Removes all cookies on the client machine by calling <tt>delete</tt> for each cookie
+ def clear(options = {})
+ @cookies.each_key{ |k| delete(k, options) }
+ end
+
# Returns a jar that'll automatically set the assigned cookies to have an expiration date 20 years from now. Example:
#
# cookies.permanent[:prefers_open_id] = true
@@ -222,6 +227,11 @@ def write(headers)
@delete_cookies.each { |k, v| ::Rack::Utils.delete_cookie_header!(headers, k, v) }
end
+ def reset! #:nodoc:
+ @set_cookies.clear
+ @delete_cookies.clear
+ end
+
private
def write_cookie?(cookie)
@@ -22,7 +22,7 @@ def flash
end
def cookies
- @request.cookies.merge(@response.cookies).with_indifferent_access
+ @request.cookie_jar
end
def redirect_to_url
@@ -20,12 +20,6 @@ def initialize(env = {})
self.user_agent = 'Rails Testing'
end
- def env
- write_cookies!
- delete_nil_values!
- super
- end
-
def request_method=(method)
@env['REQUEST_METHOD'] = method.to_s.upcase
end
@@ -70,24 +64,5 @@ def accept=(mime_types)
@env.delete('action_dispatch.request.accepts')
@env['HTTP_ACCEPT'] = Array(mime_types).collect { |mime_type| mime_type.to_s }.join(",")
end
-
- def cookies
- @cookies ||= super
- end
-
- private
- def write_cookies!
- unless @cookies.blank?
- @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
end
end
@@ -593,13 +593,13 @@ def test_symbolized_path_params_reset_after_request
end
def test_should_have_knowledge_of_client_side_cookie_state_even_if_they_are_not_set
- @request.cookies['foo'] = 'bar'
+ cookies['foo'] = 'bar'
get :no_op
assert_equal 'bar', cookies['foo']
end
def test_should_detect_if_cookie_is_deleted
- @request.cookies['foo'] = 'bar'
+ cookies['foo'] = 'bar'
get :delete_cookie
assert_nil cookies['foo']
end
@@ -430,54 +430,48 @@ def test_cookies_hash_is_indifferent_access
def test_setting_request_cookies_is_indifferent_access
- @request.cookies.clear
- @request.cookies[:user_name] = "andrew"
+ cookies.clear
+ cookies[:user_name] = "andrew"
get :string_key_mock
- assert_equal "david", cookies[:user_name]
+ assert_equal "david", cookies['user_name']
- @request.cookies.clear
- @request.cookies['user_name'] = "andrew"
+ cookies.clear
+ cookies['user_name'] = "andrew"
get :symbol_key_mock
- assert_equal "david", cookies['user_name']
+ 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_cookie_header "user_name=david; path=/"
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
+ 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'
+ def test_can_set_http_cookie_header
+ @request.env['HTTP_COOKIE'] = "user_name=david"
get :noop
- assert_equal "user_ids=1%3B2", @request.env['HTTP_COOKIE']
- assert_equal "1;2", cookies[:user_ids]
+ assert_equal 'david', cookies['user_name']
+ assert_equal 'david', cookies[:user_name]
end
private
@@ -34,12 +34,15 @@ class TestRequestTest < ActiveSupport::TestCase
assert_equal({}, req.cookies)
assert_equal nil, req.env["HTTP_COOKIE"]
- req.cookies["user_name"] = "david"
- assert_equal({"user_name" => "david"}, req.cookies)
- assert_equal "user_name=david", req.env["HTTP_COOKIE"]
+ req.cookie_jar["user_name"] = "david"
+ assert_cookies({"user_name" => "david"}, req.cookie_jar)
- 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
+ req.cookie_jar["login"] = "XJ-122"
+ assert_cookies({"user_name" => "david", "login" => "XJ-122"}, req.cookie_jar)
end
+
+ private
+ def assert_cookies(expected, cookie_jar)
+ assert_equal(expected, cookie_jar.instance_variable_get("@cookies"))
+ end
end

11 comments on commit d4658d8

Owner

pixeltrix commented on d4658d8 Jun 4, 2011

@tenderlove cherry pick to 3-1-stable - confirm/deny?

Member

josevalim replied Jun 4, 2011

If we cherry pick it to 3-1-stable, won't it be break a bunch of existing test suites?

Owner

pixeltrix replied Jun 4, 2011

Possibly - it depends on how they're written. If they're tests like:

get :set_cookie
assert_equal 'value', @response.cookies['key']

then they'll still work as the response cookies are set. Also if developers have been consistent in how they access the cookies (e.g. always using request.cookies and string keys) then they should still work. What will definitely break is if they're using request.cookies[] to setup a cookie value for the test and using cookies[] in the controller code. This could be possibly made to work by reverse merging the request.cookies hash in recycle! so that we pickup any new values.

Contributor

wincent replied Jun 4, 2011

Doesn't seem an appropriate time to introduce a potentially breaking change into 3-1-stable, given that an RC is already out.

Member

sikachu replied Jun 4, 2011

This commit seems to break railties/test/rails_info_controller_test.rb no?

Owner

pixeltrix replied Jun 4, 2011

Yes - test_process.rb needs a require for cookies middleware, fix coming.

Owner

tenderlove replied Jun 4, 2011

I have to agree with @wincent. The rc has already been released, why would we introduce a potentially breaking change? What is the benefit? Can you make it backwards compatible?

Owner

pixeltrix replied Jun 4, 2011

You're both right - the tricky issue in making it backwards compatible is that unless HTTP_COOKIE is set in the environment hash then @request.cookies is not stored anywhere so the assignment @request.cookies['key'] = 'value' is lost. That's why the previous code was writing out HTTP_COOKIE so that they would be picked up in process. However giving it some thought I think can get backwards compatibility - the tricky decision is which has precedence:

@request.env['HTTP_COOKIE'] = 'name=david'
@request.cookies['name'] = 'david'
cookies['name'] = 'david'

What do you think? I think that cookies should win followed by @request.cookies and then HTTP_COOKIE.

Contributor

wincent replied Jun 4, 2011

I don't feel strongly about the backwards compatibility (ie. I wouldn't mind introducing a breaking change in the interests of keeping the code simpler).

I just think that such a change should be made after 3.1, and not during the release candidate phase.

Owner

pixeltrix replied Jun 4, 2011

That's why I asked the question - the change had been discussed for 3.1 but due to need to put food on the table for my kids I had been remiss in merging before the RC. Feedback heard loud and clear and I won't backport to 3-1-stable.

Contributor

dchelimsky replied Jun 7, 2011

Contrary to @wincent's position, I'd say 3.1 is actually a perfectly fine choice even though the rc is already out. It is still pre-release software, and if you're aiming at SemVer, the alternative is to wait until 3.2.

Please sign in to comment.