Point stabilité, mise à jour des dépendance & qualité de code

Développement
1

Salut à tous !

Les deux gros développements du moment: Multi-utilisateurs & présence bluetooth dans Gladys 4 venant d’être mergée, je pense qu’il est temps de faire une petite pause niveau fonctionnalité et d’investir un peu de temps sur la stabilité, les mise à jour majeures des dépendances et la qualité du code.

Je pense qu’il est sain de faire ça régulièrement pour être sur que le projet continue d’être d’aussi bonne qualité qu’au début !

Dépendances:

  • J’en parlais hier, il devient critique de revoir le process de build du frontend, encore bloqué sous preact-cli@2.0.0: Mise à jour du build du front (preact-cli@2 actuellement)

  • Au niveau DB, on est sous Sequelize 4 actuellement, alors que Sequelize 6.5.0 est sorti. Je pense qu’il y a beaucoup de petites optimisations/bugfix qui ont du sortir entre ces deux versions, et ça pourrait faire un grand bien au projet de mettre à jour !

  • On est toujours en Node 12.xx, alors que Node 14 est la version LTS actuelle la plus récente.

  • Côté frontend, il va falloir retirer momentjs qui est super lourd, et deprecated ! datejs que l’on utilise dans le frontend fait tout autant le travail, en plus léger.

  • vis-data et vis-network prend une place folle pour pas grand chose, ça vaut vraiment le coup de le retirer.

  • unistore, la librarie de state managment qu’on utilise, a été mise à jour et je pense qu’il faudrait vraiment y passer ! D’ailleurs, de manière générale, je pense qu’il faut qu’on arrête de l’utiliser pour les données de composants, et se restreindre au variable globale (httpClient, user, c’est à peut près tout :stuck_out_tongue: ). On a pas mal de petit bug qui sont du au fait qu’on utilise ce store global pour stocker des états locaux au composants. C’est moi qui avait mis ça en place, à tord :slight_smile:

Qu’en pensez-vous ?

1 « J'aime »
2

Il y a une confusion entre frontend et backend, ou momentjs ET datejs sont utilisés en parallèle dans le frontend ?

Prends une folle ? C’est à dire :sweat_smile:

Pour le reste, je suis tout à fait d’accord, même si je peux pas encore donner un coup de main dessus… La stabilité c’est super important et une réécriture est souvent nécessaire, tu l’as montré régulièrement en 4 versions de Gladys …! Chapeau :slight_smile:

3

momentjs et datejs sont utilisé côté frontend. En fait on a de la chance, momentjs n’est utilisé qu’à un seul endroit: le calendrier :slight_smile: C’est de ma faute, j’ai laissé ça passé dans la review de la PR, mais clairement il faut se débarrasser de moment.

prendre une *place folle :slight_smile: Pardon j’ai loupé un mot !

Top!

Effectivement, je pense que dans tout projet informatique il faut avoir l’habitude de dédier au moins 20% du temps à de la réduction de dette technique. ça permet d’avoir un projet à jour, maintenable, et clean :slight_smile:

1 « J'aime »
4

@AlexTrovato @cicoub13 @VonOx Je ne sais pas si vous avez vu mon post, vous en pensez quoi ?

5

preact-cli@3

Oui, mais je pense que les changements Front majeurs sont difficiles aujourd’hui car 0 tests. Même remarque pour unistore un peu plus bas.

Est-ce que l’idée, c’est de réinitialiser un projet vide avec la version 3 et de copier le code Gladys dans ce nouveau projet à jour ?

Sequelize 6.5.0

Si on garde SQLite, go pour cette version. Je peux aider

Node 14

Je vois déjà la MR et ça devrait être assez facile

retirer momentjs pour remplacer par datejs

Yes, pareil : pas mal de tests manuels à faire côté Front

vis-data et vis-network

Je ne sais pas à quoi ça sert, je pense qu’on peut le virer.

unistore

Tu voudrais utiliser quoi à la place pour le store Front ?

6

@pierre-gilles je pense en effet que c’est nécessaire, et que c’est même quelque chose qu’il faut faire “au jour le jour” et éviter d’attendre 6 mois pour le faire.

Comme le dit @cicoub13 il y a pas de sujets côté front. On devrait peut-être passer du temps sur des tests du front avant migrations / changements histoire de s’assurer d’une certaines campagnes de non régression, même si cela ne remplacera pas les tests faits par un humain.
On en avait déjà parlé, et si je me souviens bien, on avait mentionné https://www.cypress.io/

Je pense qu’on va devoir y passer.

Le top serait donc qu’on se dispatch les tâches, et rien n’empêche de commencer les migrations sans les tests cypress.

Qui prend quoi ? (je prends nodejs v14 :stuck_out_tongue: )
Perso moyen chaud pour la DB,
mais je veux bien me lancer sur cypress, au moins histoire de poser les bases et introduire les tests sur le “core” (il ne faudra pas être pressé, c’est une découverte pour moi) .

1 « J'aime »
7

Allez y moi je vous regarde :sweat_smile:

Moment ça doit être dans mes cordes

EDIT: Le component react ne supporte pas dayjs :confused:

3 « J'aime »
8

Et autre sujet mais assez lié, nous avons des dépendances avec des failles de sécurité de niveau high (12 front et 4 back). Je sais que la plupart des installations ne sont pas ouvertes sur le web mais je pense que c’est mieux de mettre à jour ces dépendances.

Je pense que ça vaut le coup d’activer DependaBot : Enabling and disabling version updates - GitHub Docs

1 « J'aime »
9

Lol tu trouves tjs une excuse pour ne pas faire !! :stuck_out_tongue:

1 « J'aime »
10

10000% pour !
Automatiser à minima la recherche de vulns dans le code et limite le configurer en bloquant sur les PR c’est une bonne idée pour s’assurer d’un niveau de sécurité minimum.

11

Il faut les tests avant je pense

12

Je suis d’accord pour la maintenance générale, c’est au jour le jour :slight_smile: (version de node, upgrade mineurs/patch des dépendances, qualité de code).

Après pour les changements majeures qui n’apportent rien à l’utilisateur/au projet, je ne suis pas forcément pour la course au dépendances.

On a quand même des moyens très limité sur le projet, et franchement si tu suis absolument toutes les dépendances en mettant à jour dès qu’une nouvelle sort (je ne parle pas des mises à jour de sécurité), bah tu ne développe plus.

Sequelize/tabler/etc… ils ont des rythmes de mise à jour breaking très supérieur à ce qu’on serait capable d’absorber au quotidien. Pour ça que pour ces mises à jour, il faut faire le pour et le contre et évaluer si les pour sont supérieur au temps qu’on pourrait passer sur d’autres fonctionnalités « interne » à Gladys… :slight_smile:

Je suis d’accord!

Je suis chaud pour qu’on se mette à Cypress :slight_smile:

Je pense que personne ici n’a jamais fais de Cypress, donc limite il faudrait avancer ensemble. Si tu veux regarder pour faire un POC, et ensuite on factorisera tous ensemble ? Je peux regarder aussi de mon côté :slight_smile:

ah merde? C’est sur à 100% ? On pourrait soit proposer une PR, soit trouver un hack pour que ça marche… sinon globalize ou date-fns c’est moins pire que moment

J’ai vu, c’est aussi le but de ce point stabilité :slight_smile:

J’ai déjà dependabot activé sur le repo pour la partie « notifications », après pour la partie « PR automatisée » je suis plutôt contre.

Sur un projet de l’échelle de Gladys, ça fait des dizaines/centaines de PR par jour, ça remplit complètement les PRs et le CI, et en fait c’est juste impossible d’aller aussi vite que lui :stuck_out_tongue:

ça je suis pour :slight_smile: il faut mettre un npm audit dans la CI et bloquer uniquement sur les failles « high/critical » :slight_smile:

13

Ok j’ai déjà commencé le POC pour cypress, je pense que je vais faire quelques pages (login, dashboard, inscription) pour voir les différents aspects, et l’intégration ci github action certainement.
Je vais tenter de tenir un rapport pour les questions ouvertes et autres points de difficulté rencontrés.
Dès que j’ai un petit quelque chose, je partagerai pour qu’on en discute.

3 « J'aime »
14

Bonsoir à tous, si certains veulent jouer ou commenter la mise en place de cypress :

https://github.com/GladysAssistant/Gladys/pull/1086

La doc (qui évolue avec l’expérimentation) :

Et la je joue avec les actions Github pour automatiser la chose, c’est plutot pas mal, mais biensur, ça va rallonger le temps de build :

2 « J'aime »
15

Salut @AlexTrovato ! Super boulot, je mets ici mes réponses à tes questions ouvertes :

  1. Choix du nommage des fichiers de tests :
  2. je pense qu’il ne faut pas nommer tous nos fichier index.js mais tenter de découpage les cas fonctionnels par fichier
  3. 1 fichier par cas ? (login_success.js / login_error.js)

Un fichier par cas ça me semble énorme, tu trouves pas ? Côté serveur on fait un fichier de test par fichier dans Gladys, en général un fichier par fonction. Et ensuite dans le fichier on fait un cas par cas à tester, ça me parait plus clean, sinon ça va faire beaucoup de fichiers non ?

  1. snake_case vs camelCase ?

Côté front c’est du CamelCase avec la première lettre en majuscule, je pense qu’il faut garder la même règle pour les fichiers de tests.

  1. Démarrer le front en développement ou production ?

J’ai pas l’expérience de Cypress pour savoir quelle est la best practice la dessus ^^

  1. Démarrer également le serveur ? Ou mocker les réponses
  2. Si on démarre le serveur, on ne peut pas tester les services / intégrations
  3. Si on mock les réponses, plus de travail, maintenance plus lourde

Bonne question ça. Pour moi l’avantage de mocker ça serait effectivement pour les services, après l’inconvénient c’est que potentiellement si une route d’API change dans le code backend, et que les mocks sont pas mise à jour, bah les tests du front vont passer alors qu’en fait dans la vrai vie ça pète. C’est moins end-to-end quoi

Peut-être qu’une approche « hybride » serait possible ?

On mock tout ce qui est services, et l’API de Gladys core (login, etc…) n’est pas mockée comme ça on est certain que les routes critiques (login, forgot password, signup) fonctionnent bien en E2E.

  1. Comment détecter que le front est prêt pour démarrer les tests automatiquement ?

Là dessus j’ai pas l’expérience de Cypress, il va falloir enquêter ^^

  1. Comment gérer “proprement” la langue du navigateur ?

Il doit y avoir un moyen dans cypress non?

16

Salut,

je ne connais pas vraiment cypress, mais si le but est de faire des test E2E il faut effectivement laisser les api derrières puisque le principe est de tester l’assemblage des differentes briques.
Si le but est de faire des test plus unitaire coté front, alors il vaut mieux tout mocker même l’api core

je pense qu’il faut les 2 type de tests, les E2E qui permettent effectivement de vérifier que toutes les briques fonctionnent correctement entre elles, et les test unitaires qui permettent de savoir si un composant a un problème ou non.

Pour les tests unitaire, c’est qu’en gros on vérifie le comportement du composant selon tous les cas possible.
Ca implique de tester les props qu’on lui passe si un composant attend un prop number on test avec un number puis un string puis un null, etc… pour vérifier que ca gère bien tous les cas,
si il y a des events on vérifie que ceux ci soit bien appelé, soit correctement appelé (un click event doit être appelé qu’une fois, etc…), etc…
on test aussi les changement de states lorsque nécessaire (en gros un bouton qui aurai un state loading il faut vérifier que lorsque le state loading est actif le bouton soit dans la bonne représentation, il a bien la classe loading par exemple, etc…)

Ce sont les test les plus chiant a faire, sur des petit composant ca va vite, des qu’il s’agit d’un composant plus complexe c’est le bordel ^^
il semble y avoir plusieurs possibilité pour gérer ces tests, dans la doc preact il parle d’enzyme ou de preact testing library qui permet de faire les test unitaire de façon plus simple mais bon ^^ ( Unit Testing with Enzyme – Preact Guide ou Testing with Preact Testing Library – Preact Guide)

17

Je me suis occupé de supprimer moment du front. J’ai changé par dayjs fallait charger en plus les 2 plugins localizedFormat et localeData, l’API étant assez proche de moment, ça fonctionne bien. Si sur le long terme on voit des problèmes apparaitre j’ai vu qu’on utilisait date-fns dans ScheduledTriger qui est aussi compatible react-big-calendar on pourra se rediriger là dessus.

https://github.com/GladysAssistant/Gladys/pull/1092

3 « J'aime »
18
  1. Démarrer également le serveur ? Ou mocker les réponses
  2. Si on démarre le serveur, on ne peut pas tester les services / intégrations
  3. Si on mock les réponses, plus de travail, maintenance plus lourde

Je te suis pour tester l’intégration sur le core et mocker les services (au moins pour les cas en succés), on peut facilement mixer les 2.
Je vais compléter la PR actuelle pour fournir un exemple des 2 cas.
Du coup, je pense que ça permet de tester en mode « production », afin de valider la cible, en mode « e2e », sauf pour les services.

  1. Comment détecter que le front est prêt pour démarrer les tests automatiquement ?

Github actions sait le faire, il y a l’option dans la PR CI.

  1. Comment gérer “proprement” la langue du navigateur ?

Je pense que ma question est plutôt : doit-on faire des tests sur les libellés ? ça risque d’être assez lourd. Mais pour valider les messages d’erreur selon les cas, ça peut être utile.
Sinon ce sera par variable / config d’exécution.

19

Ok, bonne idée !

Nice :slight_smile:

ça dépend des cas je dirais. Effectivement pour les erreurs ça pourrait être bien. Surtout pour certaines pages « critiques » (login, signup) ou il faut vraiment bétonner.

Sinon, pas la peine, ça risque d’être redondant et lourd :slight_smile:

1 « J'aime »
20

@bertrandda je viens de remarquer que le service Caldav utilise moment côté backend, ça serait cool de changer aussi ici :slight_smile: