Open andriacap opened 1 month ago
camillemonchicourt
commented
1 month ago Oui en effet, c'est un soucis qu'on a rencontré plusieurs. J'ai l'impression qu'il n'a pas toujours été là, qu'il a été là, puis réglé puis revenu, mais je ne sais plus trop. C'est possible que cela soit là depuis longtemps et jamais solutionné, même si ça m'étonne que ça ne soit alors pas remonté plus tôt.
Dans tous les cas, tant mieux si c'est résolu durablement. :-)
andriacap
commented
1 month ago Ce qui est possible c'est que le bug soit apparu en ajoutant l'event "DRAWSTOP" lié à cette PR https://github.com/PnX-SI/GeoNature/pull/2779 . Mon hyptohèse c 'est que l'ensemble des cas d'usage appelant cet event n'a pas été pris en compte dans l'état final de la PR mergé . C'est pourquoi il y avait ces conditions imbriquées dans ce commit initial de la proposition de PR : https://github.com/PnX-SI/GeoNature/commit/10e905ca741f0d078cf9d2a05bd532ba89cf85f3.
Et en même temps l'issue mentionnait un autre bug et la PR suffisait à résoudre l'issue https://github.com/PnX-SI/GeoNature/pull/2779 .
Un peu complexe à tester tous les cas en effet et à vérifier qu'il n'y a pas d'autres bugs qui apparaissent ^^.
A terme des tests unitaire frontend permettraient de diminuer le risque d'apparition de ce genre de bugs .
camillemonchicourt
commented
1 month ago OK alors possible d'ajouter des tests Frontend dans cette PR pour ne pas recasser cette fonctionnalité dans le futur ?
andriacap
commented
1 month ago Je pense qu'il faudrait faire un socle de base solide de configuration de tests frontend unitaire avant de le faire ponctuellement sur une PR .
Parce qu'en l'état la majorité des PR proposés coté frontend ne propose pas de tests unitaires. Donc soit c'est imposé à toutes les PR avec des checks qui ne passent pas etc soit la PR est testée par les personnes qui review (ce qui est le cas actuellement) pour vérifier qu'il n'y ait pas de bugs introduits.
Dans l'idéal, dans le manuel de développeur on aurait un truc du genre : "Si des propositions de développement frontend sont réalisés, s'appuyer sur cet exemple de tests unitaires pour pouvoir proposer une PR 'mergeable' . Avec une présentation des configurations à utiliser etc " .
Après ce n'est que mon avis et je suis dispo et à l'écoute des personnes qui souhaitent qu'on mette en place ce genre de tests :)
camillemonchicourt
commented
1 month ago Oui ce serait l'idéal, mais on n'en a pas actuellement la capacité ni les ressources. On a mis en place une première série de tests frontend de base pour avoir un début et un cadre, en espérant les compléter plus tard, globalement ou au fur et à mesure, avec les contributions successives. En se concentrant surtout sur les parties fragiles/sensibles. Et là il semble que cela en soit une et qu'elle ait cassé récemment.
On passe beaucoup beaucoup de temps pour les autres à relire et tester les PR, sans parler des releases.
Donc si les contributions externes peuvent aider et apporter même un petit test de plus, surtout quand c'est un truc qui a cassé plusieurs fois et a certainement été recassé par une autre PR récemment, c'est bienvenue.
Mais en effet pour les tests frontend, on ne peut pas l'imposer. Donc libre à chacun.
La seule chose qu'on a pu définir comme pré-requis est qu'une nouvelle PR ne fasse pas diminuer le taux de couverture de tests backend sur lesquels on s'est concentré jusqu'à présent.
andriacap
commented
1 month ago Oui j'entend bien que c'est difficile en terme de capacité et de ressources . Et dans mon message je ne dit pas que la relecture ou les tests ne sont pas fait loin de là .
On a mis en place une première série de tests frontend de base pour avoir un début et un cadre, en espérant les compléter plus tard, globalement ou au fur et à mesure, avec les contributions successives. En se concentrant surtout sur les parties fragiles/sensibles
Ca se trouve à quel endroit ? Pour quelles parties du code ? Est ce qu'il y a un exemple sur lequel s'appuyer ? Je ne parle pas des tests frontend end to end qui concernent l'import pour lesquels on a participé. Je parle de tests unitaires .
Peut être que Jacques ou un autre développeur l'a déjà implémenté quelque part et je serais bien évidemment partant pour en parler avec la personne qui l'a mis en place .
camillemonchicourt
commented
1 month ago Ah oui, là dans le détail je ne maitrise pas assez le sujet. Je pensais à des tests dans la continuité de ceux existant, à priori end to end, ici : https://github.com/PnX-SI/GeoNature/tree/master/frontend/cypress
Version Version de GeoNature affectée par le bug --> toutes les versions depuis que l'utilisation du composant
leaflet-draw.component.tsest utiliséDescription du bug
Le bug concerne une duplication de géométrie en cours d'édition (que ce soit à la modification d'une géométrie existante ou en cours de création).
Voir captures d'écran ci dessous (cliquez pour déplier) :
 Comportement attendu
Le comportement attendu est le suivant : lorsqu'on décide de modifier une géométrie existante ou que l'on décide de modifier une nouvelle géométrie en cours d'édition il faut que la géométrie précédente ne soit pas prise en compte.
Comment reproduire Étapes à suivre pour reproduire le problème :
leaflet-draw.component.tsest appelé .Historique de modifications du composant
leaflet-draw.component.tsL'évènement "DRAWSTOP" n'était pas pris en compte jusqu'à l'intégration de cette PR : https://github.com/PnX-SI/GeoNature/pull/2779.
Le commit suivant https://github.com/PnX-SI/GeoNature/commit/10e905ca741f0d078cf9d2a05bd532ba89cf85f3 présent dans l'historique de la PR prenait en compte la gestion de ce bug. Après review de la PR , une partie du code proposé a été retiré .
L'idée étant donc de réintégrer ce qui avait été proposé sur le commit mentionné ci dessus. Une PR va être ouverte dans la foulée permettant de reprendre ce bout de code retiré et ledit code sera aussi refact pour une meilleur lisibilité .
Merci d'avance pour votre lecture