Tests needed: session.inc

The attached patch adds tests for sess_count() and session_save_session().

I 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.

Yeah, the tests I wrote are super low level. I'll try and get some data-persistence tests written soon.

#268063: Move includes/tests to simpletest/tests & provide hidden .info propery

chx,

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.

what about:

The easiest to check for persistence (and most probably the only hack free way) is to have a page callback which sets something in SESSION and then another that dumps it.

-chx (http://drupal.org/node/268063#comment-955082)

It seems that ideally, the drupalLogin() method would create a session...

To create a new session, you will have to first empty the cookie jar of the internal browser. Something like this should do the trick:

   $this->curl_options['CURLOPT_COOKIESESSION'] = TRUE;
   $this->drupalGet('');
   unset($this->curl_options['CURLOPT_COOKIESESSION']);

Cool. 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

I'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:

   $this->curl_options[CURLOPT_COOKIESESSION] = TRUE;
   $this->drupalGet('');
   unset($this->curl_options[CURLOPT_COOKIESESSION]);

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.

subscribe

I've got the session counting working. I had to implement the following every time I wanted to change sessions:

  /**
   * Reset the cookie file to the proper user.
   */
  function _session_reset($uid = 0) {
    // close the internal browser
    $this->curlClose();

    // change cookie file for user
    $this->cookie_file = file_directory_temp() . '/cookie.' . $uid . '.txt';
    $this->curl_options[CURLOPT_COOKIEFILE] = $this->cookie_file;
    $this->curl_options[CURLOPT_COOKIESESSION] = TRUE;
    $this->drupalGet('session-test/get');
    $this->assertResponse(200, t('Session test module is correctly enabled.'));
  }

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.

I 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.

Took a look at the code, looks good. I made a few minor style changes:

• Don't need to implement tearDown if no code, parent will automatically be called.
• Removed extra white-space.
• Comments should begin with capital and end with period.
• I believe there should be no space between variable and ++.
• Added documentation of parameter on sessionReset.
• Remove prefix _ from sessionReset since it is part of test class which should never be called by any other code anyway.

These are minor changes, but otherwise the test passes and looks good.

Soooooooooooo very nice! +1000000000000 yesyesyesyesyesyesyesyesyesyesyesyes

This 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.

Reading 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*:

+    $val_1 = $this->randomName();
+    $this->drupalGet('session-test/set/' . $val_1);
+    $this->assertText($val_1, t('The session value was stored.'));
+    $this->drupalGet('session-test/get');
+    $this->assertText($val_1, t('Session correctly returned the stored data for an authenticated user.'));

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.

+    // Attempt to write over val_1. If session_save_session(FALSE) is working.
+    // properly, val_1 will still be set.
+    $val_2 = $this->randomName();
+    $this->drupalGet('session-test/no-set/' . $val_2);
+    $this->assertText($val_2, t('The session value was correctly passed and not stored.'));
+    $this->drupalGet('session-test/get');
+    $this->assertText($val_1, t('Session data is not saved for session_save_session(FALSE).'));

Please remove all the assertions after all 'session-test/*set/' but the first one.

I've re-rolled. Regarding #17, I didn't remove the assertion, but re-worded it.

    $this->drupalGet('session-test/no-set/' . $value_2);
    $this->assertText($value_2, t('The session value was correctly passed to session-test/no-set.'));
    $this->drupalGet('session-test/get');
    $this->assertText($value_1, t('Session data is not saved for session_save_session(FALSE).'));

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 :)

I forgot to replace the misleading wording ("passed and not set") in both instances of the assertion.

Okay, 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".

I 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.

Good to know. In that case, disregard the patch from #20. The patch from #19 is the same without the 'Session' group.

I believe #19 has addressed all the concerns and applies cleanly.

Code 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"?

Patch is 0 bytes?

Crap.

Fixed one typo, and committed it to CVS HEAD. Thanks all!