Les tests unitaires auraient-ils aidé Citigroup à éviter cette erreur coûteuse?

J'ai lu ce qui suit: Un bogue de programmation coûte 7 millions de dollars à Citigroup après des transactions légitimes prises par erreur pour des données de test pendant 15 ans .

Lorsque le système a été mis en place au milieu des années 90, le code de programme filtrait toutes les transactions auxquelles étaient attribués des codes de branche à trois chiffres allant de 089 à 100 et utilisait ces préfixes à des fins de test.

Mais en 1998, la société a commencé à utiliser des codes de succursale alphanumériques à mesure qu’elle développait ses activités. Parmi eux se trouvaient les codes 10B, 10C, etc., que le système considérait comme étant dans la plage des exclus, de sorte que leurs transactions étaient supprimées de tout rapport envoyé à la SEC.

(Je pense que cela illustre le fait que l'utilisation d'un indicateur de données non explicite est ... sous-optimale. Il aurait été préférable de renseigner et d'utiliser une Branch.IsLivepropriété sémantiquement explicite .)

Cela dit, ma première réaction a été "Les tests unitaires auraient aidé ici" ... mais le feraient-ils?

J'ai récemment lu Pourquoi la plupart des tests unitaires sont des déchets intéressants, et ma question est la suivante: à quoi ressemblent les tests unitaires qui auraient échoué lors de l'introduction de codes de branche alphanumériques?

Demandez-vous vraiment "Les tests unitaires auraient-ils aidé ici?", Ou demandez-vous "un type de test aurait-il pu être utile ici?".

La forme de test la plus évidente qui aurait pu aider, est une assertion préalable dans le code lui-même, selon laquelle un identifiant de branche est constitué uniquement de chiffres (en supposant que ce soit l'hypothèse sur laquelle le codeur s'est fondé pour écrire le code).

Cela aurait alors pu échouer dans une sorte de test d'intégration et, dès que les nouveaux identifiants de branche alphanumériques sont introduits, l'assertion explose. Mais ce n'est pas un test unitaire.

Vous pouvez également effectuer un test d'intégration de la procédure qui génère le rapport SEC. Ce test garantit que chaque identifiant de branche réelle rapporte ses transactions (et nécessite donc une entrée réelle, une liste de tous les identifiants de branche utilisés). Donc, ce n'est pas un test unitaire non plus.

Je ne vois aucune définition ni documentation des interfaces impliquées, mais il se peut que les tests unitaires ne puissent éventuellement pas avoir détecté l'erreur car l'unité n'était pas en panne . Si l’unité est autorisée à supposer que les identificateurs de branche ne sont composés que de chiffres et que les développeurs n’ont jamais décidé ce que le code devrait faire dans le cas contraire, ils ne devraient pasrédigez un test unitaire pour imposer un comportement particulier dans le cas d'identificateurs autres que des chiffres, car le test rejetterait une implémentation valide hypothétique de l'unité qui traitait correctement les identificateurs de branche alphanumériques et vous ne souhaitiez généralement pas écrire un test unitaire empêchant la validation. futures implémentations et extensions. Ou peut-être un document écrit il y a 40 ans implicitement défini (via une plage lexicographique dans EBCDIC brut, au lieu d'une règle de classement plus conviviale) que 10B est un identificateur de test car il se situe en réalité entre 089 et 100. Mais alors Il y a 15 ans, quelqu'un a décidé de l'utiliser comme identifiant réel. Le "défaut" ne réside donc pas dans l'unité qui implémente correctement la définition d'origine: cela réside dans le processus qui n'a pas remarqué que 10B est défini comme un identifiant de test et ne doit donc pas être attribué à une branche. La même chose se produirait en ASCII si vous définissiez 089 - 100 en tant que plage de test, puis introduisiez un identifiant 10 $ ou 1.0. Il se trouve que dans EBCDIC, les chiffres viennent après les lettres.

Un test unitaire (ou sans doute un test fonctionnel) qui pourrait éventuellementpourrait avoir sauvé la journée, est un test de l'unité qui génère ou valide les nouveaux identifiants de branche. Ce test affirmerait que les identifiants ne doivent contenir que des chiffres et serait écrit afin de permettre aux utilisateurs des identifiants de branche de prendre la même chose. Ou peut-être y a-t-il une unité quelque part qui importe de vrais identificateurs de branche mais ne voit jamais les identificateurs de test, et qui pourrait être testée d'unités pour s'assurer qu'elle rejette tous les identificateurs de test (si les identificateurs ne sont que trois caractères, nous pouvons les énumérer tous et comparer le comportement de le validateur à celui du test-filtre pour s'assurer qu'ils correspondent, ce qui traite de l'objection habituelle aux tests ponctuels). Ensuite, si quelqu'un modifiait les règles, le test unitaire aurait échoué car il contredirait le comportement nouvellement requis.

Etant donné que le test existait pour une bonne raison, le moment où vous devez le supprimer en raison de modifications des exigences professionnelles devient une opportunité pour que le poste soit attribué à un poste ", recherchez dans le code tout élément qui repose sur le comportement que nous souhaitons. changement". Bien sûr, cela est difficile et donc peu fiable, cela ne garantirait absolument pas de sauver la situation. Mais si vous capturez vos hypothèses dans les tests des unités dont vous assumez les propriétés, alors vous vous donnez une chance et l'effort ne sera donc pas totalement perdu.

Je conviens bien sûr que si l’unité n’avait pas été définie au départ avec une entrée "de forme amusante", il n’y aurait rien à tester. Il peut être difficile de tester correctement les divisions de l'espace de noms fastidieux, car la difficulté réside dans l'application de votre définition amusante, elle consiste à s'assurer que tout le monde comprend et respecte votre définition amusante. Ce n'est pas une propriété locale d'une unité de code. De plus, changer un type de données de "chaîne de chiffres" en "chaîne alphanumériques" revient à faire en sorte qu'un programme basé sur ASCII traite Unicode: ce ne sera pas simple si votre code est fortement couplé à la définition d'origine, et quand le type de données est fondamental pour ce que le programme fait, alors il est souvent fortement couplé.

il est un peu dérangeant de penser que c'est un effort largement gaspillé

Si vos tests unitaires échouent parfois (lors de la refactorisation, par exemple) et que, ce faisant, vous fournissent des informations utiles (votre modification est erronée, par exemple), vos efforts ne sont pas vains. Ce qu’ils ne font pas, c’est tester si votre système fonctionne. Donc, si vous écrivez des tests unitaires au lieu de tests fonctionnels et d'intégration, vous utilisez peut-être votre temps de manière sous-optimale.

Les tests unitaires auraient peut-être permis de comprendre que les codes de branche 10B et 10C avaient été classés à tort comme "branches de test", mais j'estime qu'il est peu probable que les tests pour cette classification de branche aient été suffisamment détaillés pour détecter cette erreur.

D'autre part, des contrôles inopinés des rapports générés auraient pu révéler que les rapports 10B et 10C branchés manquaient systématiquement dans les rapports beaucoup plus tôt que les 15 années où le bogue était maintenant autorisé à rester présent.

Enfin, c’est une bonne illustration de la raison pour laquelle c’est une mauvaise idée de combiner des données de test avec les données de production réelles dans une base de données. S'ils avaient utilisé une base de données séparée contenant les données de test, il n'aurait pas été nécessaire de filtrer cela dans les rapports officiels et il aurait été impossible de filtrer trop.

Le logiciel devait gérer certaines règles de gestion. S'il y avait des tests unitaires, ceux-ci auraient vérifié que le logiciel gérait correctement les règles de gestion.

Les règles commerciales ont changé.

Apparemment, personne ne s'est rendu compte que les règles de gestion avaient changé et personne n'a modifié le logiciel pour appliquer les nouvelles règles de gestion. S'il y avait eu des tests unitaires, ces tests unitaires devraient être modifiés, mais personne ne l'aurait fait car personne ne s'est rendu compte que les règles commerciales avaient changé.

Donc non, les tests unitaires n'auraient pas compris cela.

L'exception serait que les tests unitaires et le logiciel aient été créés par des équipes indépendantes et que l'équipe effectuant les tests unitaires modifie les tests pour appliquer les nouvelles règles de gestion. Ensuite, les tests unitaires auraient échoué, ce qui, espérons-le, aurait entraîné une modification du logiciel.

Bien entendu, dans le même cas, si seul le logiciel était modifié et non les tests unitaires, ceux-ci échoueraient également. Chaque fois qu'un test unitaire échoue, cela ne signifie pas que le logiciel est faux, mais que le logiciel ou que le test unitaire (parfois les deux) est faux.

C'est l'un des gros problèmes avec les tests unitaires: ils vous plongent dans un faux sentiment de sécurité.

Si tous vos tests réussissent, cela ne signifie pas que votre système fonctionne correctement; cela signifie que tous vos tests sont réussis . Cela signifie que les parties de votre conception pour lesquelles vous avez consciemment réfléchi et rédigé des tests fonctionnent comme vous le pensiez consciemment, ce qui n’est vraiment pas si grave de toute façon: c’est le genre de choses sur lesquelles vous portiez une attention particulière. Il est donc fort probable que vous l’ayez bien compris! Mais cela ne fait rien pour attraper des cas auxquels vous n'avez jamais pensé, comme celui-ci, car vous n'avez jamais pensé à écrire un test pour eux. (Et si vous l'aviez fait, vous auriez compris que cela signifiait que des modifications de code étaient nécessaires et que vous les auriez modifiées.)

Non pas forcément.

À l'origine, l'exigence était d'utiliser des codes de branche numériques, de sorte qu'un test unitaire aurait été produit pour un composant acceptant divers codes et rejetant tout code similaire 10B. Le système aurait été passé comme fonctionnant (ce qui était).

Ensuite, l'exigence aurait changé et les codes mis à jour, mais cela aurait signifié que le code de test unitaire qui a fourni les données incorrectes (qui sont maintenant de bonnes données) devrait être modifié.

Nous supposons maintenant que les responsables du système sauraient que c'était le cas et modifieraient le test unitaire pour gérer les nouveaux codes ... mais s'ils savaient que cela se produisait, ils auraient également su modifier le code qui les traitait. codes de toute façon .. et ils ne l'ont pas fait. Un test unitaire qui avait initialement rejeté le code 10B aurait volontiers dit "tout va bien ici" lors de l'exécution, si vous ne saviez pas mettre à jour ce test.

Les tests unitaires conviennent au développement original, mais pas au système, en particulier pas 15 ans après que les exigences ont été oubliées.

Ce dont ils ont besoin dans ce genre de situation est un test d’intégration de bout en bout. Une où vous pouvez transmettre les données que vous comptez travailler et voir si elles le font. Quelqu'un aurait remarqué que leurs nouvelles données d'entrée ne produisaient pas de rapport et enquêteraient ensuite davantage.

Le test de type (le processus de test des invariants utilisant des données valides générées aléatoirement, comme illustré par la bibliothèque de tests Haskell QuickCheck et divers ports / alternatives inspirés par celui-ci dans d'autres langues) aurait bien pu résoudre ce problème, les tests unitaires ne l'auraient certainement pas fait .

En effet, lorsque les règles de validité des codes de branche ont été mises à jour, il est peu probable que quiconque ait pensé à tester ces plages spécifiques pour s’assurer de leur bon fonctionnement.

Toutefois, si les essais de type avait été utilisé, quelqu'un devrait au moment a été mis en œuvre le système d' origine ont écrit une paire de propriétés, un pour vérifier que les codes spécifiques pour les branches d'essai ont été traitées comme des données de test et un pour vérifier qu'il n'y a pas d' autres codes Etait ... lorsque la définition du type de données pour le code de branche a été mise à jour (ce qui aurait été nécessaire pour permettre de vérifier que les modifications apportées au code de branche d’un chiffre à un autre ont fonctionné), ce test aurait commencé à tester les valeurs dans la nouvelle gamme et aurait très probablement identifié la faute.

Bien sûr, QuickCheck a été développé pour la première fois en 1999, il était donc déjà trop tard pour résoudre ce problème.

Je doute vraiment que les tests unitaires fassent une différence dans ce problème. Cela ressemble à une de ces situations de vision en tunnel parce que la fonctionnalité a été modifiée pour prendre en charge les nouveaux codes de branche, mais cela n’a pas été appliqué dans toutes les zones du système.

Nous utilisons des tests unitaires pour concevoir une classe. La réexécution d'un test unitaire n'est requise que si la conception a été modifiée. Si une unité particulière ne change pas, les tests unitaires inchangés renverront les mêmes résultats qu'auparavant. Les tests unitaires ne vous montreront pas l'impact des modifications sur d'autres unités (sinon, vous n'écrivez pas de tests unitaires).

Vous pouvez uniquement détecter ce problème de manière raisonnable via:

• Tests d'intégration - mais vous devrez ajouter spécifiquement les nouveaux formats de code pour alimenter plusieurs unités du système (c'est-à-dire qu'ils ne vous montreront le problème que si les tests d'origine incluaient les branches désormais valides)
• Tests de bout en bout - l'entreprise doit exécuter un test de bout en bout intégrant les anciens et les nouveaux formats de code de branche.

Ne pas avoir suffisamment de tests de bout en bout est plus inquiétant. Vous ne pouvez pas vous fier aux tests unitaires comme test ONLY ou MAIN pour modifier le système. On dirait que cela demande seulement à quelqu'un de créer un rapport sur les nouveaux formats de code de branche pris en charge.

Une assertion intégrée à l'exécution aurait pu être utile; par exemple:

1. Créer une fonction comme bool isTestOnly(string branchCode) { ... }
2. Utilisez cette fonction pour choisir les rapports à filtrer.
3. Réutilisez cette fonction dans une assertion, dans le code de création de branche, pour vérifier ou affirmer qu'une branche n'est pas (ne peut pas être) créée à l'aide de ce type de code de branche
4. Avoir cette assertion activée dans le vrai run-time (et non "optimisée loin sauf dans la version du code réservée au développement par le débogage")

Voir également:

• Séparez les versions 'debug' et 'release'? (c.-à-d. activer les assertions au moment de l'exécution)
• Doit-on tester la mise en œuvre interne, ou seulement le comportement du public? (c’est-à-dire que les tests système sont plus utiles et offrent une meilleure couverture que les tests unitaires)

La solution consiste à échouer rapidement .

Nous n'avons pas le code, ni beaucoup d'exemples de préfixes qui sont ou non des préfixes de branche de test en fonction du code. Tout ce que nous avons c'est ceci:

• 089 - 100 => branche de test
• 10B, 10C => branche de test
• <088 => vraisemblablement de vraies branches
• > 100 => branches vraisemblablement réelles

Le fait que le code autorise les nombres et les chaînes est plus qu'un peu étrange. Bien entendu, 10B et 10C peuvent être considérés comme des nombres hexadécimaux, mais si les préfixes sont tous traités comme des nombres hexadécimaux, les valeurs 10B et 10C se situent en dehors de la plage de test et seront traitées comme de véritables branches.

Cela signifie probablement que le préfixe est stocké sous forme de chaîne mais traité comme un nombre dans certains cas. Voici le code le plus simple auquel je puisse penser qui reproduit ce problème (en utilisant C # à des fins d'illustration):

bool IsTest(string strPrefix) {
    int iPrefix;
    if(int.TryParse(strPrefix, out iPrefix))
        return iPrefix >= 89 && iPrefix <= 100;
    return true; //here is the problem
}

En anglais, si la chaîne est un nombre compris entre 89 et 100, il s'agit d'un test. Si ce n'est pas un nombre, c'est un test. Sinon ce n'est pas un test.

Si le code suit ce modèle, aucun test unitaire ne l'aurait détecté au moment du déploiement du code. Voici quelques exemples de tests unitaires:

assert.isFalse(IsTest("088"))
assert.isTrue(IsTest("089"))
assert.isTrue(IsTest("095"))
assert.isTrue(IsTest("100"))
assert.isFalse(IsTest("101"))
assert.isTrue(IsTest("10B")) // <--- business rule change

Le test unitaire montre que "10B" doit être traité comme une branche de test. L'utilisateur @ gnasher729 ci-dessus indique que les règles commerciales ont changé et c'est ce que la dernière assertion ci-dessus montre. À un moment donné, cette affirmation aurait dû passer à un isFalse, mais cela ne s'est pas produit. Les tests unitaires sont exécutés au moment du développement et de la construction, mais à aucun moment par la suite.

Quelle est la leçon ici? Le code a besoin d'un moyen pour signaler qu'il a reçu une entrée inattendue. Voici un autre moyen d'écrire ce code qui souligne qu'il s'attend à ce que le préfixe soit un nombre:

// Alternative A
bool TryGetIsTest(string strPrefix, out bool isTest) {
    int iPrefix;
    if(int.TryParse(strPrefix, out iPrefix)) {
        isTest = iPrefix >= 89 && iPrefix <= 100;
        return true;
    }
    isTest = true; //this is just some value that won't be read
    return false;
}

Pour ceux qui ne connaissent pas C #, la valeur de retour indique si le code a été capable d'analyser un préfixe de la chaîne donnée. Si la valeur de retour est true, le code appelant peut utiliser la variable isTest out pour vérifier si le préfixe de branche est un préfixe de test. Si la valeur de retour est false, le code appelant doit signaler que le préfixe donné n'est pas attendu et que la variable isTest out n'a pas de sens et doit être ignorée.

Si vous êtes d'accord avec les exceptions, vous pouvez le faire à la place:

// Alternative B
bool IsTest(string strPrefix) {
    int iPrefix = int.Parse(strPrefix);
    return iPrefix >= 89 && iPrefix <= 100;
}

Cette alternative est plus simple. Dans ce cas, le code appelant doit intercepter l'exception. Dans les deux cas, le code devrait indiquer à l'appelant qu'il ne s'attend pas à un strPrefix qui ne pourrait pas être converti en entier. De cette façon, le code échoue rapidement et la banque peut rapidement trouver le problème sans la gêne de la SEC.

Autant de réponses et même pas une citation de Dijkstra:

Les tests montrent la présence, pas l’absence de bugs.

Donc ça dépend. Si le code a été testé correctement, ce bogue n'existerait probablement pas.

Je pense qu'un test unitaire ici aurait permis de s'assurer que le problème n'existerait jamais.

Considérez que vous avez écrit la bool IsTestData(string branchCode)fonction.

Le premier test unitaire que vous écrivez devrait porter sur les chaînes nulles et vides. Ensuite, pour les chaînes de longueur incorrecte, puis pour les chaînes non entières.

Pour réussir tous ces tests, vous devrez ajouter la vérification des paramètres à la fonction.

Même si vous ne testez que les 'bonnes' données 001 -> 999 sans penser à la possibilité de 10 A, la vérification des paramètres vous obligera à réécrire la fonction lorsque vous commencerez à utiliser des caractères alphanumériques pour éviter les exceptions qu'elle générera.