correios
correios copied to clipboard
olist
Correção de bug em calculo de fretes
O cálculo do frete (calculate_freights) estava enviando o type de pacote da classe e não o tipo de pacote de envio.
Olá, obrigado pela contribuição. Por favor, você pode acrescentar teste dessa modificação? em tempo, me aprece que com essa modificação alguns testes como esse https://github.com/olist/correios/blob/419e24b658ed6cda36ee2928b601f222e4df0218/tests/test_client.py#L439 deveriam quebrar, pode verificar esses testes?
Olá, realizei os testes, os únicos quebrados não tem relação alguma com a classe de testes client. Como poderia exportar os testes para vocês? É o meu primeiro pull request.
Olá @nichmorgan, tudo bem?
você pode escrever os testes da modificação feita no seu pr em algum dos nossos arquivos de teste, e.g: https://github.com/olist/correios/blob/419e24b658ed6cda36ee2928b601f222e4df0218/tests/test_client.py também pode executar os testes com o comando make test.
Para o pr ser aprovado e feito seu merge você deve escrever testes específicos para a sua mod e 100% dos testes devem passar.
Corrijam-me se eu estiver errado, principalmente por ser novato em python,
Mas essa PR passa em todos os testes já existentes (acabei de rodar), então ela deveria ser aprovada, nao?
Além disso, a modificação sugeriada é para corrigir um possível bug, que inclusive está acontecendo comigo.
@nichmorgan Achou outra saida para o erro que encontrou, ou apenas aplicou a modificação localmente? Ou ainda nem era erro, e achou o meio certo de utilizar?
Abraços.
Exatamente @parannoide, eu mesmo estou usando meu fork, pq não sei mais como aprovar aqui, talvez inexperiência minha.
@nichmorgan e @parannoide gostaria de dar meus 2 cents, mesmo não sendo o mantenedor do repositório. Antes de tudo, sua contribuição é realmente muito boa para a comunidade open source, obrigado!
Pelo que checkei na doc dos correios, na página 4/16 essa parte do código realmente está errada e faz sentido a alteração.
Sobre os testes. Uma coisa que precisamos ter claro é que nem sempre que recebemos sucesso da suit de testes quando alteramos algo, quer dizer que a nossa alteração está correta. Algo esperado em qualquer repositório de código que tenha testes é que quando alteramos uma parte do código, o esperado é que os testes quebrem. Se isso não acontece, algo está errado com a nossa cobertura de testes.
Pelo que pude olhar aqui, encontrei alguns problemas:
- Não existe nenhum teste unitário para validar exatamente essa chamada aos correios com o parâmetro correto de package.
- O teste para a chamada dos correios está utilizando vcr, isso faz com que tanto a chamada e resposta com os correios sejam mockadas. Mitigando um pouco o erro. Então uma chamada aos correios passando o parâmetro de tipo de pacote como 1 ou 2 não está fazendo diferença para os asserts que realizamos baseado em preço e prazo.
A partir disso, acredito que seja interessante adicionar uma maior cobertura de testes para o primeiro ponto e atualizar o teste com vcr para que realize uma nova chamada aos correios.
Como eu sei que essa parte de testes com Correios não é algo trivial e intuitivo eu tomei a liberdade de subir um diff da atualização que fiz caso queira seguir como exemplo. Acabei colocando usuário e senha do ambiente de teste público dos Corrreios sem mascarar. Atualmente no projeto é mascarado com XXX ou outros número, eu não vejo problema nisso em deixar público e facilitar futuras alterações, porém, só os mantenedores poderão opiniar melhor.
@nichmorgan De qualquer forma, sua sugestão era a correção que eu precisava no momento. Muito obrigado.
@erikhenrique Obrigado pela constribuição, dependendo da minha disponibilidade nos próximos dias tentarei seguir seu exemplo.