Correção de bug em calculo de fretes

[nichmorgan]

O cálculo do frete (calculate_freights) estava enviando o type de pacote da classe e não o tipo de pacote de envio.

[gfabrizio]

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

correios/tests/test_client.py

Line 439 in 419e24b

def test_calculate_freights(client, posting_card, package):

deveriam quebrar, pode verificar esses testes?

Aguardando os tests

[nichmorgan]

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.

[gfabrizio]

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.

[memachado]

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.

[nichmorgan]

Exatamente @parannoide, eu mesmo estou usando meu fork, pq não sei mais como aprovar aqui, talvez inexperiência minha.

[erikhenrique]

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

[memachado]

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

[silviolleite]

Aguardando fix relacionados aos testes