Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Functional test get method ignores session variables from setup when given a hash of new session variables #1529

Closed
avk opened this Issue · 19 comments

5 participants

@avk
avk commented

I believe this is a functional test bug:

class SomeControllerTest < ActionController::TestCase
  setup do
    @request.session["user_id"] = 1
  end

  test "some action" do
    get :some_action, { }, { "another_var" => 2 }
    # expected: @request.session == { "user_id" => 1, "another_var" => 2 }
    # actual:  @request.session == { "another_var" => 2 }
  end
end

After the get line, the session will only have another_var, not user_id. To get the latter, you have to do:
@request.session.merge({ "another_var" => 5 }) as the third argument to get. It does not seem to matter whether the session keys are strings or symbols, both fail without a merge.

Shouldn't merging with the current @request.session be done as part of get? Happy to start a patch if we agree this is a bug.

@pixeltrix
Owner

This appears to be the intention as the code explicitly creates a new session. You can easily merge it yourself - I'd recommend using update as the session hash is a Rack session hash and update is redefined to stringify keys, e.g:

get :some_action, { }, session.update(:another_var => 2)
@pixeltrix pixeltrix closed this
@avk
avk commented

Thanks for the quick response and the session.update tip. I know merging it myself isn't hard but isn't merging the common case? Any thoughts on why the session is treated differently from the parameters and the flash, both of which are merged in (around the code you linked to)?

@pixeltrix
Owner

Well if it merged by default how would you create a test with a clean session - you'd need something like this:

class SomeControllerTest < ActionController::TestCase
  setup do
    @request.session["user_id"] = 1
  end

  test "some action" do
    @request.session = ActionController::TestSession.new
    get :some_action, { }, { :another_var => 2 }
  end
end

It would also be a breaking change without any possibility of being backwards compatible unless you add a config option to enable the old behaviour.

@avk
avk commented

If merging the session is more common, I wouldn't mind the additional

@request.session = ActionController::TestSession.new

in the odd case I'd need to create a clean session but I do see your point about backwards compatibility.

@dmathieu
Collaborator

I do think merging the session is the more common behavior.

@pixeltrix : with merging session params, there isn't necessarily a need to reinitialize the session manually.
I'd reproduce your previous example with the following :

class SomeControllerTest < ActionController::TestCase
    context "logged in" do
        setup do
            @request.session["user_id"] = 1
        end

        test some action" do
            get :some_action, { }
        end
    end

    context "logged of" do
        test "some action" do
            get :some_action, { }, { :another_var => 2 }
        end
    end
end

This way, you have all your tests use the same predefined session depending of the context.
However as this can't be backward compatible and there'd be a lot of tests broken, I don't think it should come before 3.2.

@pixeltrix
Owner

If you're going to try and stick to Semantic Versioning then it shouldn't come before 4.0 :-)

@avk

I'm not a stickler for version numbers. Don't mind waiting til 3.2 or later. Should I contribute a patch with tests?

@avk

Any more thoughts on this? Go forward or ignore?

@avk
avk commented

Ignore it is.

@pixeltrix
Owner

@avk current focus is getting 3.1 out the door - this won't be patched before 3.2 so not a high priority right now. I'll assign it to me so it doesn't get lost and give you an answer once 3.1 final has shipped.

@pixeltrix pixeltrix was assigned
@pixeltrix pixeltrix reopened this
@avk
avk commented

Awesome, thanks. Just wanted to make sure it didn't get lost.

@rafaelfranca

@pixeltrix ping.

@steveklabnik
Collaborator

@pixeltrix was this eventually added to 3.2? @avk, are you still seeing this problem?

@pixeltrix
Owner

@steveklabnik looking at it now - if it's not in then I'll add it today

@steveklabnik
Collaborator

Awesome, thanks. :heart:

@pixeltrix
Owner

Just out of interest would it be preferable to reset to the initial session after a process with a session arg or leave it merged? e.g:

class SomeControllerTest < ActionController::TestCase
  setup do
    session[:user_id] = 1
  end

  test "some action" do
    get :some_action, { }, { :another_var => 2 }
    # session == { "user_id" => 1, "another_var" => 2 }

    get :some_action
    # A: session == { "user_id" => 1 }
    # B: session == { "user_id" => 1, "another_var" => 2 }
  end
end

Which would be preferable - A or B. I can see arguments for both.

@rafaelfranca

Form me B if it is in the same test as you showed. But A if it is in different tests.

@pixeltrix
Owner

The request instance is re-created in the test setup phase so that would always be the case. The B case is nice and easy so lets go with that :-)

@pixeltrix pixeltrix closed this issue from a commit
@pixeltrix pixeltrix Merge session arg with existing session instead of overwriting
This may break existing tests that are asserting the whole session contents
but should not break existing tests that are asserting individual keys - e.g:

class SomeControllerTest < ActionController::TestCase
  setup do
    session['user_id'] = 1
  end

  test "some test" do
    get :some_action, nil, { 'another_var' => 2 }

    # This assertion will now fail
    assert_equal({ 'another_var' => 2 }, session)

    # This assertion will still pass
    assert_equal 2, session['another_var]
  end
end

Fixes #1529.
5c18bdc
@pixeltrix pixeltrix closed this in 5c18bdc
@avk

Sorry to have missed out on the conversation earlier today but glad to see my suggestion made it in. The B case makes sense to me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.