Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This information is from the code coverage report (see http://coverage.cwgordon.com/coverage).
We need to test:
1) session_save_session(FALSE)
2) sess_count().
Comment | File | Size | Author |
---|---|---|---|
#27 | session_test.patch | 8.53 KB | chx |
#25 | session_test.patch | 0 bytes | chx |
#20 | session.test.patch | 8.85 KB | jhedstrom |
#19 | session.test.patch | 8.56 KB | jhedstrom |
#18 | session.test.patch | 8.56 KB | jhedstrom |
Comments
Comment #1
randall.vg CreditAttribution: randall.vg commentedI'll make a test for this within a few days.
Comment #2
jhedstromThe attached patch adds tests for sess_count() and session_save_session().
Comment #3
chx CreditAttribution: chx commentedI do not know what to say. These tests are good but they do nto really eat into the most important issue: do sessions persist data? And not when session_save_session is FALSE. This , however, likely needs the hidden module patch which is STILL not in.
Comment #4
jhedstromYeah, the tests I wrote are super low level. I'll try and get some data-persistence tests written soon.
Comment #5
boombatower CreditAttribution: boombatower commented#268063: Move includes/tests to simpletest/tests & provide hidden .info propery
Comment #6
jhedstromchx,
I've done some research on implementing actual tests for session data, and I don't really see a way to do it with the methods provided by drupal_web_test_case. It seems that ideally, the drupalLogin() method would create a session, but it doesn't seem to from what I can tell. I could easily write session tests that, using the php methods of switching session ids and saving sessions, would test for data retention, but this would be outside of the normal session flow, so I don't know if that would be of any value.
Comment #7
boombatower CreditAttribution: boombatower commentedwhat about:
-chx (http://drupal.org/node/268063#comment-955082)
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commentedTo create a new session, you will have to first empty the cookie jar of the internal browser. Something like this should do the trick:
Comment #9
jhedstromCool. Thanks for the feedback. Given that, does this sound like a reasonable way to proceed?
1. create a session_test.module that implements these 3 menu callbacks:
* session-test/set/{value}
* session-test/get - prints the value of the session variable set above
* session-test/no-set/{value} - implements session_save_session(FALSE) so the passed value shouldn't get saved
2. Test authenticated and anonymous users via those callbacks
Comment #10
jhedstromI've got the callbacks working and properly setting/not setting/returning session data (the idea of test modules are awesome btw). However, I still can't seem to get more than 1 session working, so the counting of sessions isn't being properly tested.
No matter how many times I use:
I still only get a single session in the sessions table (the stored data changes, but the session id doesn't). Any pointers from curl experts much appreciated.
Comment #11
nickl CreditAttribution: nickl commentedsubscribe
Comment #12
jhedstromI've got the session counting working. I had to implement the following every time I wanted to change sessions:
The attached patch fails 1 test, which is checking for data-persistence after logging out and then back in. It's my understanding that session IDs are reset after this happens, but I want confirmation on that before I change the test to reflect that behavior.
Comment #13
jhedstromI answered my own question regarding session handling at logout (they are destroyed using session_destroy()). The tests have been reworked to take this into account, and they are all passing now.
Comment #14
boombatower CreditAttribution: boombatower commentedTook a look at the code, looks good. I made a few minor style changes:
These are minor changes, but otherwise the test passes and looks good.
Comment #15
chx CreditAttribution: chx commentedSoooooooooooo very nice! +1000000000000 yesyesyesyesyesyesyesyesyesyesyesyes
Comment #16
Dries CreditAttribution: Dries commentedThis looks good but there are minor code issues.
1. Session Tests -> Session tests
2. Some messages end with a point others don't.
3. We try not to write $val or %val. Write $value or %value. No needless abbreviations, please.
4. Spacing issues with the ++ operator.
Nothing major, but enough to request for a reroll.
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedReading the patch, I also think there is way too much assertions in it. Assertions should be made only where they make sense, and the message of the assertion should be as meaningful as possible.
For example, this is *GOOD*:
But in the next batch, the first assertText is not really useful, and its message ('The session value was correctly passed and not stored.') is misleading.
Please remove all the assertions after all 'session-test/*set/' but the first one.
Comment #18
jhedstromI've re-rolled. Regarding #17, I didn't remove the assertion, but re-worded it.
Those 2 assertions are testing different things. The first one verifies the expected behavior of the session_test module, while the second confirms that the value was not saved. Removing the first one, in the unlikely event that something breaks in the session_test module, would make tracking the problem down all that much more difficult.
Given that, if you still think it's redundant, I'll remove it :)
Comment #19
jhedstromI forgot to replace the misleading wording ("passed and not set") in both instances of the assertion.
Comment #20
jhedstromOkay, this is the last reroll for at least 10 minutes ;)
Towards the goal of making each assertion meaningful, I've added the group parameter to each assertion, so now they show up as "Session" instead of "Other".
Comment #21
boombatower CreditAttribution: boombatower commentedI believe it is preferred that tests don't use the group attribute for general assertions since it can already be assumed that they all relate to the test "Session", and if #250047: Rework the SimpleTest results interface and clean-up of backend code makes it in then it will bold the assertions made by tests anyway.
Comment #22
jhedstromGood to know. In that case, disregard the patch from #20. The patch from #19 is the same without the 'Session' group.
Comment #23
boombatower CreditAttribution: boombatower commentedI believe #19 has addressed all the concerns and applies cleanly.
Comment #24
Dries CreditAttribution: Dries commentedCode style issues are still not taken care of. We only capitalize the first work in a title or description: "Session Test", "Session Value", etc, etc.
"Reset the cookie file to the specified user" should that be "Reset the cookie file of the specified user"?
Comment #25
chx CreditAttribution: chx commentedComment #26
Dries CreditAttribution: Dries commentedPatch is 0 bytes?
Comment #27
chx CreditAttribution: chx commentedCrap.
Comment #28
Dries CreditAttribution: Dries commentedFixed one typo, and committed it to CVS HEAD. Thanks all!
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.