3
\$\begingroup\$

No one in my team knows how to write BDD tests, so I started writing some and it's quite working well. I think the time has come to improve the code quality.

This contains a lot of duplicated code and unused features:

<?php
require_once ('core/v3/engine.php');
require_once 'PHPUnit/Extensions/Story/TestCase.php';

class cHelperValidationSpec extends PHPUnit_Extensions_Story_TestCase {

    /**
     * Check all the output for the right cases
     * @scenario
     */
    public function specForJobId() {
        $this->given('For a mValue (mixedValue) and we want to validate if it is an integer')
            ->when('sJobId is a positive integer', 123)
            ->then('return of positive integer should be', 'TRUE')
            ->when('sJobId is a negative integer', -123)
            ->then('return of a negative integer should be', 'FALSE')
            ->when('sJobId is a float', 1.23)
            ->then('return of a float should be', 'FALSE')
            ->when('sJobId is a string containing only digits', '123')
            ->then('return of a string (containing only digits) should be', 'TRUE')
            ->when('sJobId is a string containing not only digits', '123abc-?')
            ->then('return of a string (containing not only digits) should be', 'TRUE');
    }

    public function runGiven(&$world, $action, $arguments) {
        switch($action) {
            case 'For a mValue (mixedValue) and we want to validate if it is an integer': {
                $world['helper'] = new cHelperValidation();
            }
            break;

            default: {
                return $this->notImplemented($action);
            }
        }
    }

    public function runWhen(&$world, $action, $arguments) {
        switch($action) {
            case 'sJobId is a positive integer': {}
            case 'sJobId is a negative integer': {}
            case 'sJobId is a float': {}
            case 'sJobId is a string containing only digits': {}
            case 'sJobId is a string containing not only digits': {}
            break;

            default: {
                return $this->notImplemented($action);
            }
        }
    }

    public function runThen(&$world, $action, $arguments) {
        switch($action) {
            case 'return of positive integer should be': {
                $this->assertEquals(TRUE, $world['helper']->isId(123));
            }
            case 'return of a negative integer should be': {
                $this->assertEquals(FALSE, $world['helper']->isId(-123));
            }
            case 'return of a float should be': {
                $this->assertEquals(FALSE, $world['helper']->isId(1.23));
            }
            case 'return of a string (containing only digits) should be': {
                $this->assertEquals(TRUE, $world['helper']->isId('123'));
            }
            case 'return of a string (containing not only digits) should be': {
                $this->assertEquals(FALSE, $world['helper']->isId('123abc-?'));
            }
            break;
            default: {
                return $this->notImplemented($action);
            }
        }
    }

}

The only reference I'm using for BDD is here, so there is much potential for improving this code.

\$\endgroup\$
1
  • \$\begingroup\$ Why you don't use behat with gherkins syntax? It's a PHP lib which is similar to Cucumber in Ruby on Rails. \$\endgroup\$
    – BlackScorp
    Oct 22, 2013 at 7:32

1 Answer 1

3
\$\begingroup\$

Your switch statements will run through until they find a break statement. Having the braces {} only defines a block of code and has no effect on breaking out of the case.

switch (1)
{
case 1: {
   echo 'One';
}
case 2: {
   echo 'Two';
}
default: {
   echo 'Also default';
      }
}

The one above prints out "OneTwoAlso default" whereas the following prints out "One".

switch (1)
{
case 1:
   echo 'One';
   break;
case 2:
   echo 'Two';
   break;
default:
   echo 'Also default';
}

I think your testing would be cleaner if you didn't use then and when methods. Normally I create an array of test data and expected results and loop over them. As you are writing the test for the isId method I would have something like this:

public function test_isID()
{
   $object = new Object_That_You_Are_Testing();

   $tests = array(
      array('Data'            => 123,
            'Expected_Result' => true,
            'Test_Name'       => 'Positive Integer'),
      array('Data'            => -123,
            'Expected_Result' => false,
            'Test_Name'       => 'Negative Integer'));

   foreach ($tests as $test)
   {
      $this->assertEquals($test['Expected_Result'],
                          $object->isID($test['Data']),
                          $test['Test_Name'] . ' Failed');
   }
}

You could do more clever things than this. Also a test will not always be so simple. Normally I find that I have mocked objects and am ensuring that they get called appropriately.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for this great idea - will try it when I'm writing next tests. \$\endgroup\$ Jan 2, 2012 at 11:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.