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

tests: ensure jest test are running #35

Closed
wants to merge 9 commits into from

Conversation

Luc-cpl
Copy link

@Luc-cpl Luc-cpl commented May 10, 2024

This pull request resolves failing Jest tests by fixing configurations and addressing unresolved dependencies, enabling thorough testing of the codebase.

Copy link

@renanshin renanshin left a comment

Choose a reason for hiding this comment

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

Reviewed and ok

@raryson
Copy link

raryson commented May 10, 2024

Testando aqui, tive uns problemas com windows e to tentando replicar no ubuntu

@raryson
Copy link

raryson commented May 10, 2024

Testei aqui e ta funcionando. Eu que tinha pego a branch de dev e tava testando errado.
Uploading image.png…
Só falhou o de end to end por quê não to com a aplicação rodando (eu acho).

Copy link
Contributor

@AlbuquerqueRafael AlbuquerqueRafael left a comment

Choose a reason for hiding this comment

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

Obrigado pelo pull-request @Luc-cpl


describe('AppController (e2e)', () => {
describe('Supplies (e2e)', () => {
Copy link
Contributor

@AlbuquerqueRafael AlbuquerqueRafael May 10, 2024

Choose a reason for hiding this comment

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

Poderias remover esse teste? Ele vai falhar a depender da configuração do banco.

Uma outra alternativa era criar um ' health check' endpoint e mudar esse teste para apontar para o mesmo . Assim continuariamos tendo um exemplo de teste 'e2e' funcionando.

Copy link
Author

Choose a reason for hiding this comment

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

Sim, esse endpoint está ai só para manter o exemplo para os testes e2e para quem for usar.
Não gostaria de remover enquanto não temos outros testes nessa área (para manter o exemplo).

O correto na realidade seria o teste não falhar por conta do DB.
Não sei ao certo como é essa parte com o Prisma (nunca usei ele), mas acredito que o ideal seria a suite de testes inicializar um DB limpo e "resetar" as tabelas a cada teste para ter algo mais previsível.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oi @Luc-cpl. No momento esse 'test' está falhando para qualquer desenvolvedor que tenha alguma instancia local rodando com dados.

No futuro podemos vir a consertar isso. Todavia, no momento o seu PR não conserta todas as falhas como está proposto no PR: 'tests: ensure jest test are running
'.

Daí a sugestão para:

  • Ou remover esse test. Ou adicionar um 'health-check' para ter um test e2e rodando.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, daqui a pouco faço a alteração.

Copy link
Author

Choose a reason for hiding this comment

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

@AlbuquerqueRafael
Como pode ver, mantive o teste mas removi a checagem da resposta para evitar o erro (com um aviso de @todo para fazer essa parte futuramente, assim é feita apenas a verificação da resposta 200.

Copy link
Contributor

@filipepacheco filipepacheco left a comment

Choose a reason for hiding this comment

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

  1. Aqui os testes falharam também no Supplies (e2e).
    Aquele mesmo erro referente à porta da URL.

  2. Typo em
    suplies.spec.ts
    o correto é:
    supplies.spec.ts

  3. Corrigir o conflito na package.json

@Luc-cpl
Copy link
Author

Luc-cpl commented May 10, 2024

@filipepacheco

Corrigidos os itens 2 e 3.
O teste e2e removi a verificação do conteúdo da resposta, mas deveria responder corretamente.

Copy link

@filipefraga filipefraga left a comment

Choose a reason for hiding this comment

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

falta mockar o banco apenas

This was referenced May 12, 2024
@Luc-cpl
Copy link
Author

Luc-cpl commented May 15, 2024

Desnecessário após merge do #78

@Luc-cpl Luc-cpl closed this May 15, 2024
@Luc-cpl Luc-cpl deleted the tests/working-jest-base branch May 15, 2024 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting-approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants