Skip to content

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

Merged
giggio merged 2 commits into
SOS-RS:developfrom
BertBR:test/sessions
May 23, 2024
Merged

test: cover the whole session controler and service use cases#159
giggio merged 2 commits into
SOS-RS:developfrom
BertBR:test/sessions

Conversation

@BertBR

@BertBR BertBR commented May 21, 2024

Copy link
Copy Markdown
Contributor
  • Coverage the whole session use case

@iagxferreira iagxferreira left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vanflux

vanflux commented May 21, 2024

Copy link
Copy Markdown

@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

BertBR commented May 22, 2024

Copy link
Copy Markdown
Contributor Author

@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

vanflux commented May 22, 2024

Copy link
Copy Markdown

@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

BertBR commented May 22, 2024

Copy link
Copy Markdown
Contributor Author

@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

giggio commented May 23, 2024

Copy link
Copy Markdown
Member

@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.

@giggio giggio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Estava com logs aparecendo, subi uma alteração pra escondê-los. Bora integrar.

@giggio giggio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Estava com logs aparecendo, subi uma alteração pra escondê-los. Bora integrar.

@giggio
giggio merged commit 38cccef into SOS-RS:develop May 23, 2024
@BertBR
BertBR deleted the test/sessions branch May 23, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment