Conversation
benjamin-hubert
left a comment
benjamin-hubert
left a comment
There was a problem hiding this comment.
Thomas,
Tu as commit tes .idea, qui sont propres à ton IDE de développement (PHPStorm du coup).
Ce n'est pas une bonne pratique car ça écraserait nos configs si on récupérait le projet. J'aimerai bien que tu rajoutes cet élément dans le git ignore et que tu fasses en sorte qu'ils disparaissent de ta PR.
Je te laisse regarder ;).
| <br/> | ||
| <!-- petit rajout pour avoir les liens directement sur l'acceuil --> | ||
| <ul> | ||
| <li><a href={{ path('test_interview_homepage') }}>Index du bundle Test</a></li> |
There was a problem hiding this comment.
Attention à tes liens :)
There was a problem hiding this comment.
Pourquoi? Est-ce que je dois rajouter une vérification que la route existe?
There was a problem hiding this comment.
Non, il manque des guillemets tout simplement dans ton href ;)
There was a problem hiding this comment.
Oupsss.... Corrigé en même temps que swiftmailer
|
|
||
| 1. Run the PhpUnit test. Check if there are any errors, if so fix them. | ||
| OK (en mettant en commentaires "swiftmailer" dans config_test.yml et modifier le test) | ||
| (commande à la racine : "phpunit -c app/ src/AppBundle/Tests/Controller/DefaultControllerTest.php") |
There was a problem hiding this comment.
Je ne comprend pas pourquoi tu as du désactiver le swiftmailer ? Qu'est-ce qui se passait ?
There was a problem hiding this comment.
OK j'ai vu en voulant faire le test, essaies de résoudre le problème sans virer ça du config_test :)
There was a problem hiding this comment.
Ok j'ai rajouter un require pour le swiftmailer bundle et fais un composer install, et là c'est ok, je l'ai pas configuré par contre
| 1. Create a method helloAction under AppBundle\Controller\DefaultController | ||
| * for route `/hello` | ||
| * with a proper json return `{"hello":"world!"}` | ||
| Ok (return JsonResponse) |
| (commande à la racine : "phpunit -c app/ src/AppBundle/Tests/Controller/DefaultControllerTest.php") | ||
|
|
||
| 1. Create a new Bundle "InterviewBundle" within the namespace "Test" | ||
| OK (changement de l'index de l'appli avec la commande generate:bundle) |
| (commande à la racine pour les setters/getters: "php app/console doctrine:mongodb:generate:documents TestInterviewBundle") | ||
|
|
||
| 1. Define ODM "Bios" repository under namespace Test/InterviewBundle/Repositories | ||
| Ok sauf qu'on doit utiliser "Repository" et non "Repositories" |
There was a problem hiding this comment.
Effectivement la bonne pratique est Repository mais ça aurait pu fonctionner aussi avec Repositories ;)
|
|
||
| 1. Define ODM "Bios" document under namespace Test/InterviewBundle/Documents | ||
| Ok sauf qu'on ne doit pas utiliser "Documents" mais "Document" pour que le mapping fonctionne (pas trouvé d'autres solutions) | ||
| (commande à la racine pour les setters/getters: "php app/console doctrine:mongodb:generate:documents TestInterviewBundle") |
| * Use the logger to log operations (error, warning, debug) | ||
| Ok, inscrit dans les services du Bundle (pour la portabilité) | ||
| Cependant, j'ai utilisé ma classe test pour tester le service, mais je ne me suis pas servi du logger | ||
|
|
There was a problem hiding this comment.
Ca serait bien d'implémenter le logger stp :).
Sur les retours, un service ne doit pas renvoyer de Response (une Response c'est essentiellement un objet qui HTTP pour renvoeyr des données au navigateur et donc, envoyé que par le controller). Est-ce que tu peux modifier un peu le code pour que ça soit plus propre là dessus ?
De même, ton renvoi "false" depuis tes Repository me semble un peu brouillon car je pense qu'il y'a moyen d'éviter ça derrière, mais je termine la revue pour te le confirmer !
Sinon le reste me semble bien !
There was a problem hiding this comment.
J'ai injecter le logger dans le BiosService
|
|
||
| 1. Run the PhpUnit test. Check if there are any errors, if so fix them. | ||
| OK (en mettant en commentaires "swiftmailer" dans config_test.yml et modifier le test) | ||
| (commande à la racine : "phpunit -c app/ src/AppBundle/Tests/Controller/DefaultControllerTest.php") |
There was a problem hiding this comment.
OK j'ai vu en voulant faire le test, essaies de résoudre le problème sans virer ça du config_test :)
|
|
||
| Ok j'ai mis les tests dans Test/InterviewBundle/Tests/Controller/DefaultControllerTest.php | ||
| (commande à la racine: "phpunit -c app/ src/Test/InterviewBundle//Tests/Controller/DefaultControllerTest.php") | ||
|
|
| 1. make a unit test for the BiosService | ||
| * at least 1 method of your choice | ||
| Ok j'ai mis un test de 'getAllContributions' avec retour Json à la suite des tests précédents | ||
|
|
There was a problem hiding this comment.
Alors, si tu test le service, il faut créer une nouvelle class (et pas mettre ton test dans la classe de test du controller).
Le test est pas mal sinon !
There was a problem hiding this comment.
J'ai séparé les deux tests et corriger celui de service :
en fait, il vérifiait la réponse de $client->request('GET', '/contributions/OOP'); (qui rend un json) situé au dessus et non le retour du service!
| 1. Check the symfony application for errors and fix them if any. | ||
| Ok, j'ai du modifier "test:" en "test_interview:" qui est le namespace du bundle | ||
| et ajouter du code dans le DependencyInjection/Configuration pour ajouter le paramètre ping de valeur pong (valeur par défaut obligatoire) | ||
|
|
| * If you accept, run the command | ||
|
|
||
| OK, j'ai du modifier le code demandé en 15 forcément | ||
|
|
There was a problem hiding this comment.
Ca marche, avec un check sur le paramètre qui devrait être obligatoire serait encore mieux ;)
There was a problem hiding this comment.
Well done, mais c'est juste une vérification d'existence, un message et un exit
correction des guillemets pour les liens ajout d'un composer require pour le swiftmailer bundle
Correction du test du service qui ne vérifiait pas la bonne réponse
…message d'erreur sinon
2 participants
Manque encore la bonus task mais ce sera fait d'ici la semaine prochaine