Produits Entr'ouvert » Corbo

• quand tu as ça:
  

      assert resp.location == 'http://testserver/manage/'
      resp = app.get('http://testserver/manage/')
  

  
  tu peux remplacer la deuxième ligne par
  

      resp = resp.follow()
  

  
  c'est plus clair je trouve, ça suit plus le cheminement d'un navigo.
• URL invalide with override_settings(SMS_GATEWAY_URL='http:/passerelle.com')
• inutile assert send_form.method 'post'
• sauf si l'URL à une importance (pour une API par exemple), assert resp.location 'http://testserver/manage/announce/1/' c'est mieux de faire ce test avec reverse je pense (ou alors on dit que toutes nos URLs doivent être immuables), anecdotique.
• with override_settings(SMS_GATEWAY_URL='http:/passerelle.com'): avec pytest-django on utilise directement la fixture settings et on modifie directement la valeur du setting dessus

• je ne vois pas de test que l'action du formulaire à procéder à un appel du backend de SMS (pas de mock rien, bizarre que ça ne pète pas une erreur)
• même remarque dans form_valid on dirait que send_sms() ne peut jamais foirer, send_sms() devrait retourner un résultat genre return True, None ou return False, error ou alors émettre une exception (je suis de moins en moins fan des exceptions)
• Send test {{ action }} c'est quoi action ? J'ai la vague impression que c'est mal nommé ou inutile (ce serait "SMS"?) Tout le jeu d'héritage avec corbo/send_test_announce_form.html" me semble inutile mais je rate peut-être un truc, si c'est juste pour pas mettre {{ form.as_p }} ne nous prions pas de nous répéter ici.

• Dans les tests, je ne serais pas contre quelques lignes vides (respiration après une série d'action+assert) et quelques commentaires, pour mieux comprendre ce qui se passe.
• settings.SMS_GATEWAY_URL='http:/passerelle.com' manque d'espace pèpuite. *

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

• sauf si l'URL à une importance (pour une API par exemple), assert resp.location == 'http://testserver/manage/announce/1/' c'est mieux de faire ce test avec reverse je pense (ou alors on dit que toutes nos URLs doivent être immuables), anecdotique.

je préfère ici que les tests "detectent" quand les urls changent.

Comme dit Benj, seules les URL d'API ne doivent pas bouger ; mais les autres on s'en fiche, donc ici ça peut être du reverse (mais c'est anecdotique, comme dit Benjamin)

• même remarque dans form_valid on dirait que send_sms() ne peut jamais foirer, send_sms() devrait retourner un résultat genre return True, None ou return False, error ou alors émettre une exception (je suis de moins en moins fan des exceptions)

send_sms comme pour l'envoi des mails retourne le nombre de destinations auxquelles le message a été délivré. S'il y a exception elle est logguée et la fonction retourne 0.
La vue d'envoi du SMS de test devrait au moins vérifier qu'il y a eu envoi.

Dans ce cas, ça serait plus lisible de faire

sms_sent = utils.send_sms(announce.text, [mobile])
if sms_sent == 1:
    ...
else:
    ...

dans le SMSAnnounceView::form_valid

Mais c'est quand même très dommage de ne pas avoir le message d'erreur, qui ne sera qu'au fond des logs (genre, quota SMS dépassé, message trop long, etc). Le retour des fonctions send_xxx pourraient être amélioré dans cet objectif (autre ticket à faire).

Une ligne vide ajoutée dans corbo/templates/corbo/email_test_announce_form.html pour rien, je pense. Tu la retires et c'est un ack :)