Façons d'améliorer ce code

Question

J'essaye d'écrire du code de test pour mon application Java en utilisant Scalatest. J'ai pensé, puisque Scala a une syntaxe beaucoup plus lisible, il en résulterait un code de test plus lisible.

Jusqu'à présent, ce que je réussi:

 
package com.xyz 

import org.scalatest.FlatSpec 
import org.scalatest.matchers.ShouldMatchers 
import com.xyz.SecurityService 
import org.mockito.Mockito._ 
import org.scalatest.mock.MockitoSugar 
import org.mockito.Matchers._ 
import javax.servlet.jsp.tagext.Tag 

class CheckRoleTagSpec extends FlatSpec with ShouldMatchers with MockitoSugar { 

    behavior of "CheckRole tag" 

    it should "allow access when neither role nor root defined" in { 
    val securityServiceMock = mock[SecurityService] 

    val tag = new CheckRoleTag() 
    tag.setSecurityService(securityServiceMock) 

    tag.setGroup("group") 
    tag.setPortal("portal") 

    tag.setRoot(false) 
    tag.setRole(null) 

    tag.doStartTag should be(Tag.SKIP_BODY) 
    } 

} 

Je suis très déçu par ce code. C'est pratiquement la même chose que je devrais écrire en Java. S'il vous plaît aidez-moi à le rendre plus scala-like et fonctionnel.

Answer 1

Le code ci-dessous crée une nouvelle classe anonyme, mais doStartTag renvoie le résultat comme prévu:

... 
(new CheckRoleTag{ 
    setSecurityService(mock[SecurityService]) 
    setGroup("group") 
    setPortal("portal") 
    setRoot(false) 
    setRole(null) 
} doStartTag) should be(Tag.SKIP_BODY) 
... 

Answer 2

Depuis ce test particulier est simplement appeler un groupe de setters sur un objet implémenté en Java, il n'y a pas beaucoup vous peut faire pour le rendre plus succinct ou fonctionnel ou scalaish. Vous pouvez supprimer des répétitions avec quelque chose comme

it should "allow access when neither role nor root defined" in { 
    val securityServiceMock = mock[SecurityService] 

    val tag = new CheckRoleTag() 

    locally { 
    import tag._ 
    setSecurityService(securityServiceMock) 
    setGroup("group") 
    setPortal("portal") 
    setRoot(false) 
    setRole(null) 
    } 

    tag.doStartTag should be(Tag.SKIP_BODY) 
} 

Je ne suis pas sûr que ce soit vraiment la peine dans ce cas.

Answer 3

Vous ne pouvez pas corriger un test moche en réécrivant le test. Vous ne pouvez le réparer qu'en redessinant l'API en cours de test.

Eh bien, sur le plan technique, il est possible d'écrire des tests laids pour les bonnes API si vous essayez vraiment dur, sont très mal, très, stupide, très ivre ou très fatigué. Mais écrire un test moche prend effort et les programmeurs sont paresseux, il est donc très peu probable que quelqu'un écrirait un test moche par choix. Il est presque impossible d'écrire des tests laids: on colle quelque chose, on sort quelque chose, on vérifie si on a ce qu'on attendait. C'est tout. Il n'y a vraiment rien à gâcher là. Un test utilise simplement l'API de la même manière qu'un utilisateur de l'API l'utiliserait. Il s'agit essentiellement d'un exemple d'utilisation correcte de l'API, qui permet également, presque comme un effet secondaire, de vérifier que l'API est correctement implémentée correctement . C'est pourquoi un test moche est un bon indicateur de la mauvaise conception de l'API, et c'est aussi la raison pour laquelle le test de la conception de l'API est une bonne chose, même si vous ne faites pas de TDD.

Dans ce cas particulier, je peux voir plusieurs façons d'améliorer l'API, bien que ces suggestions soient nécessairement incomplètes, superficielles et simplistes (pour ne pas dire probablement fausses), car je ne sais rien de votre domaine:

• meilleurs noms: setRoot sonne comme il met la racine. Mais, à moins que false soit la racine de votre hiérarchie, je suppose que ce que c'est en fait est de savoir si oui ou non cette balise est la racine. Donc, il devrait plutôt être nommé isRoot ou makeRoot ou setIsRoot ou quelque chose comme ça.
• Better Defaults: en continuant avec setRoot, en supposant que ma supposition est correcte et que ceci détermine si le Tag est la racine, alors la valeur par défaut est la mauvaise. Par la définition même du concept de "root", il ne peut y avoir qu'une racine .Ainsi, vous forcez vos utilisateurs à spécifier setRoot(false)à chaque fois, à l'exception de un moment où ils définissent réellement une racine. Les balises non-root devraient être la valeur par défaut, et vous devriez seulement être forcé à setRoot(true) pour ce un Tag qui en fait est la racine. Meilleurs défauts, partie II: setRole(null). Sérieusement? Vous forcez vos utilisateurs à explicitement définir le rôle à être unset? Pourquoi ne pas simplement désactiver la valeur par défaut? Après tout, le test s'appelle "... quand ni le rôle ni la racine ne sont définis", alors pourquoi les définir?
• API Courant/Builder modèle: si vous vraiment doivent construire des objets non valides (mais voir point suivant), au moins utiliser quelque chose comme une API Fluent ou un motif Builder.
• Construire uniquement des objets valides: Mais, en réalité, les objets doivent toujours être valides, complets et entièrement configurés lorsqu'ils sont construits. Vous ne devriez pas avoir à construire un objet, et puis le configurer.

De cette façon, le test devient essentiellement:

package com.xyz 

import org.scalatest.FlatSpec 
import org.scalatest.matchers.ShouldMatchers 
import com.xyz.SecurityService 
import org.mockito.Mockito._ 
import org.scalatest.mock.MockitoSugar 
import org.mockito.Matchers._ 
import javax.servlet.jsp.tagext.Tag 

class CheckRoleTagSpec extends FlatSpec with ShouldMatchers with MockitoSugar { 
    behavior of "CheckRole tag" 
    it should "allow access when neither role nor root defined" in { 
    val tag = new CheckRoleTag(mock[SecurityService], "group", "portal") 

    tag.doStartTag should be(Tag.SKIP_BODY) 
    } 
}