Sessions in test environment
Roland Swingler
I spent quite a while trying to debug what I thought might have been a
Cucumber problem with a Cucumber/Webrat/Rack-test stack and sinatra -
but turned out to be a Sinatra issue. Line 887 of sinatra/base turns
off sessions in the test environment:
builder.user Rack::Session::Cookie if sessions? and !test?
I'm annoyed with myself because I actually knew this a couple of
months ago, but completely forgot about it. Why are sessions turned
off in the test environment? I.e. is this a bug, or is it intentional
for some reason? You can get around it by setting some other
environment on your app ("integration" for example), and tests (my
tests at any rate) seem to work fine.
It might be worth adding a line in the options documentation
for :session stating this will happen if it is intentional - otherwise
it is pretty confusing.
Cheers,
Roland
Alex Chaffee
learning Sinatra. I vote for not turning off sessions in test.
Otherwise how do you test sessions? Or more precisely, how do you test
application features that store things in sessions?
---
Alex Chaffee - al...@stinky.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Damian Janowski
Swingler<roland....@gmail.com> wrote:
> I spent quite a while trying to debug what I thought might have been a
> Cucumber problem with a Cucumber/Webrat/Rack-test stack and sinatra -
> but turned out to be a Sinatra issue. Line 887 of sinatra/base turns
> off sessions in the test environment:
>
> builder.user Rack::Session::Cookie if sessions? and !test?
>
> I'm annoyed with myself because I actually knew this a couple of
> months ago, but completely forgot about it. Why are sessions turned
> off in the test environment? I.e. is this a bug, or is it intentional
> for some reason? You can get around it by setting some other
> environment on your app ("integration" for example), and tests (my
> tests at any rate) seem to work fine.
It's a known issue:
https://webrat.lighthouseapp.com/projects/10503/tickets/190-sinatra-sessions-do-not-persist
The reason is that Sinatra is assuming unit testing of routes (rather
than integration testing) where you can do:
post "/logout", {}, :session => {:user => 1}
I proposed a patch [1] [2] that keeps backwards compatibility but it
didn't get applied (I don't remember why).
Ryan, can we bring the discussion here?
[1] http://github.com/djanowski/sinatra/commit/14f6c00f19b86757b7e6b5583f9a1e61113318bf
[2] http://github.com/djanowski/sinatra/commit/629e87ee67a025984b8a864eba5273e387abfd55
Ryan Tomayko
>
> On Tue, Jun 16, 2009 at 5:53 PM, Roland
> Swingler<roland....@gmail.com> wrote:
>> I spent quite a while trying to debug what I thought might have
>> been a
>> Cucumber problem with a Cucumber/Webrat/Rack-test stack and sinatra -
>> but turned out to be a Sinatra issue. Line 887 of sinatra/base turns
>> off sessions in the test environment:
>>
>> builder.user Rack::Session::Cookie if sessions? and !test?
>>
>> I'm annoyed with myself because I actually knew this a couple of
>> months ago, but completely forgot about it. Why are sessions turned
>> off in the test environment? I.e. is this a bug, or is it intentional
>> for some reason? You can get around it by setting some other
>> environment on your app ("integration" for example), and tests (my
>> tests at any rate) seem to work fine.
>
> It's a known issue:
> https://webrat.lighthouseapp.com/projects/10503/tickets/190-sinatra-sessions-do-not-persist
>
> The reason is that Sinatra is assuming unit testing of routes (rather
> than integration testing) where you can do:
>
> post "/logout", {}, :session => {:user => 1}
>
> I proposed a patch [1] [2] that keeps backwards compatibility but it
> didn't get applied (I don't remember why).
>
> Ryan, can we bring the discussion here?
>
existing tests may be dependent on the sessions being disabled.
Ryan
Damian Janowski
> Please do. I'm familiar with the patch but afraid to apply it because
> existing tests may be dependent on the sessions being disabled.
Can we think of an example of a test relying on sessions being
disabled? Besides, I think the current implementation is a little
weird: sessions are disabled but you're able to pass a :session to
your requests regardless.
The only breakage that I can see is when you stub a method on an
object, pass it with :session and expect it to be the actual instance
received by the application. The patch is backwards-compatible if
you're not doing this (it serializes your :session so the Rack
middleware can load the cookie properly).
It seems to me that many people are complaining about this odd behavior.
Can anyone please chime in if they see a scenario where this patch
would break a test?
Thomas Sullivan
That said, I agree - it'd be rare and not really anything to worry about, imho.
-Tom
Tanner Burson
I also like that once you KNOW that this is the behavior, it's hard to be surprised by it again. It's simple, and consistent, and that's the sinatra way ;)
--Tanner
===Tanner Burson===
tanner...@gmail.com
http://www.tannerburson.com
Thomas Sullivan
Damian Janowski
> I'll play devils advocate a bit here. I actually LIKE the current
> behavior. Being forced to pass in the current session state makes me think
> a lot more about what I'm storing and why. It also lets me easily test
> REALLY broken scenarios to make sure it's being handled properly.
I'm not sure I follow. We're not talking about deprecating the ability
to pass a :session key to your requests. You'll still be able to do
that, of course.
Tanner Burson
To me there's two different (and equally valid) ways to handle web tests. The way sinatra does currently, each request in isolation, requiring the test itself to handle any shared state. And a more browser centric view, where the test emulates a browser session only interacting with the application via HTTP verbs. But I really don't see much use in mixing the two types.
I would completely support an optional plugin enabling sessions for tests, or even configuration option to switch between the two methods. But I think trying to mix the two together will only lead to more confusion.
--Tanner
Damian Janowski
> I fail to see the real value in that. If you're going to act as a browser
> and store session state, then you shouldn't be mucking about with the
> session directly. What's the testing scenario for the combination of
> tracking session state, but still being able to manipulate it externally?
>
> To me there's two different (and equally valid) ways to handle web tests.
> The way sinatra does currently, each request in isolation, requiring the
> test itself to handle any shared state. And a more browser centric view,
> where the test emulates a browser session only interacting with the
> application via HTTP verbs. But I really don't see much use in mixing the
> two types.
I agree that mixing them in the same test doesn't make a lot of sense.
But I do have integration tests and unit tests – the former rely on
the session working normally, and the latter can provide a :session
key if needed for specific scenarios (like you said, when you're doing
lower-level tests like making sure things blow up on certain
conditions, etc.).
> I would completely support an optional plugin enabling sessions for tests,
> or even configuration option to switch between the two methods. But I think
> trying to mix the two together will only lead to more confusion.
The proposed patch lets you use whichever method you prefer (or use
both for different purposes) without any additional plugins or
configuration. You can use Webrat for your integration tests and still
do unit testing and pass specific values in the :session key.
Tanner Burson
--Tanner
Alex Chaffee
"login" action that adds a user id to the session, a "logout" action
that removes it, and a "home" action that displays the login form if
the user is not logged in, and displays "hi, username!" if the user is
logged in.
The claim is that I can unit test these methods individually by
priming the session with ":session => {:user_id => 1}" or ":session =>
{:user_id => nil}". But when I unit test "login" or "logout" I need to
test the session on the way *out*. Does the current system allow this?
I'll whip up a quick test after I send this email but my memory is
that it does not.
Also, the Rack::Test framework -- which is the official recommended
way to test Sinatra apps -- does preserve state including cookies and
therefore sessions (as well as sessions (*)). I don't think it's a
stretch to say that in Rack::Test all tests are integration tests by
default. So in disabling sessions for test Sinatra is breaking the
semantics of its own recommended framework.
Given (or rather, assuming) that if I want to do a "pure" unit test I
can do so by sending an empty hash for the session data, I vote for
removing this misfeature.
- A
(*) Rack::Test has two distinct meanings for the term "session" -- one
is a cookie session, and one is Rack::Test's own current connection
which may span multiple cookie sessions (or none at all). Since both
meanings appear in the same codebase it's potentially confusing. Ryan,
may I please renew my suggestion to rename the latter
"Rack::Test::Conversation"?
---
Alex Chaffee - al...@stinky.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Alex Chaffee
> is a cookie session, and one is Rack::Test's own current connection
> which may span multiple cookie sessions (or none at all). Since both
> meanings appear in the same codebase it's potentially confusing. Ryan,
> may I please renew my suggestion to rename the latter
> "Rack::Test::Conversation"?
Sorry, I meant "Bryan" not "Ryan" :[
Damian Janowski
> What's the advantage of patching sinatra's test code, as opposed to just
> setting the environment to, say :integration, in your integration tests?
What some of us are saying is that we want to *fix* the code, not
patch it in some weird way. Both Ryan and Blake agreed that that extra
check doesn't make sense. What we're trying to discuss here is whether
this fix is completely backwards-compatible or not.
Alex Chaffee
> priming the session with ":session => {:user_id => 1}" or ":session =>
> {:user_id => nil}". But when I unit test "login" or "logout" I need to
> test the session on the way *out*. Does the current system allow this?
> I'll whip up a quick test after I send this email but my memory is
> that it does not.
In fact, it mostly works. My main app has some strangeness whereby
sessions only work in test if I enable Rack::Session::Pool but my
little test app is showing none of that.
However, I am having trouble getting the ":session =>" syntax Damian
mentioned to work from my test. Damian, can you check my code at
http://gist.github.com/131374
and see if I'm doing something weird?
> If you're going to act as a browser and store session state, then you
> shouldn't be mucking about with the session directly. What's the testing
> scenario for the combination of tracking session state, but still being able
> to manipulate it externally?
Ah, that's a good question. So should/does Rack::Test support
*cookies* or does it support *sessions*? And what should/does Sinatra
do with each before and after a test?
- A
Roland Swingler
but if you like being explicit then you could always manually disable
sessions in your test_helper (or equivalent) by just setting them to
false:
My::Sinatra::App.set :sessions, false
> I also like that once you KNOW that this is the behavior, it's hard to be surprised by it again.
this back in March, but had forgotten about it by June.
> What's the advantage of patching sinatra's test code, as opposed to just setting the environment to, say :integration, in your integration tests?
other test tools) expect you or default you to be in the test
environment, so it involves more work to use a different one.
My real point is that I found it highly unexpected - so even if the
consensus is that disabling sessions is the way to go, it would good
if it was prominently highlighted in the test/options documentation.
Cheers,
Roland
Alex Chaffee
http://gist.github.com/131374 now shows a couple of things:
1) It only works at all if ENV['RACK_ENV'] = 'test' is the first line.
Arguably the environment should only matter if you're connecting to a
database... Oddly, in dev mode the only failure is
test_logout_with_session_passed_explicitly; the ones where the test is
truly mimicking a browser work fine.
2) In test mode, there are two failures, both integration tests. I
suppose this is because in test mode Sinatra is not setting
rack.session on the response, which is what this thread is about.
- A
---
Alex Chaffee - al...@stinky.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Michel Martens
On Jun 17, 11:58 am, Tanner Burson <tanner.bur...@gmail.com> wrote:
> I'll play devils advocate a bit here. I actually LIKE the current
> behavior. Being forced to pass in the current session state makes me think
> a lot more about what I'm storing and why. It also lets me easily test
> REALLY broken scenarios to make sure it's being handled properly.
>
> I also like that once you KNOW that this is the behavior, it's hard to be
> surprised by it again. It's simple, and consistent, and that's the sinatra
> way ;)
>
understand how I spent so much time testing each request in isolation.
If you like the current behavior, you will keep it as the patch is
backwards compatible.
Testing interactively with Webrat not only allows you to test really
broken scenarios, but lets you discover new ones.
Knowing that Sinatra doesn't support cookies in test mode doesn't tell
me anything about being simple, consistent or the Sinatra way: it just
looks like an arbitrary decision, a way to solve the problem of
providing a session hash from the tests. What at some point may have
been the only solution to a problem, turned out to be not good enough.
--
Michel
Michel Martens
> What's the advantage of patching sinatra's test code, as opposed to just
> setting the environment to, say :integration, in your integration tests?
>
What's the point of adding a new environment instead of fixing Sinatra?
I don't really understand what is it that you fear.
--
Michel
Ryan Tomayko
Janowski<damian....@gmail.com> wrote:
> On Tue, Jun 16, 2009 at 9:39 PM, Ryan Tomayko<rtom...@gmail.com> wrote:
>> Please do. I'm familiar with the patch but afraid to apply it because
>> existing tests may be dependent on the sessions being disabled.
>
> Can we think of an example of a test relying on sessions being
> disabled? Besides, I think the current implementation is a little
> weird: sessions are disabled but you're able to pass a :session to
> your requests regardless.
Yes. What's disabled is the conversion from an encoded Cookie header
value to a Hash on the request. Most tests in the wild today simply
pass the session hash directly in "rack.session" -- this avoids the
encode/decode step. You're not typically trying to test session
*encoding* in your app, you're trying to test that the deserialized
session is accessed and modified properly. If the :sessions option is
enabled, the hash passed in "rack.session" is replaced with an empty
hash unless the session data is encoded and passed in a Cookie header.
Also, how do you propose testing sessions when something other than
Rack::Session::Cookie is used? The current behavior just removes an
implementation detail that's typically irrelevant to the target of the
tests.
> The only breakage that I can see is when you stub a method on an
> object, pass it with :session and expect it to be the actual instance
> received by the application. The patch is backwards-compatible if
> you're not doing this (it serializes your :session so the Rack
> middleware can load the cookie properly).
I must have misread 14f6c00 when I was looking at this before the
0.9.2 release because I don't remember seeing the encode step. I think
mapping the :session option to HTTP_COOKIE was throwing me off:
http://github.com/djanowski/sinatra/commit/629e87ee67a025984b8a864eba5273e387abfd55#L0R79
At any rate, this does look to be backwards compatible. I'll cherry
pick it onto the 0.9.x branch and see if we get any spec failures.
I'm not sure how many more 0.9.x releases we'll be doing after 0.9.3
(which is already packaged and tagged) though. All of "sinatra/test"
has been removed on master, so the only relevant issue there is
whether sessions should be disabled by default. Does anyone know if
Rack::Test expects the app to perform cookie decode? If so, we should
adjust the default.
> It seems to me that many people are complaining about this odd behavior.
I think the confusion comes from interpreting "sessions are disabled"
as "you can't use the rack.session env var or access sessions in your
app" instead of "don't add encoding/decoding of cookie based sessions
using Rack's built in middleware." But, yeah, this does seem to be
something that trips people up.
Thanks,
Ryan
Alex Chaffee
> *encoding* in your app, you're trying to test that the deserialized
> session is accessed and modified properly.
characterization of "typical" is correct. Could you take a look at
http://gist.github.com/131374 and let me know if those failing
(integration-style) tests fall under your definition of atypical?
Cause if you think they're atypical, I disagree -- they're pretty much
mainline Rack::Test usage. And if you think they are typical, then
they should be passing, but they're failing. So either way it should
give you food for thought.
---
Alex Chaffee - al...@stinky.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com
Simon Rozet
>
> > You're not typically trying to test session
> > *encoding* in your app, you're trying to test that the deserialized
> > session is accessed and modified properly.
>
> I'm confused by this distinction, and I'm not sure your
> characterization of "typical" is correct. Could you take a look at
> http://gist.github.com/131374 and let me know if those failing
> (integration-style) tests fall under your definition of atypical?
> Cause if you think they're atypical, I disagree -- they're pretty much
> mainline Rack::Test usage. And if you think they are typical, then
> they should be passing, but they're failing. So either way it should
> give you food for thought.
It seems there is a bug (unconfirmed) in Rack::Test that prevents one to
use it with Rack::Session::Cookie. I'll be looking into it shortly.
Please chill down, btw.
Thanks,
--
Simon Rozet <si...@rozet.name> http://atonie.org
Roland Swingler
As this seems to be "by design" - even if we're not sure whether it is
correct or not, I've added a documentation patch to the lighthouse
issue:
https://webrat.lighthouseapp.com/projects/10503/tickets/190-sinatra-sessions-do-not-persist
Hopefully this will make it clearer for those reading the test
documentation what happens at the moment.
HTH,
Roland
Damian Janowski
> Cucumber problem with a Cucumber/Webrat/Rack-test stack and sinatra -
> but turned out to be a Sinatra issue. Line 887 of sinatra/base turns
> off sessions in the test environment:
>
> builder.user Rack::Session::Cookie if sessions? and !test?
For the record, if you want to use Webrat, just don't use #enable.
Call #use directly:
use Rack::Session:Cookie
If you are using Rack::Test and still want to pass :session in your
unit tests: http://gist.github.com/134800