Never be to sure without Simpletest

It took me some time to start playing around with simpletest in Drupal which really is a shame. A few hours ago, I decided to start digging into it trying to write my first tests. I'm a real happy man now, because 4 lines failed on me testing a simple validation function I wrote. At first, I thought my test code was wrong, but looking at my validation function again made me realize I had a logical flaw in this code. This was my original validation function:

<?php
// formelement will be empty when called from outside a form validation
function _cjp_validate_token($string, $formelement = '') {
  if (!empty(
$string)) {
   
$token = db_fetch_object(db_query("SELECT cjp_tokens_token_expire FROM {cjp_tokens_token} WHERE cjp_tokens_token_string = '%s'", $string));
    if (
$token == false && !empty($formelement)) {
     
form_set_error($formelement, t('Je toegangscode is niet geldig.'));
    }
    elseif (
$token != false && !empty($formelement) && $token->cjp_tokens_token_expire < time()) {
     
form_set_error($formelement, t('Je toegangscode is vervallen.'));
    }
    elseif (
$token == false && empty($formelement)) {
      return
false;
    }
    elseif (
$token != false && empty($formelement)) {
      return
true;
    }
  }
}
?>

The simple test I wrote was testing if an existing token wasn't expired yet:

<?php
function testInvalidToken() {
 
$token = _cjp_generate_token("SimpleTest", 1, time()-3600, 1, true);
 
$result = _cjp_validate_token($token);
 
$this->assertFalse($result, 'Token is expired');
 
db_query("DELETE FROM {cjp_tokens_token} WHERE cjp_tokens_token_string = '%s'", $token);
}
?>

Since I didn't pass a second parameter in the validate function, the first two statements are ignored. Normally it should enter the third one returning false but since I did not test on the cjp_tokens_token_expire field, it went on to the last statement returning true. The test failed so I needed to fix code which I otherwhise never would have looked at again since I was quite sure it was working. Never be to sure. Anyway, changing the third statement as you can see underneath made the simpletest pass and me a very happy man.

<?php
 
elseif ($token != false && empty($formelement) && $token->cjp_tokens_token_expire < time()) {
?>

So for now, I promise never to write any code again without tests (if applicable of course).

Comments

Submitted by Pasqualle on July 30, 2008 - 17:46

if you try to use the "return at first error" programming technique you will have all the possibilities covered, and the code will be easier to understand.

this is your actual code converted to this format, please validate

<?php
// formelement will be empty when called from outside a form validation
function _cjp_validate_token($string, $formelement = '') {
  if (empty(
$string)) {
    return
true;
  }

 

$token = db_fetch_object(db_query("SELECT cjp_tokens_token_expire FROM {cjp_tokens_token} WHERE cjp_tokens_token_string = '%s' AND cjp_tokens_token_uid = 0", $string));

  if (!

$token) {
    if (empty(
$formelement)) {
      return
false;
    }
   
form_set_error($formelement, t('Je toegangscode is niet geldig.'));
    return
false;
  }

  if (

$token->cjp_tokens_token_expire < time()) {
    if (empty(
$formelement)) {
      return
false;     
    }
   
form_set_error($formelement, t('Je toegangscode is vervallen.'));
    return
false;
  }
  return
true;
}
?>

I think this is very powerful coding style, there is no else clause in the code. I really miss that it is not used in Drupal, all validation functions should be written in this format.

Edit: Very nice code, I validated and updated it to a very nice working piece.

Submitted by Pasqualle on July 30, 2008 - 18:26

I am not sure about the (removing return) changes you made in the code, now there is a possibility when you can see two error messages..

Submitted by swentel on July 30, 2008 - 19:08

Hmm, you're right. I'm setting the error on the same element, that's why I thought it didn't matter. Should read the api better :)

You are here