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

[16.0][FIX] l10n_br_cnpj_search: fix response assignment error in tests #3312

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

corredato
Copy link
Contributor

Corrige o erro nos testes do #3289

Não tenho certeza de como posso otimizar isso, mas resolve o problema.

@antoniospneto
Copy link
Contributor

antoniospneto commented Aug 28, 2024

@corredato eu não entendi bem como isso resolve os problemas lá..
Eu olhei só por cima, mas parece que os testes estão falhando por causa dos fixtures do vcr, eu particulamente acho desnecessario adicionar a complexidade da lib VCR para testar algo tão simples, minha sugestão é fazer um mock da resposta (request) utilizando apenas a lib mock nativa do python mesmo.. isso vai deixar tudo bem mais simples...

Veja que nem para testar as NFE a gente usa VC, é só o mock puro e dá super certo.

@corredato
Copy link
Contributor Author

@antoniospneto Dá uma olhada aqui: https://github.com/OCA/l10n-brazil/actions/runs/10595742717/job/29362165862?pr=3289#step:9:2558

Parece que quando o módulo foi migrado, não teve cobertura total dos testes, esse bloco acabou passando com problema

@antoniospneto
Copy link
Contributor

@antoniospneto Dá uma olhada aqui: https://github.com/OCA/l10n-brazil/actions/runs/10595742717/job/29362165862?pr=3289#step:9:2558

Parece que quando o módulo foi migrado, não teve cobertura total dos testes, esse bloco acabou passando com problema

Ah entenid, mas o erro ali é por outro motivo, veja meu comentário acima

@antoniospneto
Copy link
Contributor

trivial fix

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-3312-by-antoniospneto-bump-minor, awaiting test results.

@antoniospneto
Copy link
Contributor

@corredato

Esqueci de comentar mas a convenção da OCA diz para sempre adicionar o nome do módulo na mensagem do commit, exemplo
[FIX] l10n_br_cnpj_search: bla bla bla.

só pra ficar ligado para as próximas valeu,

@OCA-git-bot OCA-git-bot merged commit 7713445 into OCA:16.0 Aug 28, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at a6ad7f2. Thanks a lot for contributing to OCA. ❤️

@corredato
Copy link
Contributor Author

@corredato

Esqueci de comentar mas a convenção da OCA diz para sempre adicionar o nome do módulo na mensagem do commit, exemplo [FIX] l10n_br_cnpj_search: bla bla bla.

só pra ficar ligado para as próximas valeu,

Beleza, valeu pelo aviso

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