Add end-to-end tests to Bottin #2

Merged
erwan merged 3 commits from test-go-Bottin into main 2021-07-22 07:59:11 +00:00
Member

Rajout d'un test automatique sous drone.yml qui teste:

  • requête ADD
  • requête Search
  • requête Modify
  • requête Delete
  • ajout de personnes dans un groupe puis suppression de celui-ci -> regarder si le groupe a bien disparu dans membersOf

Nécessite correction d'un bogue d'encodage ASN.1 dans goldap utilisé par ldapserver

Rajout d'un test automatique sous drone.yml qui teste: - requête ADD - requête Search - requête Modify - requête Delete - ajout de personnes dans un groupe puis suppression de celui-ci -> regarder si le groupe a bien disparu dans membersOf Nécessite correction d'un bogue d'encodage ASN.1 dans goldap utilisé par ldapserver
erwan changed title from WIP:test-go-Bottin to test-go-Bottin 2021-07-09 13:44:24 +00:00
erwan changed title from test-go-Bottin to WIP:test-go-Bottin 2021-07-09 13:46:53 +00:00
erwan changed title from WIP:test-go-Bottin to test-go-Bottin 2021-07-09 14:59:10 +00:00
Owner

Salut Erwan, il faudrait faire un squash de tes commits pour pas avoir plein de commits intermédiaires. Ca signifie réécrire l'historique du dépôt pour remplacer tes 21 commits par moins de commits, où chacun des nouveau commits est la "somme" de plusieurs des commits précédents. Idéalement il faudrait deux ou trois commits qui représentent bien les étapes du travail proposé, comme ça je pourrais review chaque modif séparément. Par exemple: un commit pour l'ajout du code goldap, un commit pour la création du script de test, un commit pour l'ajout du test à Drone, et un commit pour la partie documentation / rapport experimental. À toi de voir ce qui fait sens comme découpage. Le souci c'est pour moi pour relire ton travail, quand je vais sur https://git.deuxfleurs.fr/Deuxfleurs/bottin/pulls/2/commits il y a plein d'étapes intermédiaires que je n'ai pas envie de voir donc il faudrait les faire disparaître. Tu peux apprendre à faire un squash par exemple ici: https://www.internalpointers.com/post/squash-commits-into-one-git

Salut Erwan, il faudrait faire un squash de tes commits pour pas avoir plein de commits intermédiaires. Ca signifie réécrire l'historique du dépôt pour remplacer tes 21 commits par moins de commits, où chacun des nouveau commits est la "somme" de plusieurs des commits précédents. Idéalement il faudrait deux ou trois commits qui représentent bien les étapes du travail proposé, comme ça je pourrais review chaque modif séparément. Par exemple: un commit pour l'ajout du code goldap, un commit pour la création du script de test, un commit pour l'ajout du test à Drone, et un commit pour la partie documentation / rapport experimental. À toi de voir ce qui fait sens comme découpage. Le souci c'est pour moi pour relire ton travail, quand je vais sur <https://git.deuxfleurs.fr/Deuxfleurs/bottin/pulls/2/commits> il y a plein d'étapes intermédiaires que je n'ai pas envie de voir donc il faudrait les faire disparaître. Tu peux apprendre à faire un squash par exemple ici: <https://www.internalpointers.com/post/squash-commits-into-one-git>
erwan requested review from quentin 2021-07-13 11:29:02 +00:00
quentin changed title from test-go-Bottin to Add end-to-end tests to Bottin 2021-07-13 12:45:07 +00:00
quentin requested changes 2021-07-13 13:04:32 +00:00
quentin left a comment
Owner

Merci pour ton travail Erwan, il a permi de débusquer pas mal de bugs je pense !
Parce que ton travail sur les tests sera notre fondation pour la suite, je te propose de refactoriser ton code entre plusieurs fichiers et de réduire la complexité de tes fonctions, ça nous aidera beaucoup. Pour le reste, ton code semble tout à fait fonctionnel et juste algorithmiquement ! Ah et dernier point, si tu pouvais ne garder que de l'angais dans ton code, que ce soit pour les commentaires et les logs, ce serait parfait !

Quelques points pour faire les corrections :

  • Mettre goldap de côté dans une branche et annuler l'import de goldap dans cette branche
  • Séparer ton code en plusieurs fichiers et séparer génération/comparaison des données et intéractions avec LDAP (que ce soit pour l'ajout ou la récupération). Essayer de garder une complexité cyclomatique faible.
  • Réaliser les corrections mineures (nom de fichier, commentaires et log en anglais)
Merci pour ton travail Erwan, il a permi de débusquer pas mal de bugs je pense ! Parce que ton travail sur les tests sera notre fondation pour la suite, je te propose de refactoriser ton code entre plusieurs fichiers et de réduire la complexité de tes fonctions, ça nous aidera beaucoup. Pour le reste, ton code semble tout à fait fonctionnel et juste algorithmiquement ! Ah et dernier point, si tu pouvais ne garder que de l'angais dans ton code, que ce soit pour les commentaires et les logs, ce serait parfait ! Quelques points pour faire les corrections : - Mettre goldap de côté dans une branche et annuler l'import de goldap dans cette branche - Séparer ton code en plusieurs fichiers et séparer génération/comparaison des données et intéractions avec LDAP (que ce soit pour l'ajout ou la récupération). Essayer de garder une complexité cyclomatique faible. - Réaliser les corrections mineures (nom de fichier, commentaires et log en anglais)
go.mod Outdated
@ -3,6 +3,7 @@ module bottin
go 1.13
require (
github.com/go-ldap/ldap/v3 v3.3.0 // indirect
Owner

Pourquoi ce commentaire //indirect ? On peut le supprimer si il ne sert à rien ;)

Pourquoi ce commentaire `//indirect` ? On peut le supprimer si il ne sert à rien ;)
erwan marked this conversation as resolved
@ -0,0 +1,25 @@
package message
Owner

Vu qu'on a pas corrigé de bug encore sur LDAP, ce serait bien de l'importer dans une branche à part où on corrigera les bugs en meme temps. On aura donc deux branches :

  • cette branche où on introduit des tests
  • une nouvelle branche qui importe et fix goldap
Vu qu'on a pas corrigé de bug encore sur LDAP, ce serait bien de l'importer dans une branche à part où on corrigera les bugs en meme temps. On aura donc deux branches : - cette branche où on introduit des tests - une nouvelle branche qui importe et fix goldap
erwan marked this conversation as resolved
@ -7,3 +7,3 @@
"time"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
Owner

Pareil, garder cette modification pour une autre branche :)

Pareil, garder cette modification pour une autre branche :)
erwan marked this conversation as resolved
@ -1,6 +1,6 @@
package ldapserver
import ldap "github.com/lor00x/goldap/message"
import ldap "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
@ -4,3 +4,3 @@
"fmt"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
@ -6,3 +6,3 @@
"fmt"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
@ -1,6 +1,6 @@
package ldapserver
import ldap "github.com/lor00x/goldap/message"
import ldap "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
@ -4,3 +4,3 @@
"strings"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
main.go Outdated
@ -16,3 +16,3 @@
consul "github.com/hashicorp/consul/api"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
main.go Outdated
@ -327,0 +330,4 @@
}
admin_pass_str = base64.RawURLEncoding.EncodeToString(admin_pass)
} else {
server.logger.Printf("It seems that exists a password in environnement variable")
Owner

Je mettrais bien quelque chose comme ça :

logger.Debug("BOTTIN_DEFAULT_ADMIN_PW environment variable is set, using it for admin's password")

Je pense qu'on devrait utiliser le logger "normal" ici, du moins si on peut.

Je mettrais bien quelque chose comme ça : ```go logger.Debug("BOTTIN_DEFAULT_ADMIN_PW environment variable is set, using it for admin's password") ``` Je pense qu'on devrait utiliser le logger "normal" ici, du moins si on peut.
erwan marked this conversation as resolved
read.go Outdated
@ -7,3 +7,3 @@
ldap "bottin/ldapserver"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
Owner

idem autre branche

idem autre branche
erwan marked this conversation as resolved
@ -0,0 +1,8 @@
module bottin/integration
Owner

Juste pour info, là en réalité on créer un projet go à part qui nous sert pour tester.
À terme, ça pourrait être intéressant de l'intégrer dans le projet parent en utilisant le framework de test de go.

Mais je pense qu'on peut dire que c'est en dehors du scope de cette demande de fusion.

Juste pour info, là en réalité on créer un projet go à part qui nous sert pour tester. À terme, ça pourrait être intéressant de l'intégrer dans le projet parent en utilisant le framework de test de go. Mais je pense qu'on peut dire que c'est en dehors du scope de cette demande de fusion.
erwan marked this conversation as resolved
@ -0,0 +1,431 @@
package main
Owner

Je te recommande d'exécuter go fmt tonfichier.go pour le formatter automatiquement de sorte à qu'il formatte de manière standard ton fichier.

Vis à vis du nommage, je dirais que tu as fais un test de bout en bout :
chaque étape de ton test dépend de la précédente, le test entier étant un scénario.

C'est différent du test d'intégration qui va tester indépendamment.

Je te propose de renommer ton dossier test_automatic en test, ce qui est la convention.
Et de renommer integration.go en e2e.go (end to end, de bout en bout), ce qui sera plus clair pour les autres personnes.

Je te recommande d'exécuter `go fmt tonfichier.go` pour le formatter automatiquement de sorte à qu'il formatte de manière standard ton fichier. Vis à vis du nommage, je dirais que tu as fais un test de bout en bout : chaque étape de ton test dépend de la précédente, le test entier étant un scénario. C'est différent du test d'intégration qui va tester indépendamment. Je te propose de renommer ton dossier `test_automatic` en `test`, ce qui est la convention. Et de renommer integration.go en e2e.go (end to end, de bout en bout), ce qui sera plus clair pour les autres personnes.
erwan marked this conversation as resolved
@ -0,0 +20,4 @@
func printError(LDAPError error) {
Owner

Handler pour gérer les erreurs de LDAP

Handler pour gérer les erreurs de LDAP
erwan marked this conversation as resolved
@ -0,0 +28,4 @@
func createOU(l *ldap.Conn) error {
req := ldap.NewAddRequest("ou=groups,dc=deuxfleurs,dc=fr",nil)
Owner

Ce qui pourrait être intéressant c'est d'avoir une fonction qui fait les appels à la lib LDAP pour ajouter l'organisation unit.

Et une autre fonction qui contient les données.

Exemple de la signature de la fonction qui ajoute les organisation units:

func createOU (l *ldap.Conn, dn string, attributes map[string][]string)

Et après tu peux écrire tes données de test comme ça :


func createHierarchy(l *ldap.Conn) {
  base := map[string][]string{
    "objectclass": []string{"organizationalUnit", "top"},
    "structuralobjectclass": []string{"organizationalUnit"},
  }
  
  base["description"] = []string{"OrganizationalUnit qui regroupe tous les groupes"}
  createOU(l, "ou=groups,dc=deuxfleurs,dc=fr", base)
  
  base["description"] = []string{"OrganizationalUnit qui regroupe tous les users"}
  createOU(l, "ou=users,dc=deuxfleurs,dc=fr", base)
}

Ce qui pourrait être intéressant c'est d'avoir une fonction qui fait les appels à la lib LDAP pour ajouter l'organisation unit. Et une autre fonction qui contient les données. Exemple de la signature de la fonction qui ajoute les organisation units: ```go func createOU (l *ldap.Conn, dn string, attributes map[string][]string) ``` Et après tu peux écrire tes données de test comme ça : ```go func createHierarchy(l *ldap.Conn) { base := map[string][]string{ "objectclass": []string{"organizationalUnit", "top"}, "structuralobjectclass": []string{"organizationalUnit"}, } base["description"] = []string{"OrganizationalUnit qui regroupe tous les groupes"} createOU(l, "ou=groups,dc=deuxfleurs,dc=fr", base) base["description"] = []string{"OrganizationalUnit qui regroupe tous les users"} createOU(l, "ou=users,dc=deuxfleurs,dc=fr", base) } ```
erwan marked this conversation as resolved
@ -0,0 +49,4 @@
return err
}
func generateName(r *rand.Rand) (name string) {
Owner

Là aussi on a un handler, en tout cas de la logique pure.

Là aussi on a un handler, en tout cas de la logique pure.
erwan marked this conversation as resolved
@ -0,0 +51,4 @@
func generateName(r *rand.Rand) (name string) {
for only_one := true; only_one; _, only_one = all_names[name]{
name = fmt.Sprintf("%d",r.Int())
Owner

Tu peux rendre ta fonction de génération plus intéressante si tu pioches sur un ensemble de caractère plus grand que juste des nombres. Idéalement tout UTF-8 mais peut-être pas, au moins [a-zA-Z0-9], peut-être aussi des accents et des caractères spéciaux ?

Quelque chose comme ça :

charset := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/-*éç"
charset[r.Intn(len(charset)-1)]

Et normalement tu as pas besoin du sprintf.

Tu peux rendre ta fonction de génération plus intéressante si tu pioches sur un ensemble de caractère plus grand que juste des nombres. Idéalement tout UTF-8 mais peut-être pas, au moins `[a-zA-Z0-9]`, peut-être aussi des accents et des caractères spéciaux ? Quelque chose comme ça : ```go charset := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/-*éç" charset[r.Intn(len(charset)-1)] ``` Et normalement tu as pas besoin du sprintf.
erwan marked this conversation as resolved
@ -0,0 +58,4 @@
return
}
func createGroup(r *rand.Rand, l *ldap.Conn) (tab_AddRequest []ldap.AddRequest, err error) {
Owner

Tu fais trois choses ici :

  1. Tu as une logique pour créer un seul groupe
  2. Tu as de la logique pour créer plusieurs groupes
  3. Tu as de la logique pour collecter les données des groupes que tu as généré, le tab_AddRequest.

Je dirais juste, d'une manière générale, génère toutes tes données en dehors des fonctions
qui font des appels à LDAP.

Tu pourrais avoir un fichier qui génèrent les données.

Tu fais trois choses ici : 1. Tu as une logique pour créer un seul groupe 2. Tu as de la logique pour créer plusieurs groupes 3. Tu as de la logique pour collecter les données des groupes que tu as généré, le `tab_AddRequest`. Je dirais juste, d'une manière générale, génère toutes tes données en dehors des fonctions qui font des appels à LDAP. Tu pourrais avoir un fichier qui génèrent les données.
erwan marked this conversation as resolved
@ -0,0 +117,4 @@
// return ldap.Attribute{}
}
func test_attributes(l *ldap.Conn, tab_AddRequest []ldap.AddRequest, filter_objectclass, user_or_group string) (err error) {
Owner

Tu l'as dit toi même séparer récupérer les résultats et les comparer.

Tu l'as dit toi même séparer récupérer les résultats et les comparer.
erwan marked this conversation as resolved
@ -0,0 +151,4 @@
//j est l'indice qui représente la j-ème valeur de notre attribut
var j int
var att *ldap.Attribute
for i,attributes := range result_attributes {
Owner

Simplifier avec la fonction Entries.GetAttributesValues.
En tout cas, ne pas hésiter à créer un handler pour gérer ça.

Simplifier avec la fonction Entries.GetAttributesValues. En tout cas, ne pas hésiter à créer un handler pour gérer ça.
erwan marked this conversation as resolved
@ -0,0 +174,4 @@
return nil
}
func clean(l *ldap.Conn, AddReq_users, AddReq_groups []ldap.AddRequest,user, group bool) (err error){
Owner

Juste pour info, c'est pas terrible des booleens comme ça parce que tu ne sais pas à quoi correspond le booleen quand tu appelles la fonction.

Donc déjà souvent on va préférer mettre un enum, tu aurais pu faire un seul enum avec user et group.
USER, GROUP, et BOTH.

En plus, si tu n'utilises pas cette possibilité, ça ne sert à rien de la mettre.

Enfin, les deux sont des tableaux, sur lesquels tu itères, donc en réalité si tu veux en "désactiver" un, tu peux simplement mettre un tableau vide.

Pour l'utilisateur, tu peux créer un type spécial, nommé KeepUser et KeepGroup, qui sont en réalité des tableaux vides.

Juste pour info, c'est pas terrible des booleens comme ça parce que tu ne sais pas à quoi correspond le booleen quand tu appelles la fonction. Donc déjà souvent on va préférer mettre un enum, tu aurais pu faire un seul enum avec user et group. USER, GROUP, et BOTH. En plus, si tu n'utilises pas cette possibilité, ça ne sert à rien de la mettre. Enfin, les deux sont des tableaux, sur lesquels tu itères, donc en réalité si tu veux en "désactiver" un, tu peux simplement mettre un tableau vide. Pour l'utilisateur, tu peux créer un type spécial, nommé KeepUser et KeepGroup, qui sont en réalité des tableaux vides.
erwan marked this conversation as resolved
@ -0,0 +194,4 @@
}
}
}
defer log.Debug("Fin clean")
Owner

Pourquoi un defer log.Debug ? :P

Pourquoi un defer log.Debug ? :P
erwan marked this conversation as resolved
Owner

?? bien que tu mettes un nombre fixe pour la source, la seed change à chaque run.
Saurais-tu expliquer/corriger ça ?

?? bien que tu mettes un nombre fixe pour la source, la seed change à chaque run. Saurais-tu expliquer/corriger ça ?
erwan marked this conversation as resolved
@ -0,0 +1,40 @@
Introduction et Observation premières:
Owner

On peut l'enlever mais ne jette pas ces informations, reporte les bien toutes dans l'issue !

On peut l'enlever mais ne jette pas ces informations, reporte les **bien toutes** dans l'issue !
erwan marked this conversation as resolved
@ -0,0 +1,12 @@
#!/bin/sh
Owner

Un nom qui me semble bien pour ce fichier c'est runner.sh mais tu peux conserver le nom actuel si tu préfères :)

Un nom qui me semble bien pour ce fichier c'est `runner.sh` mais tu peux conserver le nom actuel si tu préfères :)
erwan marked this conversation as resolved
@ -0,0 +1,12 @@
#!/bin/sh
Owner

Pour être sûr que toutes les erreurs soient bien remontées, je te recommande de commencer ton script par :

set -e

Le modifier -x te permet aussi de voir la commande finale exécutée, ça peut être pratique lors de la lecture des logs de drone, tu aurais alors :

set -ex
Pour être sûr que toutes les erreurs soient bien remontées, je te recommande de commencer ton script par : ``` set -e ``` Le *modifier* `-x` te permet aussi de voir la commande finale exécutée, ça peut être pratique lors de la lecture des logs de drone, tu aurais alors : ``` set -ex ```
erwan marked this conversation as resolved
@ -0,0 +1,12 @@
#!/bin/sh
#export BOTTIN_DEFAULT_ADMIN_PW=$(openssl rand -base64 24)
Owner

Ça je suppose qu'il n'y avait pas openssl dans l'image Consul.
Dans ce cas, on peut l'enlever, et c'est un sujet intéressant à discuter.
Ta solution de contournement (mettre en dur le mot de passe dans le .drone.yml) me semble adaptée.

Ça je suppose qu'il n'y avait pas openssl dans l'image Consul. Dans ce cas, on peut l'enlever, et c'est un sujet intéressant à discuter. Ta solution de contournement (mettre en dur le mot de passe dans le `.drone.yml`) me semble adaptée.
erwan marked this conversation as resolved
@ -0,0 +8,4 @@
./bottin > /dev/null 2>&1 &
sleep 1
./test_automatic/integration
rm config.json
Owner

Le rm n'est pasobligatoire, à chaque build drone repart d'un environnement propre, tu n'as pas besoin de nettoyer das tes scripts de test

Le `rm` n'est pasobligatoire, à chaque *build* drone repart d'un environnement propre, tu n'as pas besoin de nettoyer das tes scripts de test
erwan marked this conversation as resolved
@ -0,0 +9,4 @@
sleep 1
./test_automatic/integration
rm config.json
exit 0
Owner

Enlever le exit 0 pour ne pas fausser le test, si un programme est en erreur, on veut que ça passe rouge dans drone !

Enlever le exit 0 pour ne pas fausser le test, si un programme est en erreur, on veut que ça passe rouge dans drone !
erwan marked this conversation as resolved
write.go Outdated
@ -9,3 +9,3 @@
consul "github.com/hashicorp/consul/api"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
Owner

idem branche

idem branche
erwan marked this conversation as resolved
erwan requested review from quentin 2021-07-16 15:21:10 +00:00
erwan force-pushed test-go-Bottin from 524d10bbad to 7144c5c32d 2021-07-19 16:33:02 +00:00 Compare
erwan force-pushed test-go-Bottin from 7144c5c32d to e6731dcd52 2021-07-19 16:37:16 +00:00 Compare
erwan force-pushed test-go-Bottin from e6731dcd52 to 5cfc1a856a 2021-07-19 16:48:27 +00:00 Compare
erwan force-pushed test-go-Bottin from 5cfc1a856a to 10971b23ce 2021-07-19 16:49:29 +00:00 Compare
erwan force-pushed test-go-Bottin from 10971b23ce to 1fd7c9874d 2021-07-19 16:56:00 +00:00 Compare
erwan force-pushed test-go-Bottin from 1fd7c9874d to 9a8c19ec0f 2021-07-19 16:58:35 +00:00 Compare
Owner

Ok pour moi

Ok pour moi
quentin approved these changes 2021-07-22 07:55:23 +00:00
quentin left a comment
Owner

Ok pour moi

Ok pour moi
quentin approved these changes 2021-07-22 07:58:18 +00:00
quentin left a comment
Owner

Ok avec moi approuvé

Ok avec moi approuvé
erwan merged commit 9a8c19ec0f into main 2021-07-22 07:59:11 +00:00
erwan deleted branch test-go-Bottin 2021-07-22 07:59:29 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/bottin#2
No description provided.