Est-ce une mauvaise pratique de modifier le code uniquement à des fins de test

Je discute avec un collègue programmeur pour savoir s'il est judicieux ou non de modifier un élément de code fonctionnel uniquement pour le rendre testable (via des tests unitaires, par exemple).

Mon opinion est que c'est OK, dans les limites du maintien de bonnes pratiques d'ingénierie logicielle orientées objet et bien sûr (pas "rendre tout public", etc.).

Mon collègue est d'avis que la modification de code (qui fonctionne) uniquement à des fins de test est fausse.

Juste un exemple simple, pensons à ce morceau de code utilisé par un composant (écrit en C #):

public void DoSomethingOnAllTypes()
{
    var types = Assembly.GetExecutingAssembly().GetTypes();

    foreach (var currentType in types)
    {
        // do something with this type (e.g: read it's attributes, process, etc).
    }
}

J'ai suggéré que ce code puisse être modifié pour appeler une autre méthode qui fera le travail réel:

public void DoSomething(Assembly asm)
{
    // not relying on Assembly.GetExecutingAssembly() anymore...
}

Cette méthode utilise un objet Assembly sur lequel travailler, ce qui permet de transmettre votre propre Assembly pour effectuer les tests. Mon collègue ne pensait pas que c'était une bonne pratique.

Qu'est-ce qui est considéré comme une bonne pratique courante?

La modification du code pour le rendre plus testable a des avantages au-delà de la testabilité. En général, un code plus testable

• Est plus facile à entretenir,
• Est plus facile de raisonner,
• Est plus faiblement couplé, et
• A une meilleure conception globale, architecturalement.

Il y a (apparemment) des forces opposées en jeu.

• D'une part, vous souhaitez appliquer l'encapsulation
• D'autre part, vous voulez pouvoir tester le logiciel

Les partisans de la confidentialité de tous les détails de la mise en œuvre sont généralement motivés par le désir de maintenir l'encapsulation. Cependant, garder tout ce qui est verrouillé et indisponible est une approche mal comprise de l’encapsulation. Si l'objectif ultime était de tout garder indisponible, le seul vrai code encapsulé serait le suivant:

static void Main(string[] args)

Votre collègue propose-t-il d'en faire le seul point d'accès dans votre code? Tous les autres codes devraient-ils être inaccessibles par des appelants externes?

À peine. Alors qu'est-ce qui convient pour rendre certaines méthodes publiques? N'est-ce pas finalement une décision de conception subjective?

Pas assez. Ce qui tend à guider les programmeurs, même à un niveau inconscient, est, encore une fois, le concept d’encapsulation. Vous vous sentez en sécurité lorsque vous exposez une méthode publique lorsqu'elle protège correctement ses invariants .

Je ne voudrais pas exposer une méthode privée qui ne protège pas ses invariants, mais souvent vous pouvez le modifier afin qu'il ne protège ses invariants, et puis l' exposer au public (bien sûr, avec TDD, vous faites de la inverse).

L'ouverture d'une API pour la testabilité est une bonne chose , car vous appliquez réellement le principe Open / Closed .

Si vous n'avez qu'un seul appelant de votre API, vous ne savez pas à quel point votre API est réellement flexible. Les chances sont, c'est assez inflexible. Les tests agissent en tant que deuxième client, vous donnant de précieux commentaires sur la flexibilité de votre API .

Ainsi, si les tests suggèrent que vous devriez ouvrir votre API, faites-le; mais maintenez l’encapsulation, non pas en cachant la complexité, mais en exposant la complexité de manière irréprochable.

On dirait que vous parlez d' injection de dépendance . C’est très courant, et à l’OMI, très nécessaire à la testabilité

Pour répondre à la question plus générale de savoir s'il est judicieux de modifier le code uniquement pour le rendre testable, réfléchissez-y de la manière suivante: le code a de multiples responsabilités, notamment: a) être exécuté, b) être lu par des humains, et c) être testé. Les trois sont importants, et si votre code ne remplit pas les trois responsabilités, alors je dirais que ce n'est pas un très bon code. Alors modifiez-vous!

C'est un peu un problème de poulet et d'oeufs.

L’une des principales raisons pour lesquelles il est bon d’avoir une bonne couverture de test de votre code est qu’elle vous permet de refactoriser sans crainte. Mais vous êtes dans une situation où vous devez refactoriser le code afin d'obtenir une bonne couverture de test! Et votre collègue a peur.

Je vois le point de vue de votre collègue. Vous avez un code qui fonctionne (vraisemblablement), et si vous allez le refactoriser - pour une raison quelconque - vous risqueriez de le casser.

Mais si ce code doit faire l'objet d'une maintenance et d'une modification continues, vous courrez ce risque à chaque fois que vous travaillez dessus. Et refactoriser maintenant et obtenir une couverture de test maintenant vous permettra de prendre ce risque, dans des conditions contrôlées, et de mettre le code en meilleure forme pour une modification ultérieure.

Je dirais donc, à moins que cette base de code particulière soit relativement statique et ne devrait pas faire l'objet d'un travail important à l'avenir, que vous voulez faire est une bonne pratique technique.

Bien sûr, qu’il s’agisse d’une bonne pratique commerciale , c’est un sacré casse-tête…

Il peut s’agir d’une différence d’emphase par rapport aux autres réponses, mais je dirais que le code ne doit pas être refondu strictement pour améliorer la testabilité. La testabilité est très importante pour la maintenance, mais la testabilité n’est pas une fin en soi. En tant que tel, je différerais toute refactorisation de ce type jusqu'à ce que vous puissiez prédire que ce code nécessitera une maintenance pour poursuivre certaines activités.

Au moment où vous déterminez que ce code nécessitera un peu de maintenance, ce serait un bon moment pour refactoriser la testabilité. Selon votre analyse de rentabilisation, il peut être judicieux de supposer que tout le code nécessitera éventuellement une maintenance, auquel cas la distinction que je fais avec les autres réponses ici ( par exemple la réponse de Jason Swett ) disparaît.

Pour résumer: la testabilité à elle seule n'est pas (IMO) une raison suffisante pour refactoriser une base de code. La testabilité a un rôle important à jouer dans la maintenance sur une base de code, mais il est impératif pour l'entreprise de modifier la fonction de votre code qui devrait conduire votre refactoring. S'il n'y a pas une telle exigence commerciale, il serait probablement préférable de travailler sur quelque chose qui intéressera vos clients.

(Le nouveau code, bien sûr, est activement maintenu, il devrait donc être écrit pour être testable.)

Je pense que votre collègue a tort.

D'autres ont déjà mentionné les raisons pour lesquelles c'est déjà une bonne chose, mais tant que vous êtes autorisé à le faire, tout devrait bien se passer.

La raison de cette mise en garde est que toute modification du code se fait au prix d'une nouvelle vérification du code. En fonction de ce que vous faites, ce travail de test peut en réalité être un gros effort en soi.

Ce n’est pas nécessairement à vous de prendre la décision de refactoriser ou de travailler sur de nouvelles fonctionnalités qui profiteront à votre entreprise / client.

J'ai utilisé des outils de couverture de code dans le cadre des tests unitaires pour vérifier si tous les chemins d'accès au code étaient exercés. En tant que très bon codeur / testeur, je couvre généralement 80 à 90% des chemins de code.

Lorsque j'étudie les chemins non couverts et que je fais des efforts pour certains d'entre eux, je découvre des bogues tels que des erreurs qui "ne se produiront jamais". Alors oui, la modification du code et la vérification de la couverture de test permettent d'obtenir un meilleur code.

Votre problème ici est que vos outils de test sont de la merde. Vous devriez être capable de simuler cet objet et d'appeler votre méthode de test sans la changer - car si cet exemple simple est très simple et facile à modifier, il se passe quoi quand vous avez quelque chose de beaucoup plus compliqué.

De nombreuses personnes ont modifié leur code pour introduire les classes IoC, DI et les interfaces, simplement pour permettre le test unitaire à l'aide des outils de mocking et de test unitaire nécessitant ces modifications. Je ne crois pas que ce soit une chose saine, pas quand vous voyez du code assez simple qui se transforme en un cauchemar d’interactions complexes entièrement motivé par le besoin de rendre chaque méthode de classe totalement découplée de tout le reste . Et pour ajouter l'insulte à la blessure, nous avons alors de nombreux arguments pour décider si les méthodes privées doivent être testées ou non! (bien sûr ils devraient, quoi

Le problème, bien sûr, réside dans la nature de l’outil de test.

Il existe maintenant de meilleurs outils disponibles qui pourraient mettre ces modifications de conception au lit pour toujours. Microsoft a Fakes (nee Moles) qui vous permet d’empiler des objets concrets, y compris des objets statiques, de sorte que vous n’avez plus besoin de changer votre code pour s’adapter à l’outil. Dans votre cas, si vous utilisiez Fakes, vous remplaceriez l'appel GetTypes par le vôtre qui renvoyait des données de test valides et non valides - ce qui est très important, votre modification suggérée ne prévoit rien du tout.

Pour répondre: votre collègue a raison, mais peut-être pour les mauvaises raisons. Ne changez pas le code pour tester, changez votre outil de test (ou votre stratégie de test complète pour avoir plus de tests unitaires de type intégration au lieu de tests aussi fins).

Martin Fowler discute de ce sujet dans son article « Les blagues ne sont pas des talons»

Une bonne pratique courante consiste à utiliser les journaux de test et de débogage d'unité . Les tests unitaires garantissent que si vous apportez des modifications supplémentaires au programme, votre ancienne fonctionnalité ne se détériorera pas. Les journaux de débogage peuvent vous aider à suivre le programme au moment de l'exécution.
Il arrive parfois que même au-delà de ce besoin, nous n’avons besoin que de quelque chose à des fins de test. Il n'est pas rare de changer le code pour cela. Mais il faut faire attention à ce que le code de production n'en soit pas affecté. En C ++ et C, ceci est réalisé en utilisant la MACRO , qui est une entité de compilation. Ensuite, le code de test n’entre pas du tout dans l’environnement de production. Je ne sais pas si une telle disposition existe en C #.
De plus, lorsque vous ajoutez du code de test dans votre programme, il devrait être clairement visibleque cette partie du code est ajoutée à des fins de test. Ou bien le développeur qui essaie de comprendre le code va simplement transpirer sur cette partie du code.

Il existe de sérieuses différences entre vos exemples. Dans le cas de DoSomethingOnAllTypes(), une implication do somethingest applicable aux types de l'assembly actuel. Mais DoSomething(Assembly asm)indique explicitement que vous pouvez lui passer n’importe quel assemblage.

La raison pour laquelle je le signale est que bon nombre d' institutions dépendant de l'injection de dépendance ne font que sortir du cadre de l'objet d'origine. Je sais que vous avez dit " ne pas tout rendre public" ", mais c'est l'une des plus grosses erreurs de ce modèle, suivie de près par celle-ci: ouvrir les méthodes de l'objet aux utilisations auxquelles elles ne sont pas destinées.

Votre question a donné peu de contexte dans lequel votre collègue a argumenté donc il y a place à la spéculation

"mauvaise pratique" ou non dépend de comment et quand les changements sont faits.

Dans mon cas, votre exemple pour extraire une méthode DoSomething(types)est ok.

Mais j'ai vu du code qui ne va pas comme ça:

public void DoSomethingOnAllTypes()
{
  var types = (!testmode) 
      ? Assembly.GetExecutingAssembly().GetTypes() 
      : getTypesFromTestgenerator();

  foreach (var currentType in types)
  {
     if (!testmode)
     {
        // do something with this type that made the unittest fail and should be skipped.
     }
     // do something with this type (e.g: read it's attributes, process, etc).
  }
}

Ces modifications ont rendu le code plus difficile à comprendre car vous avez augmenté le nombre de chemins de code possibles.

Ce que je veux dire avec quand et comment :

si vous avez une implémentation fonctionnelle et que, dans le but de "mettre en œuvre des capacités de test", vous avez apporté les modifications, vous devez alors tester à nouveau votre application, car vous avez peut-être enfreint votre DoSomething()méthode.

Il if (!testmode)est plus difficile à comprendre et à tester que la méthode extraite.