Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 23, 2024

Conversation

BertBR
Copy link
Contributor

@BertBR BertBR commented May 21, 2024

  • Coverage the whole session use case

Copy link

@iagxferreira iagxferreira left a comment

Choose a reason for hiding this comment

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

LGTM!

@vanflux
Copy link

vanflux commented May 21, 2024

@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
Copy link
Contributor Author

BertBR commented May 22, 2024

@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
Copy link

vanflux commented May 22, 2024

@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
Copy link
Contributor Author

BertBR commented May 22, 2024

@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
Copy link
Member

giggio commented May 23, 2024

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

Copy link
Member

@giggio giggio left a comment

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.

Copy link
Member

@giggio giggio left a comment

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
1 check passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants