wardencommunity / warden

General Rack Authentication Framework
MIT License
2.48k stars 204 forks source link

Calling `login_as` twice from the same test "leaks" to another test #167

Open mejibyte opened 6 years ago

mejibyte commented 6 years ago

I have an RSpec test in a Rails app that calls login_as and looks something like this:

require 'rails_helper'

RSpec.describe "weird behavior", type: :request do
  let!(:user) { FactoryBot.create(:user) }

  it "test a" do
    login_as user
    get "/"
    puts session.to_hash
    expect(session["warden.user.user.key"]).to_not be_blank
  end

  it "test b" do
    get "/"
    puts session.to_hash
    expect(session["warden.user.user.key"]).to be_blank
  end
end

It passes:

$ bundle exec rspec spec/requests/settings/password_spec.rb --order defined

weird behavior
{"session_id"=>"06f305ce65e80a53f9729add786fc2d2", "warden.user.user.key"=>[[535], "$2a$04$cGbUk2KvDf/mT4tv4psQW."]}
  test a
{}
  test b

Finished in 0.58912 seconds (files took 3.06 seconds to load)
2 examples, 0 failures

Accidentally, I added a second login_as in test A (and didn't notice -- in real life my test is more complicated so it was easy to miss):

require 'rails_helper'

RSpec.describe "weird behavior", type: :request do
  let!(:user) { FactoryBot.create(:user) }

  it "test a" do
    login_as user
    get "/"
    login_as user  # <- problematic call!
    puts session.to_hash
    expect(session["warden.user.user.key"]).to_not be_blank
  end

  it "test b" do
    get "/"
    puts session.to_hash
    expect(session["warden.user.user.key"]).to be_blank
  end
end

This resulted in flaky tests that would fail or pass depending on the order in which RSpec chose to run them (it works if B runs before A).

After hours of debugging, I realized the problem is that the second login_as call in test A is "leaking" to test B. Looking at the output, you can see that the Warden user key in the session printed from test B is the same as the one printed from test A!

$ bundle exec rspec spec/requests/settings/password_spec.rb --order defined

weird behavior
{"session_id"=>"3087df2477685b564db5f397dc1f6778", "warden.user.user.key"=>[[537], "$2a$04$zNWHx2qhfFzuO2c26MHpqO"]}
  test a
{"session_id"=>"21eb074aa142c542dd2fc04dc7ccaefd", "warden.user.user.key"=>[[537], "$2a$04$zNWHx2qhfFzuO2c26MHpqO"]}
  test b (FAILED - 1)

Failures:

  1) weird behavior test b
     Failure/Error: expect(session["warden.user.user.key"]).to be_blank
       expected `[[537], "$2a$04$zNWHx2qhfFzuO2c26MHpqO"].blank?` to return true, got false
     # ./spec/requests/settings/password_spec.rb:17:in `block (2 levels) in <top (required)>'

Finished in 0.62204 seconds (files took 3.11 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/requests/settings/password_spec.rb:14 # weird behavior test b

This is very surprising and breaks test isolation in an unexpected way. Can we make the "queue" of pending login_ascalls automatically disappear at the end of each spec? (Not sure if this is even feasible.)

jsmestad commented 6 years ago

Glad you tracked down the issue. This sounds like something RSpec would handle than the test helpers themselves. The test helpers are test framework agnostic and thus have no concept when a test starts, ends, or moves onto the next one.

What I'd suggest adding is a shared context in RSpec to cleanup authenticated requests:

RSpec.shared_context "authenticated", authenticated: true do
  after(:each) { Warden.test_reset! }
end

RSpec.configure do |config|
  config.include_context "authenticated", authenticated: true
end

Then on any test that needs authentication you can add it 'logs someone in', authenticated: true do ... end.

If most of your tests require authentication, you can follow the devise recommendation and setup a global after(:each) hook:

RSpec.configure do |config|
  config.after(:each) { Warden.test_reset! }
end

Hope this helps