add first test for app client#7
add first test for app client#7Maestro31 wants to merge 2 commits intodevcherchecollegue-org:mainfrom
Conversation
p1-dta
left a comment
p1-dta
left a comment
There was a problem hiding this comment.
Ma recommandation : ne pas faire d' import <module>, préférer from <module> import <>
|
|
||
| async def on_receive_event(self, event): | ||
| if isinstance(event, MessageReceivedEvent): | ||
| print("yolo") |
There was a problem hiding this comment.
add an ellipsis + comment about the expected behavior.
Maybe remove the print ?
| event_classes = [x for x in list( | ||
| map(lambda x: x.__name__, type(event).mro())) if x != 'object'] |
There was a problem hiding this comment.
x is not a good variable name at all.
remove ambiguity? :
| event_classes = [x for x in list( | |
| map(lambda x: x.__name__, type(event).mro())) if x != 'object'] | |
| event_classes = [x for x in list( | |
| map((lambda x: x.__name__), type(event).mro())) if x != 'object'] |
why map? :
| event_classes = [x for x in list( | |
| map(lambda x: x.__name__, type(event).mro())) if x != 'object'] | |
| event_classes = [x for x in | |
| [x.__name__ for x in type(event).mro()] | |
| if x != 'object'] |
Simpler solution:
| event_classes = [x for x in list( | |
| map(lambda x: x.__name__, type(event).mro())) if x != 'object'] | |
| event_classes = [x.__name__ for x in type(event).mro() | |
| if x.__name__ != 'object'] |
|
|
||
| subscribers = [] | ||
| for event_class in event_classes: | ||
| if not event_class in self._subscribers.keys(): |
There was a problem hiding this comment.
not x in y is not the "right" way.
x not in y is better.
and a key not in keys can be checked without .keys() if _subscribers is a dict:
| if not event_class in self._subscribers.keys(): | |
| if event_class not in self._subscribers: |
| subscribed_events = list( | ||
| map(lambda event_class: event_class.__name__, subscriber.subscribed_to())) |
There was a problem hiding this comment.
List comprehension, create a map only to cast is into a list is not pythonic.
| subscribed_events = list( | |
| map(lambda event_class: event_class.__name__, subscriber.subscribed_to())) | |
| subscribed_events = [event_class.__name__ for event_class in subscriber.subscribed_to()] |
| map(lambda event_class: event_class.__name__, subscriber.subscribed_to())) | ||
|
|
||
| for event_class_name in subscribed_events: | ||
| if not event_class_name in self._subscribers.keys(): |
There was a problem hiding this comment.
again here:
| if not event_class_name in self._subscribers.keys(): | |
| if event_class_name not in self._subscribers: |
|
|
||
| class EventLogger(): | ||
| def __init__(self): | ||
| pass |
There was a problem hiding this comment.
if __init__ will be filled with code in the future, prefer ellipsis to pass.
If not, then delete the __init__ method.
There was a problem hiding this comment.
@Vikka connait pas ellipsis, tu peux détailler ? Quelle différence ?
There was a problem hiding this comment.
Et si __init__ n'est pas implémenter, autant ne pas le mettre non ? on l'ajoutera quand on en aura besoin.
There was a problem hiding this comment.
Ellipsis est .... Pas le caractère utf8, juste '.'*3.
Ellipsis est généralement reconnu comme préférable comme marqueur "WIP", tandis que pass comme "nothing to do here". Mais ellipsis étant très peu connu / utilisé, il n'y a pas de concensus solide ni de norme. Cette sémantique de "required, but not set yet" est quelque chose que j'ai compris à force de lire et consulter des échanges sur Python, Pythonic et le fonctionnement de python.
There was a problem hiding this comment.
Et si
__init__n'est pas implémenter, autant ne pas le mettre non ? on l'ajoutera quand on en aura besoin.
C'est ce que je dis ici : "If not, then delete the init method.". Donc je plussoie.
Il n'y pas moyen de s'en sortir avec une classe abstraite du coup ? |
|
Hey @Vikka @titouanfreville, @Maestro31 avait fait ce PR juste pour illustrer une manière de faire les tests sans avoir de dépendances... prenez ça en considération s'il vous plait. Je crois pas que c'était attendu d'être merge tel quel |
4 participants
Voilà le premier test qui permet de tester en isolation l'application et vérifie que le client du bot est bien lancé lorsqu'on démarre l'application.
Procéder de cette manière permet d'avoir une classe concrète qui contient l'implémentation de discord.Client et une autre de test DiscordClientStub qui permet de s'en passer pour les tests et ne sert que de bouchon avec un avantage que l'on n'a pas besoin de connexion internet et une grande rapidité d'exécution des tests. La seule contrainte est bien évidemment que ces deux classes évoluent en miroir (le langage python n'étant pas statiquement typé).
Le but n'est pas de tester que le client du bot lance bien un bot sur le serveur à travers le réseau, car, ceci est garantie par la librairie, tester cette partie doit se faire dans des tests d'infra qui sont plus lents et ne sont pas voués à être lancés systématiquement pendant le développement.