Deprecate *_via_redirect integration test methods #18693

Merged
merged 1 commit into from Jan 28, 2015

Conversation

Projects
None yet
6 participants
@aditya-kapoor
Contributor

aditya-kapoor commented Jan 26, 2015

As asked by @dhh in #18333.
Closes #18333.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 26, 2015

Member

In the deprecation, we should just add "Please use follow_redirect! manually after the request call for the same behavior."

Member

dhh commented Jan 26, 2015

In the deprecation, we should just add "Please use follow_redirect! manually after the request call for the same behavior."

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 26, 2015

@@ -57,33 +57,63 @@ def test_get_via_redirect
@session.get_via_redirect(path, args, headers)
end
+ def test_get_via_redirect_is_deprecated

This comment has been minimized.

@egilburg

egilburg Jan 26, 2015

Contributor

Instead of adding new tests, you can wrap assert_deprecated around existing test assertions. I think this will also prevent the existing tests from spamming deprecation text in the test log.

@egilburg

egilburg Jan 26, 2015

Contributor

Instead of adding new tests, you can wrap assert_deprecated around existing test assertions. I think this will also prevent the existing tests from spamming deprecation text in the test log.

@aditya-kapoor

This comment has been minimized.

Show comment
Hide comment
@aditya-kapoor

aditya-kapoor Jan 27, 2015

Contributor

@ddh, @egilburg please review..

Contributor

aditya-kapoor commented Jan 27, 2015

@ddh, @egilburg please review..

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 27, 2015

Contributor

@aditya-kapoor looks OK although usually you'd try reducing assert_deprecated around as little lines as possible.

Contributor

egilburg commented Jan 27, 2015

@aditya-kapoor looks OK although usually you'd try reducing assert_deprecated around as little lines as possible.

@@ -98,30 +98,35 @@ def request_via_redirect(http_method, path, parameters = nil, headers_or_env = n
# Performs a GET request, following any subsequent redirect.
# See +request_via_redirect+ for more information.
def get_via_redirect(path, parameters = nil, headers_or_env = nil)
+ ActiveSupport::Deprecation.warn('Please use follow_redirect! manually after the request call for the same behavior.')

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 27, 2015

Member

The message should be:

get_via_redirect is deprecated and will be removed in the next version of Rails. Please use follow_redirect! manually after the request call for the same behavior.
@rafaelfranca

rafaelfranca Jan 27, 2015

Member

The message should be:

get_via_redirect is deprecated and will be removed in the next version of Rails. Please use follow_redirect! manually after the request call for the same behavior.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 27, 2015

Member

Also remove the whitespaces changes in the patch.

Member

rafaelfranca commented Jan 27, 2015

Also remove the whitespaces changes in the patch.

@aditya-kapoor

This comment has been minimized.

Show comment
Hide comment
Contributor

aditya-kapoor commented Jan 28, 2015

@rafaelfranca done...

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 28, 2015

Member

Merging

Member

rafaelfranca commented Jan 28, 2015

Merging

@rafaelfranca rafaelfranca merged commit 751f752 into rails:master Jan 28, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Jan 28, 2015

Merge pull request #18693 from aditya-kapoor/deprecate_via_redirect
Deprecate *_via_redirect integration test methods

@aditya-kapoor aditya-kapoor deleted the aditya-kapoor:deprecate_via_redirect branch Jan 29, 2015

@mhartl

This comment has been minimized.

Show comment
Hide comment
@mhartl

mhartl Jan 31, 2015

Contributor

I like to use defined?(post_via_redirect) to define an integration_test? boolean, which works because post_via_redirect isn't defined inside non-integration tests. Now that the *_via_redirect methods are deprecated, does anyone have a suggestion for a replacement?

Contributor

mhartl commented Jan 31, 2015

I like to use defined?(post_via_redirect) to define an integration_test? boolean, which works because post_via_redirect isn't defined inside non-integration tests. Now that the *_via_redirect methods are deprecated, does anyone have a suggestion for a replacement?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 31, 2015

Member

What are you using the integration_test? check for?

On Jan 30, 2015, at 7:29 PM, Michael Hartl notifications@github.com wrote:

I like to use defined?(post_via_redirect) to define an integration_test? boolean, which works because post_via_redirect isn't defined inside non-integration tests. Now that the *_via_redirect methods are deprecated, does anyone have a suggestion for a replacement?


Reply to this email directly or view it on GitHub #18693 (comment).

Member

dhh commented Jan 31, 2015

What are you using the integration_test? check for?

On Jan 30, 2015, at 7:29 PM, Michael Hartl notifications@github.com wrote:

I like to use defined?(post_via_redirect) to define an integration_test? boolean, which works because post_via_redirect isn't defined inside non-integration tests. Now that the *_via_redirect methods are deprecated, does anyone have a suggestion for a replacement?


Reply to this email directly or view it on GitHub #18693 (comment).

@mhartl

This comment has been minimized.

Show comment
Hide comment
@mhartl

mhartl Jan 31, 2015

Contributor

I define a log_in_as test helper that posts to the Sessions controller if called inside an integration test but manipulates the session object directly otherwise.

Contributor

mhartl commented Jan 31, 2015

I define a log_in_as test helper that posts to the Sessions controller if called inside an integration test but manipulates the session object directly otherwise.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 31, 2015

Member

Ah, I think maybe having separate helpers when they do separate things is a better deal than multiple paths inside a single method. But curious to see the implementation!

On Jan 30, 2015, at 8:13 PM, Michael Hartl notifications@github.com wrote:

I define a log_in_as test helper that posts to the Sessions controller if called inside an integration test but manipulates the session object directly otherwise.


Reply to this email directly or view it on GitHub #18693 (comment).

Member

dhh commented Jan 31, 2015

Ah, I think maybe having separate helpers when they do separate things is a better deal than multiple paths inside a single method. But curious to see the implementation!

On Jan 30, 2015, at 8:13 PM, Michael Hartl notifications@github.com wrote:

I define a log_in_as test helper that posts to the Sessions controller if called inside an integration test but manipulates the session object directly otherwise.


Reply to this email directly or view it on GitHub #18693 (comment).

@mhartl

This comment has been minimized.

Show comment
Hide comment
@mhartl

mhartl Jan 31, 2015

Contributor

The log_in_as method is an abstraction layer over the idea of logging in inside a test. If you don't switch on the test type, you need something like log_in_as_integration and log_in_as_non_integration which is rather verbose. The implementation (from the Rails Tutorial sample app's test helper) looks like this:

# Logs in a test user.
def log_in_as(user, options = {})
  password    = options[:password]    || 'password'
  remember_me = options[:remember_me] || '1'
  if integration_test?
    post login_path, session: { email:       user.email,
                                password:    password,
                                remember_me: remember_me }
  else
    session[:user_id] = user.id
  end
end

  private

    # Returns true inside an integration test.
    def integration_test?
      defined?(post_via_redirect)
    end

(Come to think of it, the definitions of password and remember_me should probably be moved into the if branch.) I don't particularly care about post_via_redirect per se, and indeed its use in the integration_test? method is a bit of a kludge, but I do want to have an automatic way of detecting whether the log_in_as method is being invoked inside an integration test. With the deprecation of *_via_redirect, it would be great to have an alternate way of doing that.

Contributor

mhartl commented Jan 31, 2015

The log_in_as method is an abstraction layer over the idea of logging in inside a test. If you don't switch on the test type, you need something like log_in_as_integration and log_in_as_non_integration which is rather verbose. The implementation (from the Rails Tutorial sample app's test helper) looks like this:

# Logs in a test user.
def log_in_as(user, options = {})
  password    = options[:password]    || 'password'
  remember_me = options[:remember_me] || '1'
  if integration_test?
    post login_path, session: { email:       user.email,
                                password:    password,
                                remember_me: remember_me }
  else
    session[:user_id] = user.id
  end
end

  private

    # Returns true inside an integration test.
    def integration_test?
      defined?(post_via_redirect)
    end

(Come to think of it, the definitions of password and remember_me should probably be moved into the if branch.) I don't particularly care about post_via_redirect per se, and indeed its use in the integration_test? method is a bit of a kludge, but I do want to have an automatic way of detecting whether the log_in_as method is being invoked inside an integration test. With the deprecation of *_via_redirect, it would be great to have an alternate way of doing that.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 31, 2015

Member

These two paths share almost nothing. Not even the signature. I would split them out. Much clearer, imo:

# Logs in a test user.
def log_in_as(user, password: 'password', remember_me: '1')
  post login_path, session: { email:       user.email,
                              password:    password,
                              remember_me: remember_me }
end

def log_in_as(user)
  session[:user_id] = user.id
end
Member

dhh commented Jan 31, 2015

These two paths share almost nothing. Not even the signature. I would split them out. Much clearer, imo:

# Logs in a test user.
def log_in_as(user, password: 'password', remember_me: '1')
  post login_path, session: { email:       user.email,
                              password:    password,
                              remember_me: remember_me }
end

def log_in_as(user)
  session[:user_id] = user.id
end
@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 31, 2015

Member

In fact, I would also rename them to follow the intent. The integration helper is legitimately log_in_as but the controller test is more accurately described as logged_in_as, since it sets the state AFTER someone has been logged in – it doesn't perform the login action.

Member

dhh commented Jan 31, 2015

In fact, I would also rename them to follow the intent. The integration helper is legitimately log_in_as but the controller test is more accurately described as logged_in_as, since it sets the state AFTER someone has been logged in – it doesn't perform the login action.

@mhartl

This comment has been minimized.

Show comment
Hide comment
@mhartl

mhartl Feb 1, 2015

Contributor

I appreciate the feedback and will consider incorporating it in future refinements of the tutorial. In the mean time, does anyone have any suggestions for writing an integration_test? method that will still work after the *_via_redirect methods have been removed?

Contributor

mhartl commented Feb 1, 2015

I appreciate the feedback and will consider incorporating it in future refinements of the tutorial. In the mean time, does anyone have any suggestions for writing an integration_test? method that will still work after the *_via_redirect methods have been removed?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Feb 1, 2015

Member

If you don't care about the implementation, you can just do is_a? ActiveSupport::IntegrationTest or whatever it is.

Member

dhh commented Feb 1, 2015

If you don't care about the implementation, you can just do is_a? ActiveSupport::IntegrationTest or whatever it is.

@georgeclaghorn

This comment has been minimized.

Show comment
Hide comment
@georgeclaghorn

georgeclaghorn Feb 1, 2015

Member

Consider separate implementations in the controller and integration test classes:

class ActionDispatch::IntegrationTest
  def log_in_as(user, password: 'password', remember_me: '1')
    post login_path, session: {
      email: user.email,
      password: password,
      remember_me: remember_me
    }
  end
end

class ActionController::TestCase
  def log_in_as(user)
    session[:user_id] = user.id
  end
end
Member

georgeclaghorn commented Feb 1, 2015

Consider separate implementations in the controller and integration test classes:

class ActionDispatch::IntegrationTest
  def log_in_as(user, password: 'password', remember_me: '1')
    post login_path, session: {
      email: user.email,
      password: password,
      remember_me: remember_me
    }
  end
end

class ActionController::TestCase
  def log_in_as(user)
    session[:user_id] = user.id
  end
end
@mhartl

This comment has been minimized.

Show comment
Hide comment
@mhartl

mhartl Feb 1, 2015

Contributor

@georgeclaghorn Cool, thanks for the suggestion.

Contributor

mhartl commented Feb 1, 2015

@georgeclaghorn Cool, thanks for the suggestion.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Apr 23, 2016

Deprecate `request_via_redirect` method.
- Followup of rails#18693.
- I think we missed deprecating `request_via_redirect` in that pull
  request.
- Originally requested by DHH here
  rails#18333.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Apr 24, 2016

Deprecate `request_via_redirect` method.
- Followup of rails#18693.
- I think we missed deprecating `request_via_redirect` in that pull
  request.
- Originally requested by DHH here
  rails#18333.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment