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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not mutate AC::TestRequest::DEFAULT_OPTIONS #26442

Merged
merged 1 commit into from Sep 11, 2016

Conversation

Projects
None yet
5 participants
@kirs
Member

kirs commented Sep 10, 2016

Long story short: at Shopify, I've been hunting the reason why ActionDispatch::IntegrationTest-backed tests always fails when it's executed after any controller test.

In our app controllers, we modify request.session_options a lot to store metadata.
I found that the Rails 5 implementation of ActionController::TestCase is assigning session_options from TestSession::DEFAULT_OPTIONS, which originally comes from Rack: https://github.com/rack/rack/blob/master/lib/rack/session/abstract/id.rb#L191

In this case, every time you manipulate request.session_options, ActionController::TestSession::DEFAULT_OPTIONS gets mutated ( 馃槺 ). As a result, Rack::Session::Abstract::Persisted::DEFAULT_OPTIONS got mutated as well.
Writing to session_options from controller test made integration test inherit that value and everything broke.

I've also sent a PR to Rack to avoid the issue in future: rack/rack#1110
We should dup it to avoid the issue.

review @rafaelfranca

@rafaelfranca rafaelfranca self-assigned this Sep 10, 2016

@rafaelfranca

View changes

actionpack/test/controller/request/test_request_test.rb Outdated
@@ -6,6 +6,11 @@ def test_test_request_has_session_options_initialized
assert @request.session_options
end
def test_mutate_session_options

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 10, 2016

Member

test_mutate_session_options_does_not_affect_default_options

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Sep 10, 2016

Member

we can make it test_mutating_session_options_does_not_affect_default_options 馃槃

@kirs

This comment has been minimized.

Member

kirs commented Sep 10, 2016

Updated 馃憣

@kirs

This comment has been minimized.

Member

kirs commented Sep 10, 2016

There is a test failing (ApplicationTests::BinSetupTest#test_bin_setup_output) but it looks totally unrelated to my changes. Looks like some unexpected Bundler warnings started to print.

@kirs

This comment has been minimized.

Member

kirs commented Sep 10, 2016

I think that Travis upgraded the Bundler version recently (from 1.12.5 to 1.13.0) and it's the reason why the test started to fail.

@arthurnn

This comment has been minimized.

Member

arthurnn commented Sep 10, 2016

@kirs looks good to me. I hit retry on travis, lets see if it goes green. master is green as far as I can see, so it should pass here too.

@kirs

This comment has been minimized.

Member

kirs commented Sep 10, 2016

Retrying doesn't help. I think the problem has been fixed here: #26449

@arthurnn arthurnn merged commit a09135d into rails:master Sep 11, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate Code Climate didn't find any new or fixed issues.
Details

rafaelfranca added a commit that referenced this pull request Sep 12, 2016

Merge pull request #26442 from kirs/action-controller-session-options鈥
鈥-mutate1

Do not mutate AC::TestRequest::DEFAULT_OPTIONS
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Sep 12, 2016

Backported in 1701c8f

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