PHPPRO

Les tests unitaires, pour ceux qui les utilisent, sont bien pratiques pour tester notre code. Malheureusement ils peuvent introduire, si ils sont rédigés de façon maladroite des problèmes qui peuvent être compliqués à comprendre a posteriori. Imaginons le code suivant (volontairement) mal codé :

class UserService {
    /**
      * Checks user access rights
      *
      * @param User $user the user
      *
      * @return null
      * @throws Exception if access is denied for the specified user
      */
    public function checkAccess($user) {
        // ... (100 lignes)
        if($user->role==='standard') {
            throw new Exception("Access denied");
        }
        if($user->role==='basic-admin') {
            throw new Exception("Access denied");
        }
        if($user->role==='editor') {
            throw new Exception("Access denied");
        }
        // ... (100 lignes)
    }
}

Imaginons que nous n'avions pas écrit ce code nous même, et qu'il soit assez volumineux et complexe (plus de 300 lignes). Nous désirons rajouter des tests unitaires sur cette méthode, nous réalisons le premier test suivant :

class UserServiceTest extends PHPUnit_Framework_TestCase {
    public function testGetUserForIdNullThrowException() {
        $svc = new UserService();
        try {
            $svc->checkUser(null);
            $this->fail("No exception thrown");
        } catch(Exception $e) {
        }
    }
}

Nous exécutons le test pour voir le résultat et ... le test est vert (succès). Confiant dans notre code, nous pensons que ce cas ($user null) est traité. Grosse erreur (je vous le dis, cela n'est pas traité dans le code, mais le test est quand même vert) !

Nous sommes en effet devant un énorme effet de bord du au fonctionnement du framework PHPUnit...

En effet, lorsque nous appelons checkUser(null), le code de checkUser ne vérifie pas la nullité du paramètre $user, il fait les différents if($user->role === ...), (avec $user == null), mais comme vous le savez PHP est permissif et ne va donc pas planter, les if seronts juste faux et aucune exception ne sera levée par notre code applicatif. la ligne en gras suivante:

        try {
__            $svc->checkUser(null);__
            $this->fail("No exception thrown");
        } catch(Exception $e) {
        }

ne va donc pas lever d'exceptions (comme attendu), et la ligne suivante dans le tests unitaires va être exécutée :

        try {
            $svc->checkUser(null);
__            $this->fail("No exception thrown");__
        } catch(Exception $e) {
        }

Cette ligne (le fail), va en réalité lever une exception de type Exception (c'est le fonctionnement standard de PHPUnit). Or, comme nous avons fait un try/catch par dessus, cette exception va être attrapé et le bloc catch va être exécuté, comme il est vide, aucune assertion fausse ne sera déclenchée et la fin du test sera atteinte sans aucune erreur, le test sera donc : vert (succès) !

Le test sera donc en succès alors que notre code ne lève pas d'exception quand le $user est null !

Je vous l'accorde, il est recommandé de mettre des assertions dans toutes les branches d'un test, nous aurions pu alors écrire :

        try {
            $svc->checkUser(null);
            $this->fail("No exception thrown");
        } catch(Exception $e) {
            $this->assertTrue(true);
        }

Le problème aurait été le même et nous serions passé dans le catch, l'assertions triviale aurait été vraie et le test sera toujours vert !

Nous aurions pu voir apparaître le problème si nous avions fait une assertions légèrement différente dans le catch :

        try {
            $svc->checkUser(null);
            $this->fail("No exception thrown");
        } catch(Exception $e) {
            $this->assertEquals("Access denied",$e->getMessage());
        }

Le test aurait été rouge (cool !) mais le message d'échec aurait été le suivant :

Failed asserting that <string:No exception thrown> is equal to <string:Access denied>

Quézako ?

En fait, dans la branche catch(), $e contient l'exception levée par le fail() (car notre code ne lève pas d'exception dans ce cas puisqu'il est buggé) et PHPUnit tente donc de faire une assertion entre la chaîne "Access denied" écrite en dur dans le test, et le message de l'exception qui est en fait "No exception thrown" !

Une méthode plus sûre

Mais alors qu'elle est la bonne méthode pour écrire un test qui aurait mis en exergue le bug de notre méthode checkAccess() ?

Le problème réel vient du fait que nous réalisons un catch sur le même type d'exception que celui levé par le framework PHPUnit (Exception), il suffit juste de réaliser le catch sur un type d'exception plus spécialisé, par exemple NullPointerException (classe à créer) comme ceci :

        try {
            $svc->checkUser(null);
            $this->fail("No exception thrown");
        } catch(NullPointerException $e) {
            $this->assertEquals("Access denied",$e->getMessage());
        }

Lorsque nous exécuterions le test, il échouerait, et le message d'erreur serait :

No exception thrown

Gagné ! Nous aurions vu que notre code applicatif était buggé et qu'il ne traite pas le cas ou le user est nul. Cela impose cependant que l'exception levée par notre méthode checkAccess() dans le cas d'un user null soit par exemple du type NullPointerException (et non pas Exception).

Récemment, PHPUnit a ajouté le support des "annotations" dans le commentaire de la méthode de test pour indiquer que le test doit lever une exception, nous aurions pu utiliser le test suivant :

class UserServiceTest extends PHPUnit_Framework_TestCase {
    /**
      * @expectedException NullPointerException
      */
    public function testGetUserForIdNullThrowException() {
        $svc = new UserService();
        $svc->checkUser(null);
    }
}

il est bien plus concis !

Conclusion :

Spécialisez toujours vos exceptions dans votre code, n'utilisez jamais des exceptions de type Exception si vous voulez réaliser des tests unitaires.