Add end-to-end tests to Bottin #2
Loading…
Reference in a new issue
No description provided.
Delete branch "test-go-Bottin"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Rajout d'un test automatique sous drone.yml qui teste:
Nécessite correction d'un bogue d'encodage ASN.1 dans goldap utilisé par ldapserver
WIP:test-go-Bottinto test-go-Bottintest-go-Bottinto WIP:test-go-BottinWIP:test-go-Bottinto test-go-BottinSalut 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
test-go-Bottinto Add end-to-end tests to BottinMerci 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 :
@ -3,6 +3,7 @@ module bottin
go 1.13
require (
github.com/go-ldap/ldap/v3 v3.3.0 // indirect
Pourquoi ce commentaire
//indirect
? On peut le supprimer si il ne sert à rien ;)@ -0,0 +1,25 @@
package message
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 :
@ -7,3 +7,3 @@
"time"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
Pareil, garder cette modification pour une autre branche :)
@ -1,6 +1,6 @@
package ldapserver
import ldap "github.com/lor00x/goldap/message"
import ldap "bottin/goldap"
idem autre branche
@ -4,3 +4,3 @@
"fmt"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
idem autre branche
@ -6,3 +6,3 @@
"fmt"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
idem autre branche
@ -1,6 +1,6 @@
package ldapserver
import ldap "github.com/lor00x/goldap/message"
import ldap "bottin/goldap"
idem autre branche
@ -4,3 +4,3 @@
"strings"
ldap "github.com/lor00x/goldap/message"
ldap "bottin/goldap"
idem autre branche
@ -16,3 +16,3 @@
consul "github.com/hashicorp/consul/api"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
idem autre branche
@ -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")
Je mettrais bien quelque chose comme ça :
Je pense qu'on devrait utiliser le logger "normal" ici, du moins si on peut.
@ -7,3 +7,3 @@
ldap "bottin/ldapserver"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
idem autre branche
@ -0,0 +1,8 @@
module bottin/integration
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.
@ -0,0 +1,431 @@
package main
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
entest
, 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.
@ -0,0 +20,4 @@
func printError(LDAPError error) {
Handler pour gérer les erreurs de LDAP
@ -0,0 +28,4 @@
func createOU(l *ldap.Conn) error {
req := ldap.NewAddRequest("ou=groups,dc=deuxfleurs,dc=fr",nil)
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:
Et après tu peux écrire tes données de test comme ça :
@ -0,0 +49,4 @@
return err
}
func generateName(r *rand.Rand) (name string) {
Là aussi on a un handler, en tout cas de la logique pure.
@ -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())
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 :
Et normalement tu as pas besoin du sprintf.
@ -0,0 +58,4 @@
return
}
func createGroup(r *rand.Rand, l *ldap.Conn) (tab_AddRequest []ldap.AddRequest, err error) {
Tu fais trois choses ici :
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.
@ -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) {
Tu l'as dit toi même séparer récupérer les résultats et les comparer.
@ -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 {
Simplifier avec la fonction Entries.GetAttributesValues.
En tout cas, ne pas hésiter à créer un handler pour gérer ça.
@ -0,0 +174,4 @@
return nil
}
func clean(l *ldap.Conn, AddReq_users, AddReq_groups []ldap.AddRequest,user, group bool) (err error){
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.
@ -0,0 +194,4 @@
}
}
}
defer log.Debug("Fin clean")
Pourquoi un defer log.Debug ? :P
?? bien que tu mettes un nombre fixe pour la source, la seed change à chaque run.
Saurais-tu expliquer/corriger ça ?
@ -0,0 +1,40 @@
Introduction et Observation premières:
On peut l'enlever mais ne jette pas ces informations, reporte les bien toutes dans l'issue !
@ -0,0 +1,12 @@
#!/bin/sh
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 :)@ -0,0 +1,12 @@
#!/bin/sh
Pour être sûr que toutes les erreurs soient bien remontées, je te recommande de commencer ton script par :
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 :@ -0,0 +1,12 @@
#!/bin/sh
#export BOTTIN_DEFAULT_ADMIN_PW=$(openssl rand -base64 24)
Ç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.@ -0,0 +8,4 @@
./bottin > /dev/null 2>&1 &
sleep 1
./test_automatic/integration
rm config.json
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@ -0,0 +9,4 @@
sleep 1
./test_automatic/integration
rm config.json
exit 0
Enlever le exit 0 pour ne pas fausser le test, si un programme est en erreur, on veut que ça passe rouge dans drone !
@ -9,3 +9,3 @@
consul "github.com/hashicorp/consul/api"
message "github.com/lor00x/goldap/message"
message "bottin/goldap"
idem branche
524d10bbad
to7144c5c32d
7144c5c32d
toe6731dcd52
e6731dcd52
to5cfc1a856a
5cfc1a856a
to10971b23ce
10971b23ce
to1fd7c9874d
1fd7c9874d
to9a8c19ec0f
Ok pour moi
Ok pour moi
Ok avec moi approuvé