Blog

No tengas miedo a refactorizar un test legacy en PHP

Wuola amiguis 👋🏻!

Llevo esta semana trabajando en un nuevo desarrollo, que me ha dado la oportunidad de poder enseñaros mis tips o consejitos 🤓 del día, esos que me ayudan a mejorar la legibilidad de los 🧪test en PHP que encuentro. Porque ya que paso por ahí, vamos a ir mejorando el código poquito a poquito.

PD: Aclaro que aunque se vea más de un test, en el que más me voy a centrar es en el primero, porque el principio que aplico es lo mismo en todos más o menos 🤪

Dejo por aquí cosas que veremos:

Plugins – Plugin - String Manipulation
Librerías – Librería de mocks PHP - Prophecy
Cosas del lenguaje PHP – Devolver varios valores en una función – file_get_contents() – sprintf()

COMENZAMOS (Música épica suena)

Refactor-time

Imaginemos que andas un día, como otro cualquiera super feliz con un desarrollo entre manos, no sé tu, pero yo me veo tal que así

Y de repente QUÉ ES ESO ?? UN TEST LEGACY HA APARECIDO

Yo siempre me imagino esta canción cuando digo eso.

🧪 El test legacy en cuestión

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103


public function testValidDataForClassicContractShouldReturnValidJson(): void
    {
        $contract = (new ContractMockBuilder(
            $this->prophesize(ClassicContract::class),
            $this->prophesize(Address::class)
        ))->build();

        $invoice = (new InvoiceMockBuilder($this->prophesize(Invoice::class)))->build();

        $billingRepository = (new BillingRepositoryMockBuilder(
            $this->prophesize(BillingRepositoryInterface::class)
        ))->build();

        $clientApiDto = (new ClientApiDtoMockBuilder($this->prophesize(ClientApiDto::class)))
            ->build();

        $clientCommunication = (new ClientCommunicationMockBuilder(
            $this->prophesize(ClientCommunicationInterface::class)
        ))
            ->shouldCallByEkonId(
                ContractMockBuilder::DEFAULT_CONTRACT_EKON_ID,
                $clientApiDto
            )->build();

        $userDto = (new UserDtoMockBuilder($this->prophesize(UserDto::class)))->build();

        $userCommunication = (new UserCommunicationMockBuilder(
            $this->prophesize(UserCommunicationInterface::class)
        ))
            ->shouldCallByUserId(
                ContractMockBuilder::DEFAULT_CLIENT_USER_ID,
                $userDto
            )->build();

        $applicationLogger = (new ApplicationLoggerMockBuilder(
            $this->prophesize(ApplicationLogger::class)
        ))
            ->build();

        $createInvoiceEmailDTO = new CreateInvoiceEmailDTO(
            $billingConceptRepository,
            $clientCommunication,
            $userCommunication,
            $applicationLogger
        );
        $invoiceEmailDTO = $createInvoiceEmailDTO->execute($contract, $invoice);
        $jsonSerializedInvoiceEmail = json_encode($invoiceEmailDTO);
        $this->assertJsonStringEqualsJsonString(
            '{"invoiceId":"2015G00000001","contractId":"2015000001","erpId":"' .
            ClientApiDtoMockBuilder::CLIENT_API_DTO . '","cups":"ES021701005100000PL","clientName":"' .
            ClientApiDtoMockBuilder::CLIENT_API_DTO_BUSINESS_NAME . '","clientPersonId":"4610000A","clientAddress":"' .
            'full address","holderName":"Paquito","holderPersonId":"46140000X",' .
            '"holderAddress":"Calle P\u00e0dua n\u00fam. 106, EN-1","contactName":"Paquito",' .
            '"contactEmails":["paquito@test.com"],"dueDate":"08\/11\/2015","dateStart":"1\/10\/2015",' .
            '"dateEnd":"31\/10\/2015","issueDate":"1\/11\/2015","totalAmount":"19.29","voidedInvoice":null,' .
            '"language":"ca","rateCode":"3.2","billedDays":"31","consumptionKwh":"187.36",' .
            '"consumptionCubicMeters":"16.00","conversionFactor":"11.710000",' .
            '"bankAccount":"ES8014650120341700537356","vatPercentage":"21.0","vatAmount":"3.35",' .
            '"invoiceConcepts":[],"readingRanges":[],' .
            '"comment":"\u00a1Gracias por ser como eres!",' .
            '"hash":"0a812fac182cda772c851c761db35ed3477573d3fe8d0f2694539bc651e87f9a",' .
            '"fileName":"0a812fac182cda772c851c761db35ed3477573d3fe8d0f2694539bc651e87f9a",' .
            '"taxableOtherCostsAmount":"0","nonTaxableOtherCostsAmount":"0","totalInvoice":"3.35","fee":"19.95",' .
            '"regularizationAmount": null,"tags":[], "product": "classic"}',
            $jsonSerializedInvoiceEmail
        );
    }


public function testValidInvoiceConceptShouldCreateInvoiceConceptsEmailDTO(): void
{
$invoiceConcept = (new InvoiceConceptMockBuilder($this->prophesize(InvoiceConcept::class)))
->build();

        $billingConceptRepository = (new BillingConceptRepositoryMockBuilder(
            $this->prophesize(BillingConceptRepositoryInterface::class)
        ))->build();

        $clientCommunication = (new ClientCommunicationMockBuilder(
            $this->prophesize(ClientCommunicationInterface::class)
        ))
            ->build();

        $userCommunication = (new UserCommunicationMockBuilder(
            $this->prophesize(UserCommunicationInterface::class)
        ))
            ->build();

        $applicationLogger = (new ApplicationLoggerMockBuilder(
            $this->prophesize(ApplicationLogger::class)
        ))
            ->build();

        $createInvoiceEmailDTO = new CreateInvoiceEmailDTO(
            $billingConceptRepository,
            $clientCommunication,
            $userCommunication,
            $applicationLogger
        );
        $invoiceCocnept = $createInvoiceEmailDTO->createInvoiceConceptDto($invoiceConcept);

        $this->assertNotEmpty($invoiceCocnept);
    }

Voy a empezar haciendo más legibles los test. Hay muchas veces que se suele poner el prefijo “test” para que PHPUnit los pueda leer, pero esto es bastante incómodo la verdad, por no hablar de cuando tenemos que leer algo super largo en camelCase. Por lo que utilizaremos la anotación @test que pa' eso está y pasaremos el enunciado a snake_case

Nos apoyaremos en el plugin string manipulation para cambiar el texto directamente a snake_case

💡 ProTIP: Shortcut en Mac ⇧⌥M

Aquí de paso, vemos que usan una librería de mock que se llama Prophecy hasta dentro de la cocina 🤯. Esto no es de por sí malo, pues hay que entender que lo mismo en su momento era la mejor manera en la que podían hacer los test, ahora mismo tenemos más cultura de ellos, pero lo mismo mañana todo cambia.

Dicho esto, simplemente nos limitaremos a mejorar lo que podamos.

GIVEN

Vemos que el bloque GIVEN es bastante complicado de identificar a simple vista, aparte de que se repite en los 2 test que vemos. Entonces, lo que podemos ir haciendo es extraerlo a métodos de inicialización que sean parte del GIVEN. Aquí identifico dos bloques, uno que necesita el groso de contratos y facturas, pero hay otro bloque que sólo se usa para construir el CreateInvoiceEmailDTO

code_blocks

Ahora el bloque nos queda mucho más simplificado, desestructurando el retorno de la función para devolvernos múltiples valores. Más info aqui

El test que ya nos trae una instancia de createInvoiceEmailDTO

code_1

Y el test que solo necesita los parámetros para crear createInvoiceEmailDTO

code_2

WHEN

Vamos simplemente a dejar claro donde está el when separándolo del given y del assert

assert_block

😮‍💨UFFF…. madre mía, eso ha sido tan duro de hacer que tengo que levantarme a desconectar 5 min.

two-hours-later

Bueno, seguimos al tajo para el siguiente y último bloque

ASSERT

asser_json_block

Para el momento estaba bien, pero es bastante complicado o incomodo de trabajar así un json, aunque usemos inject language , por lo que aquí he optado por usar la función de PHP file_get_contents, que nos ayuda a leer un fichero pasándolo a string, quedando finalmente tal que así:

assert_refactorized

Así mejor en mi opinión

flirt-millhouse

Si necesitamos inyectarle variables al JSON, podemos ayudarnos con la función sprintf poniendo en el fichero JSON %s en los valores que queremos sustituir:

json_y_sprintf

Si vemos el bloque del test, ahora creo que queda más clara la división del given, when y assert

final_result

Resultado final

🥸 Antes

Ver código antes del proceso de refactorización

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103


public function testValidDataForClassicContractShouldReturnValidJson(): void
    {
        $contract = (new ContractMockBuilder(
            $this->prophesize(ClassicContract::class),
            $this->prophesize(Address::class)
        ))->build();

        $invoice = (new InvoiceMockBuilder($this->prophesize(Invoice::class)))->build();

        $billingRepository = (new BillingRepositoryMockBuilder(
            $this->prophesize(BillingRepositoryInterface::class)
        ))->build();

        $clientApiDto = (new ClientApiDtoMockBuilder($this->prophesize(ClientApiDto::class)))
            ->build();

        $clientCommunication = (new ClientCommunicationMockBuilder(
            $this->prophesize(ClientCommunicationInterface::class)
        ))
            ->shouldCallByEkonId(
                ContractMockBuilder::DEFAULT_CONTRACT_EKON_ID,
                $clientApiDto
            )->build();

        $userDto = (new UserDtoMockBuilder($this->prophesize(UserDto::class)))->build();

        $userCommunication = (new UserCommunicationMockBuilder(
            $this->prophesize(UserCommunicationInterface::class)
        ))
            ->shouldCallByUserId(
                ContractMockBuilder::DEFAULT_CLIENT_USER_ID,
                $userDto
            )->build();

        $applicationLogger = (new ApplicationLoggerMockBuilder(
            $this->prophesize(ApplicationLogger::class)
        ))
            ->build();

        $createInvoiceEmailDTO = new CreateInvoiceEmailDTO(
            $billingConceptRepository,
            $clientCommunication,
            $userCommunication,
            $applicationLogger
        );
        $invoiceEmailDTO = $createInvoiceEmailDTO->execute($contract, $invoice);
        $jsonSerializedInvoiceEmail = json_encode($invoiceEmailDTO);
        $this->assertJsonStringEqualsJsonString(
            '{"invoiceId":"2015G00000001","contractId":"2015000001","erpId":"' .
            ClientApiDtoMockBuilder::CLIENT_API_DTO . '","cups":"ES021701005100000PL","clientName":"' .
            ClientApiDtoMockBuilder::CLIENT_API_DTO_BUSINESS_NAME . '","clientPersonId":"4610000A","clientAddress":"' .
            'full address","holderName":"Paquito","holderPersonId":"46140000X",' .
            '"holderAddress":"Calle P\u00e0dua n\u00fam. 106, EN-1","contactName":"Paquito",' .
            '"contactEmails":["paquito@test.com"],"dueDate":"08\/11\/2015","dateStart":"1\/10\/2015",' .
            '"dateEnd":"31\/10\/2015","issueDate":"1\/11\/2015","totalAmount":"19.29","voidedInvoice":null,' .
            '"language":"ca","rateCode":"3.2","billedDays":"31","consumptionKwh":"187.36",' .
            '"consumptionCubicMeters":"16.00","conversionFactor":"11.710000",' .
            '"bankAccount":"ES8014650120341700537356","vatPercentage":"21.0","vatAmount":"3.35",' .
            '"invoiceConcepts":[],"readingRanges":[],' .
            '"comment":"\u00a1Gracias por ser como eres!",' .
            '"hash":"0a812fac182cda772c851c761db35ed3477573d3fe8d0f2694539bc651e87f9a",' .
            '"fileName":"0a812fac182cda772c851c761db35ed3477573d3fe8d0f2694539bc651e87f9a",' .
            '"taxableOtherCostsAmount":"0","nonTaxableOtherCostsAmount":"0","totalInvoice":"3.35","fee":"19.95",' .
            '"regularizationAmount": null,"tags":[], "product": "classic"}',
            $jsonSerializedInvoiceEmail
        );
    }


   public function testValidInvoiceConceptShouldCreateInvoiceConceptsEmailDTO(): void
    {
        $invoiceConcept = (new InvoiceConceptMockBuilder($this->prophesize(InvoiceConcept::class)))
            ->build();

        $billingConceptRepository = (new BillingConceptRepositoryMockBuilder(
            $this->prophesize(BillingConceptRepositoryInterface::class)
        ))->build();

        $clientCommunication = (new ClientCommunicationMockBuilder(
            $this->prophesize(ClientCommunicationInterface::class)
        ))
            ->build();

        $userCommunication = (new UserCommunicationMockBuilder(
            $this->prophesize(UserCommunicationInterface::class)
        ))
            ->build();

        $applicationLogger = (new ApplicationLoggerMockBuilder(
            $this->prophesize(ApplicationLogger::class)
        ))
            ->build();

        $createInvoiceEmailDTO = new CreateInvoiceEmailDTO(
            $billingConceptRepository,
            $clientCommunication,
            $userCommunication,
            $applicationLogger
        );
        $invoiceCocnept = $createInvoiceEmailDTO->createInvoiceConceptDto($invoiceConcept);

        $this->assertNotEmpty($invoiceCocnept);
    }

🤩 Después

Ver código después del proceso de refactorización

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14


    /** @test */
    public function valid_data_for_classic_contract_should_return_valid_json(): void
    {
        [$contract, $invoice, $createInvoiceEmailDTO] = $this->initializeSceneryTo(self::CLASSIC_CONTRACT);

        $invoiceEmailDTO = $createInvoiceEmailDTO->execute($contract, $invoice);

        $jsonExpected = file_get_contents('tests/unit/[...]/JSON/emailDtoFromClassicContract.json');
        $jsonSerializedInvoiceEmail = json_encode($invoiceEmailDTO);
        $this->assertJsonStringEqualsJsonString(
            sprintf($jsonExpected, ClientApiDtoMockBuilder::CLIENT_API_DTO, ClientApiDtoMockBuilder::CLIENT_API_DTO_BUSINESS_NAME),
            $jsonSerializedInvoiceEmail
        );
    }

Conclusión

Creo que el $this-> de este lenguaje es lo que peor llevo, pero igualmente se muestra bastante flexible para poder refactorizar un test, tenga las líneas que tenga. Y me gustaría hacer especial énfasis en que el código perfecto no existe, pero sí que podemos hacer que tenga menos imperfecciones, dándole amor cuando más cultura de mejorarlos tenemos 🥳.

Y si has llegado hasta aquí, espero que hayas disfrutado del camino y de la lectura 👏🏼 Espero debates de cosas que harían ustedes para mejorarlo incluso .

Publicado el 26/09/2023 por

Rubén Zamora

https://rubenzagon.com

¿Quieres más? te invitamos a suscribirte a nuestro boletín para avisarte cada vez que recopilemos contenido de calidad que compartir.

Si disfrutas leyendo nuestro blog, ¿imaginas lo divertido que sería trabajar con nosotros? ¿te gustaría?

Puedes contratarnos si necesitáis externalizar un desarrollo
O quizas os hace falta que echemos una mano con algún proyecto.
También tienes las puertas abiertas para unirte a nuestro equipo