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: add builder to test #1409

Closed
wants to merge 2 commits into from

Conversation

CarlosZiegler
Copy link

Ola @aprendendofelipe e pessoal :)
Estava lendo essa issue: #1405 e resolvi aplicar um pattern Builder para os testes, isso seria uma prova de conceito, caso seja aprovada implementacao, ai podemos refatorar e implementar em alguns testes.

Qualquer dica sera bem vinda.

PS: Esse primeiro seria um rabisco, normalmente eu uso Typescript nos meus projetos entao teria que ver qual o shape que precisamos no builder e remodelar ele caso necessario.

@vercel
Copy link

vercel bot commented May 9, 2023

@CarlosZiegler is attempting to deploy a commit to the TabNews Team on Vercel.

A member of the Team first needs to authorize it.

@aprendendofelipe
Copy link
Collaborator

Fala @CarlosZiegler, obrigado pelo PR! 💪

Eu gostei da ideia. Principalmente porque pode deixar a leitura dos testes mais fácil para quem for contribuir 👍

@CarlosZiegler
Copy link
Author

Opa desculpe a demora muita coisa por se fazer aqui haha, assim que possivel implemento algo a mais :)

@aprendendofelipe aprendendofelipe marked this pull request as draft July 14, 2023 16:21
@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 17, 2023

Dei uma olhada nesse PR para tentar entender e ajudar a progredir. Essa foi a primeira vez em que li os testes (tanto os "originais" quanto os propostos no PR). Não tenho certeza se essa alteração deixou a leitura dos testes mais fácil para mim.

Por exemplo, a forma de testar com Object.keys pode acabar enganando quem chama o método, pensando que está comparando objeto contra objeto (toEqual) ao invés de comparando os campos declarados ( toMatchObject, que poderia ser usado no lugar do laço com Object.keys).

Outro ponto que me dificultou um pouco o entendimento também foi a comparação strict. Me fiz algumas perguntas como:

  • Quando eu deveria usar strict = true? Encontrei a menção ao toStrictEqual em outro PR (Cobre de testes todo header da requisição #643), mas me parece que o toEqual atenderia o cenário descrito lá, então continuei com a dúvida.
  • Por que precisa do JSON.parse e JSON.stringify? Cheguei a investigar mas também não cheguei numa conclusão.

Até o ponto em que está, tive a impressão que essa abstração não ajudou muito, e com certeza tem pontos que não estou enxergando, por isso deixei minhas dúvidas aqui.

@Rafatcb Rafatcb added the testes Criação ou melhoria de testes label Dec 17, 2023
@Rafatcb Rafatcb added the pendente Aguardando ação do autor do Pull Request label Dec 27, 2023
@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 31, 2023

Bom, como o PR está há 7 meses parado, está com alguns conflitos, e meu comentário ficou sem respostas, estou fechando o PR para organizar o repositório. Caso alguém queira dar continuidade, sugiro criar uma nova branch a partir da main, para evitar conflitos, já levando em consideração o que comentei ao revisar o código. Lembrando que o issue #1405 continua aberto para ser resolvido.

Obrigado pela contribuição, @CarlosZiegler!

@CarlosZiegler
Copy link
Author

Bom dia @Rafatcb , primeiramente agradeço o review. Acabei recebendo as notificações na minha lixeira :( então desculpa a demora na resposta. Sobre a diferença de strictEqual é equal acredito que strict não compara apenas o conteúdo mas o tipo , por exemplo json x class . Mas sim condoned o que poderia ser usado o match, no momento me atentei a deixar o teste mais próximo do original.

Sobre o PR estar 7 meses parados, isso foi por conta de outras tarefas do meu dia a dia, acabei não dando a prioridade nisso, infelizmente.

Novamente obrigado pelo review clareou algumas ideias aqui.

@Rafatcb
Copy link
Collaborator

Rafatcb commented Dec 31, 2023

então desculpa a demora na resposta. (...) Sobre o PR estar 7 meses parados, isso foi por conta de outras tarefas do meu dia a dia, acabei não dando a prioridade nisso, infelizmente.

Sem problemas, @CarlosZiegler! O objetivo não foi realizar uma cobrança sua, apenas organizar o repositório mesmo. Mesmo que você não possa abrir um novo PR, quem for contribuir poderá se inspirar no seu código 👍

Mais uma vez, obrigado por dedicar seu tempo contribuindo aqui.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pendente Aguardando ação do autor do Pull Request testes Criação ou melhoria de testes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants