SOS-RS / backend

Auxílio RS: Projetos de Resposta a Emergências por Chuvas e Alagamentos
https://sos-rs.com
MIT License
706 stars 306 forks source link

test: cover the whole session controler and service use cases #159

Closed BertBR closed 1 month ago

BertBR commented 2 months ago
vanflux commented 2 months ago

@BertBR Poderia explicar a utilidade de testar esse código da controller e service dessa forma? Posso estar enganado, mas como não tem nenhuma regra de negócio sendo checada, não parece fazer sentido.

Se simplesmente fizermos mais um include no prisma -> vamos ter que alterar o código de teste. Se alterarmos a forma de desativar o token pra usar o user.id ao invés do user.login -> vamos ter que alterar o código de teste.

Parece que esse teste ta checando se o código de hoje é exatamente igual ao código de hoje sem nenhuma liberdade pra nada. Acho que faz mais sentido fazer testes de integração pra isso ai, um teste que rode a api, insira dados no banco, chame as rotas e veja se o retorno ta correto. Se alterarmos de prisma pra sequelize, se alterarmos a ordem da invalidação das sessions, se fizermos qualquer coisa que faça o comportamento ser o mesmo e mude o código o teste ainda passa se tiver tudo certo.

BertBR commented 2 months ago

@BertBR Poderia explicar a utilidade de testar esse código da controller e service dessa forma? Posso estar enganado, mas como não tem nenhuma regra de negócio sendo checada, não parece fazer sentido.

Se simplesmente fizermos mais um include no prisma -> vamos ter que alterar o código de teste. Se alterarmos a forma de desativar o token pra usar o user.id ao invés do user.login -> vamos ter que alterar o código de teste.

Parece que esse teste ta checando se o código de hoje é exatamente igual ao código de hoje sem nenhuma liberdade pra nada. Acho que faz mais sentido fazer testes de integração pra isso ai, um teste que rode a api, insira dados no banco, chame as rotas e veja se o retorno ta correto. Se alterarmos de prisma pra sequelize, se alterarmos a ordem da invalidação das sessions, se fizermos qualquer coisa que faça o comportamento ser o mesmo e mude o código o teste ainda passa se tiver tudo certo.

O objetivo é simples, meu caro: Garantir que o que está em produção, funcionando hoje e agregando valor continue funcionando. E sim, o objetivo de testes unitários é exatamente esse, se houver alteração, tem que mapear o comportamento novo em um teste, o que é facilmente feito em menos de 5 minutos por quem estiver implementando a nova feature, por exemplo.

Em relação aos testes de integração, concordo que abrangem um escopo maior e devem sim ser priorizados em MVP ou inicio de projeto de qualquer dimensão. Esse será meu próximo passo para contribuições aqui, auto gerar testes de integração através de chamadas na api com keploy

vanflux commented 2 months ago

@BertBR Poderia explicar a utilidade de testar esse código da controller e service dessa forma? Posso estar enganado, mas como não tem nenhuma regra de negócio sendo checada, não parece fazer sentido. Se simplesmente fizermos mais um include no prisma -> vamos ter que alterar o código de teste. Se alterarmos a forma de desativar o token pra usar o user.id ao invés do user.login -> vamos ter que alterar o código de teste. Parece que esse teste ta checando se o código de hoje é exatamente igual ao código de hoje sem nenhuma liberdade pra nada. Acho que faz mais sentido fazer testes de integração pra isso ai, um teste que rode a api, insira dados no banco, chame as rotas e veja se o retorno ta correto. Se alterarmos de prisma pra sequelize, se alterarmos a ordem da invalidação das sessions, se fizermos qualquer coisa que faça o comportamento ser o mesmo e mude o código o teste ainda passa se tiver tudo certo.

O objetivo é simples, meu caro: Garantir que o que está em produção, funcionando hoje e agregando valor continue funcionando. E sim, o objetivo de testes unitários é exatamente esse, se houver alteração, tem que mapear o comportamento novo em um teste, o que é facilmente feito em menos de 5 minutos por quem estiver implementando a nova feature, por exemplo.

Em relação aos testes de integração, concordo que abrangem um escopo maior e devem sim ser priorizados em MVP ou inicio de projeto de qualquer dimensão. Esse será meu próximo passo para contribuições aqui, auto gerar testes de integração através de chamadas na api com keploy

TLDR; Entendo que você quer garantir que o sistema está funcionando corretamente (e eu também quero), mas esse tipo de teste unitário parece ser overkill pra atingir esse propósito. Parece que ele está testando se o código de hoje é exatamente igual ao código de hoje e não apenas validando o comportamento. Na minha visão esse teste não deveria ser sensível a esse ponto: mudei a implementação 1 vírgula e mesmo correto o teste começa a falhar e tenho que reescrever o teste de cada método que alterei 1 vírgula. Se for assim melhor ver se a HASH de cada método é igual a anterior hahaha (brincadeira).

Resumo: Acho que esses métodos praticamente sem regra de negócio deveriam ser testados aplicando um seed no db, chamando a api e verificando a resposta porque testar unitariamente esses métodos é só pra sobrecarregar o dev.

Obs: Não estou falando que testes unitários são inúteis, mas acho que este é um uso ruim pra eles. Se tu fosse fazer um função de por exemplo: valida cpf, parse date, format date, sanitiza sei lá oq, convert sei lá oq pra sei lá oq 2... -> isso faz sentido, porque não tem essas dependências do prisma, de db, de coisas desse tipo que precisam de um mock sem nexo.

BertBR commented 2 months ago

@BertBR Poderia explicar a utilidade de testar esse código da controller e service dessa forma? Posso estar enganado, mas como não tem nenhuma regra de negócio sendo checada, não parece fazer sentido. Se simplesmente fizermos mais um include no prisma -> vamos ter que alterar o código de teste. Se alterarmos a forma de desativar o token pra usar o user.id ao invés do user.login -> vamos ter que alterar o código de teste. Parece que esse teste ta checando se o código de hoje é exatamente igual ao código de hoje sem nenhuma liberdade pra nada. Acho que faz mais sentido fazer testes de integração pra isso ai, um teste que rode a api, insira dados no banco, chame as rotas e veja se o retorno ta correto. Se alterarmos de prisma pra sequelize, se alterarmos a ordem da invalidação das sessions, se fizermos qualquer coisa que faça o comportamento ser o mesmo e mude o código o teste ainda passa se tiver tudo certo.

O objetivo é simples, meu caro: Garantir que o que está em produção, funcionando hoje e agregando valor continue funcionando. E sim, o objetivo de testes unitários é exatamente esse, se houver alteração, tem que mapear o comportamento novo em um teste, o que é facilmente feito em menos de 5 minutos por quem estiver implementando a nova feature, por exemplo. Em relação aos testes de integração, concordo que abrangem um escopo maior e devem sim ser priorizados em MVP ou inicio de projeto de qualquer dimensão. Esse será meu próximo passo para contribuições aqui, auto gerar testes de integração através de chamadas na api com keploy

TLDR; Entendo que você quer garantir que o sistema está funcionando corretamente (e eu também quero), mas esse tipo de teste unitário parece ser overkill pra atingir esse propósito. Parece que ele está testando se o código de hoje é exatamente igual ao código de hoje e não apenas validando o comportamento. Na minha visão esse teste não deveria ser sensível a esse ponto: mudei a implementação 1 vírgula e mesmo correto o teste começa a falhar e tenho que reescrever o teste de cada método que alterei 1 vírgula. Se for assim melhor ver se a HASH de cada método é igual a anterior hahaha (brincadeira).

Resumo: Acho que esses métodos praticamente sem regra de negócio deveriam ser testados aplicando um seed no db, chamando a api e verificando a resposta porque testar unitariamente esses métodos é só pra sobrecarregar o dev.

Obs: Não estou falando que testes unitários são inúteis, mas acho que este é um uso ruim pra eles. Se tu fosse fazer um função de por exemplo: valida cpf, parse date, format date, sanitiza sei lá oq, convert sei lá oq pra sei lá oq 2... -> isso faz sentido, porque não tem essas dependências do prisma, de db, de coisas desse tipo que precisam de um mock sem nexo.

Mano, se o teste está sensível a implementações então o problema não é o teste mas sim a arquitetura do projeto...Mas enfim, acho que essa discussão não vai levar a nenhum resultado. Seu ponto ja ficou claro pra mim, vou fechar a PR pra evitar mais ruídos

giggio commented 1 month ago

@BertBR os testes trazem valor, sim, vou revisá-los. Principalmente o teste do serviço, que tem um caminho específico e é um serviço extremamente importante.