Temps estimé: 1 heure

Critique de tests unitaires

Pour chacun des tests ci-dessous, identifiez les problèmes.

Note: Il peut y avoir plusieurs autres problèmes que ceux donnés dans la solution. Nous avons focalisé sur le(s) plus gros problème(s) pour chaque question. Si vous avez trouvé autre chose et que vous voulez valider votre réponse, n'hésitez pas à nous contacter!

Question 1

@Test
public void givenANumber_whenNumberIsBiggerThanFive_thenNumberIsActivated() {
   double aNumber = Math.random() % 10;

   numberActivator.activateNumbersBiggerThanFive(aNumber);

   if (aNumber > 5) {
       assertTrue(numberActivator.hasActivated(aNumber));
   } else {
       assertFalse(numberActivator.hasActivated(aNumber));
   }
}

☑️ Solution

Ce test devrait être séparé en deux tests dans lesquels l'élément aléatoire devrait être enlevé.

Aussi, le test est difficile à lire puisque c'est un gros bloc de code. Votre code de test doit être autant propre que votre code de production!

Question 2

@Test
public void givenARandomColor_whenCheckingIfColorIsRed_thenColorCanBeRed() {
   Color aRandomColor = Color.random();

   boolean isRed = colorAnalyser.isRed(aRandomColor);

   assertTrue(isRed);
}

☑️ Solution

Un test unitaire doit être stable dans sa manière de tester le comportement d'une méthode.

Il existe des techniques pour utiliser de l'entropie afin de tester, mais ne vous lancer pas dans cela dans le cadre du cours. Ici, on dit que le test est "flickering" car il va échouer sans raison.

Question 3

@Test
public void givenAnAdult_whenAdultHasChildren_thenAdultBecomesAParent() {
   double anAdult = new Adult();

   anAdult.addChildren();

   boolean isParent = anAdult.isParent();
}

☑️ Solution

Il n'y a aucun asserts. Ce test sert à rien et il n'aurait jamais pu échouer s'il avait été écrit en TDD!

Ne sous-estimez pas ce problème, ça arrive plus souvent qu'on le croit et c'est simple à éviter : prennez le temps d'admirer votre test qui échoue! Vous pouvez aussi prendre l'habitude d'écrire l'assertion en premier si vous préférez.

Question 4

@Test
public void givenAVirus_whenVirusDestroysEconomy_thenStockMarketGoesUp() {
   Virus aVirus = new Virus();

   economy.isDestroyedBy(aVirus);

   assertThat(economy.getStockMarketDirection()).isEqualTo(Direction.UP);
}

☑️ Solution

Il n'y a pas d'erreur majeure dans ce test. Certains pourraient avoir des préférences sur certaines techniques, mais il n'y a, à proprement parlé, pas d'erreur selon ce que nous avons vu à ce jour.

Question 5

@Test
public void givenTeamMembers_whenWorkingEfficientlyAsATeam_thenWorkIsEasy() {
   TeamMembers teamMembers = new TeamMembers();

   project.workWith(teamMembers);

   assertThat(project).contains(teamMembers);
}

☑️ Solution

Le nom doit représenter le comportement qui est testé. Soit le nom n'est pas bon, soit le test ne test pas la bonne chose, mais ça ne va pas!

Question 6

@Test
public void givenOneElement_whenPopping_thenTheElementIsObtained() {
    Stack stack = new Stack();
    stack.push("bonjour");

    val elementObtained = stack.pop();

    assertEquals("bonjour", elementObtained);
}

@Test
public void givenTwoElements_whenPopping_thenTheSecondElementIsObtained() {
    Stack stack = new Stack();
    stack.push("bonjour");
    stack.push("salut");

    val elementObtained = stack.pop();

    assertEquals("salut", elementObtained);
}

☑️ Solution

Il est impossible que le deuxième test ait échoué s'il a été fait en TDD.

Le premier test est un sous-cas du deuxième, alors on enlèverait le premier test. Un nom plus générique tel que givenMultipleElements_whenPopping_thenTheLastAddedElementIsObtained

Il y a aussi potentiellement un problème de "magic string" dans ce test.

Note: Si le premier test ajoutait de la documentation pertinente, on pourrait décider de le garder. Ce n'est pas le cas ici par contre.

Question 7

@Test
public void givenMultipleElements_whenPopping_thenTheLastAddedElementIsObtained() {
    Stack stack = new Stack();
    for (int i = 0; i < 100; i++) {
        stack.push("Element " + i);
    }

    val elementObtained = pile.pop();

    assertEquals("Element 99", elementObtained);
}

☑️ Solution

De ma carrière, je n'ai jamais vu un cas de test unitaire où une boucle était nécessaire. Ça ne veut pas dire qu'il n'existe pas, mais je mettrai à jour ce solutionnaire si ça arrive.

Le problème ici est l'ajout de bruit qui n'est pas pertinent pour le test. Le cas "réduit" de deux éléments couvre exactement le même comportement sans ajouter ce bruit. Il n'y a aucune différence entre 2 et 100 éléments et ça n'ajoute aucun code différent.

Il y a encore le problème de "magic string", mais dans ce cas-ci la "magic number" 100 est encore pire, on ajoute une surcharge cognitive au lecteur: Pourquoi 100? Est-ce important?

Question 8

public function test_getData_shouldReturnExternalProviderAccessExpectedData()
{
    $user = $this->arrangeUser();
    $this->userService
        ->method('getUser')
        ->with(self::USER_ID)
        ->willReturn($user);

    $tokenExpiryTimestamp = new ExpiryTimestamp(self::TOKEN_UNIX_EXPIRY_TIMESTAMP);
    $externalProviderAccess = new ExternalProviderAccess(
        new EncryptedRefreshToken(self::TOKEN_VALUE, $tokenExpiryTimestamp),
        self::DATAGATEWAY_ID,
        self::WEBUSER_ID
    );

    $externalProviderAccess->id = self::ACCESS_ID;
    $externalProviderAccess->active = self::ACTIVE_INDICATOR;
    $externalProviderAccess->updatePhone = self::UPDATE_PHONE_INDICATOR;
    $externalProviderAccess->updateEmail = self::UPDATE_EMAIL_INDICATOR;
    $externalProviderAccess->updateAddress = self::UPDATE_ADDRESS_INDICATOR;
    $externalProviderAccess->updateContact = self::UPDATE_CONTACT_INDICATOR;
    $this->externalProviderAccessRepository
        ->method('findAccessByUserIdAndProviderId')
        ->with($user->Id, self::PROVIDER_ID)
        ->willReturn($externalProviderAccess);
    $this->externalProviderRepository
        ->method('getSystemName')
        ->with(self::PROVIDER_ID)
        ->willReturn(self::API_ENVIRONMENT);

    $actualData = $this->externalProviderView->getData(['id' => self::RECORD_SET_ID], []);

    $this->assertFalse($actualData['isNewAccess']);
    $this->assertEquals(self::SCREEN_NAME, $actualData['userScreenName']);
    $this->assertEquals(self::WEBUSER_ID, $actualData['userId']);
    $this->assertEquals(self::API_ENVIRONMENT, $actualData['apiEnvironment']);
    $this->assertEquals(self::DATAGATEWAY_ID, $actualData['dataGatewayId']);
    $this->assertEquals(
        self::TOKEN_UNIX_EXPIRY_TIMESTAMP,
        $actualData['refreshTokenUnixExpiryTimestamp']
    );

    $actualAccessConfigurations = $actualData['configurations'];
    $this->assertEquals($externalProviderAccess->active, $actualAccessConfigurations['active']);
    $this->assertEquals($externalProviderAccess->updateEmail, $actualAccessConfigurations['updateEmail']);
    $this->assertEquals($externalProviderAccess->updatePhone, $actualAccessConfigurations['updatePhone']);
    $this->assertEquals($externalProviderAccess->updateAddress, $actualAccessConfigurations['updateAddress']);
    $this->assertEquals($externalProviderAccess->updateContact, $actualAccessConfigurations['updateContact']);
}

☑️ Solution

BEAUCOUP DE CHOSES!!

Résumé en 1 phrase: ce test est impossible à lire. Le nom du test est simple, et semble indiquer quelque chose de relativement facile à tester. Quelle ligne de ce test execute le code de production?

En fait, nous verrons plus tard dans la session que le test "nous parle" et indique un problème de design sous-jacent.

Il y a plusieurs réponses possibles ici, voici un début de liste (incomplète donc) :

Manque de séparation claire entre le given, when, then
Plusieurs contantes n'ajoutent pas d'information, tel que self::UPDATE_ADDRESS_INDICATOR
Il y a trop d'asserts, ce test fait trop de choses
Le comportement testé ne semble pas celui décrit dans le nom
Plusieurs autres bonnes réponses

Question 9

[TestMethod]
public void GivenARoomThatWasJustVacated_WhenCleaningARoomByTheCrew_ThenShouldReceiveAnInvoiceForTheCleaningFeesIncludingTheTaxes()
{
    var invoice = room.clean()

    Assert.IsGreaterThanZero(invoice.GetTotal())
}

☑️ Solution

Le nom du test a beaucoup trop de bruit! Le fait qu'il soit long est peut-être un smell, mais n'est pas un problème en soit. Par contre, on ne doit pas mettre des détails non pertinents à la compréhension du test.

GivenARoomThatWasJustVacanted: Clairement, le fait que les personnes viennent de partir ou non n'a pas d'importance. La preuve: le test n'a pas de given!
WhenCleaningARoomByTheCrew: Juste WhenCleaning est suffisant, on n'a pas besoin de savoir par qui dans ce test
ThenShouldReceiveAnInvoiceForTheCleaningFeesIncludingTheTaxes: Juste ShouldCalculateTotalCleaningFees est suffisant, le fait de calculer les taxes est un concept de la facture (donc, dans InvoiceTest)

Question 10

@Test
public void shouldBeAbleToAddAPropertyToTheConfiguration() {
    Configuration configuration = new Configuration();

    configuration.addProperty(A_PROPERTY_NAME, A_PROPERTY_VALUE);

    assertThat(configuration.properties.get(A_PROPERTY_NAME)).isEqualTo(A_PROPERTY_VALUE)
}

☑️ Solution

Le test sait maintenant que, sous la configuration, les propriétés sont conservées dans une HashMap<K, V>. Il connait donc maintenant l'implémentation et n'est pas en blackbox. Qu'arrive-t-il si on désire utiliser autre chose qu'une HashMap? Le comportement reste identique, mais on doit changer le test. Puisque les tests valident un comportement, nous devrions pouvoir changer les détails d'implémentation sans changer ce test.

La solution est d'avoir une méthode qui permet de lire la configuration. Je vous entend dire "mais alors, on ajoute une méthode pour le test, et c'est mal!". La réalité est que "addProperty" n'a pas de comportement en soi (ça ne servirait à personne), alors c'est sur que le comportement implique de lire les valeurs. Un exemple serait :

@Test
public void shouldBeAbleToAddAPropertyToTheConfiguration() {
    Configuration configuration = new Configuration();

    configuration.addProperty(A_PROPERTY_NAME, A_PROPERTY_VALUE);

    String retrievedValue = configuration.readProperty(A_PROPERTY_NAME);
    assertThat(retrievedValue).isEqualTo(A_PROPERTY_VALUE)
}

Question 11

@Test
fun `should admit the patient to the emergency`() {
    val patient = givenAPatient()

    er.admit(patient)

    assertThat(er.rooms.first().beds.first().patient).isSameAs(patient)
}

☑️ Solution

Ce test connait l'implémentation. Il sait que l'urgence est composé de chambres et que celles-ci ont des lits. Qu'arrive-t-il si un jour l'urgence a des civière dans les corridors? Qu'arrive-t-il si un jour on décide de coder un algorithme qui fait que les patients ne sont pas forcément assignés à la première chambre?

Dans tous ces cas, si on change la façon de gérer les chambres/lits, le test va échouer même si le comportement du point de vu extérieur (admettre un patient) reste valide.

Question 12

it('should compute the invoice total', () => {
  expect(new InvoiceBuilder().withItem(new Item(20.0)).build().getTotal()).toBe(
    20.0
  )
})

☑️ Solution

Le test devrait clairement séparer le given/when/then.

it('should compute the invoice total', () => {
  // given
  const builder = new InvoiceBuilder()
  const item = new Item(20.0)

  // when
  const invoice = builder.withItem(item).build()

  // then
  expect(invoice.getTotal()).toBe(20.0)
})

On pourrait aussi dire que ce test n'est pas blackbox, mais c'est difficile à dire ici sans plus de context.

Question 13

public class PlaneTest {
    private static final int NUMBER_OF_SEATS = 36;

    private Plane plane;

    @Test
    public void whenAPassengerBooksTheLastSeatOnThePlane_thenThePlaneIsFull() {
        plane = new Plane(NUMBER_OF_SEATS);
        givenPassengersOnThePlane(NUMBER_OF_SEATS - 1);

        plane.book();

        assertTrue(plane.isFull());
    }

    @Test
    public void givenThePlaneIsNowFull_whenBooking_thenAnErrorIsThrown() {
        assertThatThrowBy(() -> plane.book())
            .isInstanceOf(PlaneIsFullException.class);
    }
}

☑️ Solution

Ces 2 tests sont interdépendants. Ceci rend le debuggage plus difficile, puisqu'un changement dans le premier test peut faire échouer le deuxième. De plus, jUnit ne garanti pas l'ordre d'execution des tests, alors celui-ci pourrait échouer de manière aléatoire.

Chaque test doit être isolé des autres.

Question 14

@Test
public void whenFiringAnEmployee_thenEmployeeIsFiredAndReceivesATerminationLetterThatContainsTheLastCheck() {
    Employee employee = new Employee();
    Money remainingVacations = employee.calculateRemainingVacations();

    employee.fire();

    assertTrue(employee.isFired());
    assertEquals("Termination letter", employee.getLastCommunication().getTitle());
    assertEquals(remainingVacations, employee.getLastPaycheck().getAmount());
}

☑️ Solution

Ce test en fait trop. Avoir plusieurs asserts n'est pas forcément un problème si ceux-ci constituent ensemble un seul "assert logique". Par contre, ici nous en avons plusieurs :

Valider que l'employé est bien renvoyé.
Valider qu'il a reçu sa lettre de fin d'emploi
Valider qu'il a reçu une paie avec son 4%

Bref, le test a 3 raisons d'échouer. On pourrait même en compter plus si on voit que le test connait également le fait que l'employé contient des communications et des paies.

La solution est de faire plusieurs tests, un pour chaque comportement.

précédent suivant