Utilitaire de tests simplifié

lstephan commented

2020-05-11 00:24:21 +02:00

(Migrated from git.eleves.ens.fr)

On simplifie l'utilitaire de tests utilisé partout (sauf pour kfet).

moins de fourre-tout: un mixin par fonctionnalité spéciale
meilleure doc
quelques simplifications

On simplifie l'utilitaire de tests utilisé partout (sauf pour `kfet`). * moins de fourre-tout: un mixin par fonctionnalité spéciale * meilleure doc * quelques simplifications

lstephan commented

2020-05-11 00:30:13 +02:00

(Migrated from git.eleves.ens.fr)

@mpepin j'ai mis le tag discussion pour répondre au débat suivant : quelle est notre position en ce qui concerne les mixins divers et variés dans les utilitaires de test ?

Personnellement, si c'est bien documenté et relativement indépendant des autres tests (i.e., pas besoin de connaitre par coeur le code des mixins pour écrire des tests), je suis pour ; je sais que tu es plus mesuré là dessus donc j'aimerais bien ton avis sur les mixins dans cette MR :slight_smile:

@mpepin j'ai mis le tag `discussion` pour répondre au débat suivant : quelle est notre position en ce qui concerne les mixins divers et variés dans les utilitaires de test ? Personnellement, si c'est bien documenté et relativement indépendant des autres tests (i.e., pas besoin de connaitre par coeur le code des mixins pour écrire des tests), je suis pour ; je sais que tu es plus mesuré là dessus donc j'aimerais bien ton avis sur les mixins dans cette MR :slight_smile:

lstephan commented

2020-05-11 01:17:39 +02:00

(Migrated from git.eleves.ens.fr)

added 1 commit

65171d12 - Fix event tests

Compare with previous version

added 1 commit <ul><li>65171d12 - Fix event tests</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1618&start_sha=3b43ad84b572dc51dd1d7e1826e00c3e5698836d)

lstephan commented

2020-05-11 12:54:48 +02:00

(Migrated from git.eleves.ens.fr)

added 1 commit

b8ba7ec4 - Fix tests for python3.7 (?)

Compare with previous version

added 1 commit <ul><li>b8ba7ec4 - Fix tests for python3.7 (?)</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1619&start_sha=65171d1276484bfb6c707be1b9f73e9491697203)

lstephan commented

2020-05-11 13:31:17 +02:00

(Migrated from git.eleves.ens.fr)

added 1 commit

50266f24 - Fix tests for python3.7 (?)

Compare with previous version

added 1 commit <ul><li>50266f24 - Fix tests for python3.7 (?)</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1620&start_sha=b8ba7ec4af5a8c6ad225ec3cb0bbf1365fee6ced)

mpepin commented

2020-05-11 20:58:59 +02:00

(Migrated from git.eleves.ens.fr)

je sais que tu es plus mesuré là dessus

C'est vrai ! 😉

En gros pour préciser un peu ma "règle" : pour moi, idéalement on ne devrait pas avoir à lire de la doc pour comprendre un test. Mais du coup un helper qui fait un seul truc et qui a un nom clair qui permet de deviner en gros ce qu'il fait, je suis pour.

Typiquement :

ton assertCalEqual pour moi c'est l'exemple parfait de ce qu'il faut faire : juste en lisant le nom de la méthode ce sais ce que ça fait. Conséquences :
- quand je lis un test qui l'utilise, je sais ce qui se passe sans avoir à réfléchir et je peux me concentrer sur le test plutôt que sur comprendre les outils
- quand j'écris un test, je sais tout de suite si j'en ai besoin ou pas, je peux deviner en gros comment m'en servir (les détails étant dans la docstring) et en plus je suis content de ne pas avoir à l'implémenter moi même !
en revanche le ViewTestCaseMixin.test_forbidden par exemple c'est le contraire : sans lire le code / une docstring de 20 kilomètres je ne sais pas ce que ça vérifie ni comment m'en servir.

Évidemment ce n'est que mon avis et il est sûrement biaisé par le fait que j'ai lu plus de tests que je n'en ai écrits

Franchement si on n'avait que des helpers de la forme assertCalEqual je serais favorable à tout mettre dans un gros mixin 👍

> je sais que tu es plus mesuré là dessus C'est vrai ! :wink: En gros pour préciser un peu ma "règle" : pour moi, idéalement on ne devrait pas avoir à lire de la doc pour comprendre un test. Mais du coup un helper qui fait **un** seul truc et qui a un nom clair qui permet de deviner en gros ce qu'il fait, je suis pour. Typiquement : - ton `assertCalEqual` pour moi c'est l'exemple parfait de ce qu'il **faut** faire : juste en lisant le nom de la méthode ce sais ce que ça fait. Conséquences : - quand je lis un test qui l'utilise, je sais ce qui se passe sans avoir à réfléchir et je peux me concentrer sur le test plutôt que sur comprendre les outils - quand j'écris un test, je sais tout de suite si j'en ai besoin ou pas, je peux deviner en gros comment m'en servir (les détails étant dans la docstring) et en plus je suis content de ne pas avoir à l'implémenter moi même ! - en revanche le `ViewTestCaseMixin.test_forbidden` par exemple c'est le contraire : sans lire le code / une docstring de 20 kilomètres je ne sais pas ce que ça vérifie ni comment m'en servir. Évidemment ce n'est que mon avis et il est sûrement biaisé par le fait que j'ai lu plus de tests que je n'en ai écrits Franchement si on n'avait que des helpers de la forme `assertCalEqual` je serais favorable à tout mettre dans un gros mixin :+1:

mpepin commented

2020-05-11 21:13:48 +02:00

(Migrated from git.eleves.ens.fr)

Je ne connaissais pas le DictReader, c'est pratique !

Je verrais bien ici un assertCsvEqual(content, expected) qui détecte toute seul s'il faut utiliser csv.reader ou csv.DictReader en fonction du type de expected et qui fait la comparaison direct. T'en penses quoi ?

Ça ne règle pas le souci du test non-déterministe sur les events mais d'un autre côté ça ne me paraît ok de rendre la fonction d'export CSV plus déterministe pour régler le problème.

Je ne connaissais pas le `DictReader`, c'est pratique ! Je verrais bien ici un `assertCsvEqual(content, expected)` qui détecte toute seul s'il faut utiliser `csv.reader` ou `csv.DictReader` en fonction du type de expected et qui fait la comparaison direct. T'en penses quoi ? Ça ne règle pas le souci du test non-déterministe sur les events mais d'un autre côté ça ne me paraît ok de rendre la fonction d'export CSV plus déterministe pour régler le problème.

mpepin commented

2020-05-11 21:25:12 +02:00

(Migrated from git.eleves.ens.fr)

Il manque les self.assertListEqual(remaining/unexpected, []) je pense

Il manque les `self.assertListEqual(remaining/unexpected, [])` je pense

mpepin commented

2020-05-11 21:36:29 +02:00

(Migrated from git.eleves.ens.fr)

Vu qu'on n'utilise pas l'événement plus bas dans le test, je simplifierais en :

def _find_cal_event(self, ev, l):
    for i, elt in enumerate(l):
        if self._test_event_equal(ev, elt):
            return i
    return None

mais c'est un détail

Vu qu'on n'utilise pas l'événement plus bas dans le test, je simplifierais en : ```python def _find_cal_event(self, ev, l): for i, elt in enumerate(l): if self._test_event_equal(ev, elt): return i return None ``` mais c'est un détail

mpepin commented

2020-05-11 21:37:57 +02:00

(Migrated from git.eleves.ens.fr)

On n'utilise jamais la feature avec le getter si? On ne pourrait pas les virer ? Ça simplifie drastiquement.

mpepin commented

2020-05-11 21:43:58 +02:00

(Migrated from git.eleves.ens.fr)

C'est dommage d'avoir à faire tout ça… Ça te va si je patch l'export des inscriptions pour qu'il soit plus déterministe ?

lstephan commented

2020-05-11 21:56:24 +02:00

(Migrated from git.eleves.ens.fr)

En fait, il y a deux types de mixins (c'est dommage qu'ils soient un peu mélangés, d'ailleurs) :

ceux qui t'aident à écrire des tests (du genre assertCalEqual), et qui ont l'air de t'aller 👍
ceux qui font des tests "basiques" automatisés, du genre test_forbidden ou test_urls: je comprends que tu sois plus dubitatif là dessus, mais ça évite quand même bcp de répétition de code en échange...

En fait, il y a deux types de mixins (c'est dommage qu'ils soient un peu mélangés, d'ailleurs) : - ceux qui t'aident à écrire des tests (du genre `assertCalEqual`), et qui ont l'air de t'aller :thumbsup: - ceux qui font des tests "basiques" automatisés, du genre `test_forbidden` ou `test_urls`: je comprends que tu sois plus dubitatif là dessus, mais ça évite quand même bcp de répétition de code en échange...

mpepin commented

2020-05-11 22:08:45 +02:00

(Migrated from git.eleves.ens.fr)

mais ça évite quand même bcp de répétition de code en échange...

Justement mon point de vue c'est que c'est parfaitement acceptable de faire des copier-coller dans des tests si ça permet de les rendre self-contained. Mais j'entends que c'est peut-être relou quand t'en écris plein

> mais ça évite quand même bcp de répétition de code en échange... Justement mon point de vue c'est que c'est parfaitement acceptable de faire des copier-coller dans des tests si ça permet de les rendre self-contained. Mais j'entends que c'est peut-être relou quand t'en écris plein

lstephan commented

2020-05-12 00:25:40 +02:00

(Migrated from git.eleves.ens.fr)

C'est aussi une possibilité, oui !

lstephan commented

2020-05-12 00:26:02 +02:00

(Migrated from git.eleves.ens.fr)

On l'utilise pour la date il me semble...

lstephan commented

2020-05-12 00:29:55 +02:00

(Migrated from git.eleves.ens.fr)

Des avis parmi nos (futurs) devs ? @champeno @areitz @thubrecht, au pif =)

lstephan commented

2020-05-12 01:13:38 +02:00

(Migrated from git.eleves.ens.fr)

Fait et branché partout, sauf dans le test avec les problèmes d'ordering plus bas =)

lstephan commented

2020-05-12 01:13:42 +02:00

(Migrated from git.eleves.ens.fr)

Oops, fixed

lstephan commented

2020-05-12 01:13:44 +02:00

(Migrated from git.eleves.ens.fr)

Done !

lstephan commented

2020-05-12 01:14:20 +02:00

(Migrated from git.eleves.ens.fr)

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1622&start_sha=50266f2466d72cd175e771c8a3ddd1754c3e15cd#2ad0175b779ca05808e870cb16c58163e8643201_71_86)

lstephan commented

2020-05-12 01:14:31 +02:00

(Migrated from git.eleves.ens.fr)

added 2 commits

9b044042 - Fix ical tests
6fff995c - Expand CSVResponseMixin functionality

Compare with previous version

added 2 commits <ul><li>9b044042 - Fix ical tests</li><li>6fff995c - Expand CSVResponseMixin functionality</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1622&start_sha=50266f2466d72cd175e771c8a3ddd1754c3e15cd)

mpepin commented

2020-05-14 21:24:12 +02:00

(Migrated from git.eleves.ens.fr)

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1635&start_sha=6fff995ccdcc66210a63850cedba585e0d75532b#8aa96677bc804e0169ce84dba74cec930dfa2d39_146_143)

mpepin commented

2020-05-14 21:24:13 +02:00

(Migrated from git.eleves.ens.fr)

added 1 commit

707b7b76 - Make events tests deterministic

Compare with previous version

added 1 commit <ul><li>707b7b76 - Make events tests deterministic</li></ul> [Compare with previous version](/klub-dev-ens/gestioCOF/merge_requests/421/diffs?diff_id=1635&start_sha=6fff995ccdcc66210a63850cedba585e0d75532b)

mpepin commented

2020-05-14 21:28:08 +02:00

(Migrated from git.eleves.ens.fr)

ah j'avais mal lu, laissons donc

mpepin commented

2020-05-14 21:29:07 +02:00

(Migrated from git.eleves.ens.fr)

On peut avoir cette conversation avec tout le monde, ça serait intéressant, mais on ne va rien ajouter à cette MR non ?

mpepin commented

2020-05-14 21:29:07 +02:00

(Migrated from git.eleves.ens.fr)

resolved all discussions