Feat/tests #37
Feat/tests #37
Conversation
|
Salut @TeddyRoncin ! Par contre, une PR de 4 000 lignes ajoutées et 200 supprimés sans explications c'est hors d'atteinte pour un humain normal. En tout cas, excellent boulot, et hésite pas à me contacter si t'as besoin d'aide ou de conseils ! |
|
Hello, ouais, pas de problème. Le truc des espaces c'était pour le linting et respecter ce qu'il fallait faire avant de merge une pr, mais ok, je vais en faire une autre a côté pour ça |
|
Et au fait je sais pas si tu as vu le gform passer, mais le projet devrait beaucoup avancer ce semestre, on a relancé le dev, on est en train de recruter 10ish nouveaux ;) |
0a66013
to
b01104b
Compare
There was a problem hiding this comment.
Ok, j'ai fini le review de ta PR !
J'ai mis plein de commentaires de review, j'espère qu'ils te seront utiles.
Pour php-cs-fixer, quand je le lance sur ta branche, ça me fait les modifs dont on a parlé, mais quand je le lance sur la branche dev, ça ne les fait pas. Est-ce que tu peux merge dev dans ta branche pour voir si ça change quelque chose ? Attention, merge dev dans ta branche, pas l'inverse
Par contre, je ne peux pas tester toutes les merveilleuses fonctionnalités que tu as développées, j'ai cette erreur au lancement des tests :
/var/www/html$ php bin/phpunit
PHPUnit 9.5.28 by Sebastian Bergmann and contributors.
Testing
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE
Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 69632 bytes) in /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php on line 251
Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 69632 bytes) in /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php on line 251
| $client = static::createClient(); | ||
| $client->setDefaultOptions(['headers' => ['CAS-LOGIN' => 'test', 'Content-Type' => 'application/merge-patch+json']]); |
There was a problem hiding this comment.
Tu devrais englober ces deux lignes-ci dans un helper. Ce helper prendrait en paramètre le login CAS, le content-type et autre, et retourne un object client
There was a problem hiding this comment.
La méthode a été créée dans le EtuUTTApiTestCase, il manque juste à l'utiliser
(Je réponds juste ici pour être sûr de ne pas oublier de le faire 😄 )
| // Update the database | ||
| $this->em->persist($firstUESubscription); | ||
| $this->em->persist($secondUESubscription); | ||
| // TODO : fix the error that occurs when uncommenting this line (Doctrine\ORM\EntityNotFoundException: Unable to find "Proxies\__CG__\App\Entity\UE" entity identifier associated with the UnitOfWork) |
There was a problem hiding this comment.
Évite de laisser des commentaires de TODO dans une PR, fais la PR quand tout est done.
C'est pour cela que je te conseille de faire une PR par groupe de test. Une PR pour les test User, une pour les groupes...
There was a problem hiding this comment.
Yes, mais cette erreur j'ai pas compris du tout pourquoi elle se passait, et si on décommente pas la ligne, le test ne teste pas quelque chose d'intéressant. Maintenant que tu t'es déter à tout review tu veux quand même que je fasse plusieurs PR différentes ou tant pis ?
There was a problem hiding this comment.
Dans ce cas, ne commit pas ce morceau et laisse le en local jusqu'à ce que tu fixes le problème.
Quand tu fais une PR, c'est que ton code est fonctionnel et terminé
Et oui, je veux bien que tu fasses une PR par thème pour deux raisons :
- Une PR de 3000 lignes de codes à review c'est pas humain à review (j'ai pas tout lu)
- Certains points de la PR sont bons, mais d'autres non. Donc si tu sépares ta PR en plusieurs, certains points vont être merge avant les autres que tu dois retravailler
There was a problem hiding this comment.
Quand tu fais une PR, c'est que ton code est fonctionnel et terminé
Yes, je vais passer la PR en draft du coup pour le moment
Une PR de 3000 lignes de codes à review c'est pas humain à review (j'ai pas tout lu)
Ah ok, je pensais que tu avais quand même tout lu (j'avoue que le peu de choses que tu avais à redire m'avait quand même étonné 🤣 )
Du coup ouais, je te fais ça dans la semaine
| $client = static::createClient(); | ||
| $client->setDefaultOptions(['headers' => ['CAS-LOGIN' => 'test']]); | ||
| // studentId is too big : argument should be skipped and return everything | ||
| $crawler = $client->request('GET', '/users?studentId=999999999999999999999999999999999999999'); |
There was a problem hiding this comment.
Pas la peine d'aller aussi haut, tu peux enlever la moitié des 9
Tu étais vachement déter mdr, merci beaucoup ! :)
C'est bizarre, je viens de refaire un php-cs-fixer sur le projet (normalement tout devrait être comme sur cette branche, donc sans avoir déjà linté le code), mais il me trouve quasiment rien (genre 4 fichiers).
J'ai pas ça de mon côté. Essaie de lancer un |
|
Salut beau gosse 👋 |
|
Pour la limite de mémoire, maintenant nous utilisons Docker pour que tout le monde ait la même configuration.
Dans les deux cas, il faut absolument que tu merge Dev dans ta branche pour avoir les dernières grosses modifications, et que tu utilises le projet avec Docker au lieu de ta config perso. N'hésite pas à m'appeler si tu as des problèmes avec l'installation |
Ah ok, bien vu ! Désolé, je me souviens plus du tout de l'avoir fait
Je peux essayer de voir si ça se fait, mais je suis pas sûr qu'on puisse maîtriser la RAM utilisée dans les tests (je ferai quand même des recherches là-dessus, parce que ouais c'est chiant, surtout si la quantité de RAM utilisée est proportionnelle aux nombre de tests)
Ok, je vais essayer avec Docker du coup, j'avais commencé à tester la dernière fois mais j'avais eu un peu la flemme sur le coup :) |
Added many filters for the /users routes. User's nickname are now returned when fetching a collection of users
…e current version Some code (in the filters) was outdated and not updated during the rebase
…breaking changes were fixed Most notable changes are the change of the version of Symfony, ApiPlatform and PHP
…ions of libraries
groups is a keyword in mysql, so I replaced the table name in the code (App\Entity\Group) by `groups`. Using `...` tells mysql that the content has to be evaluated, and is not a keyword
Started to implement application tests on routes asso/* and user/*
testing content of json
Made 1 class per route. GET users/ is now more tested to verify it responds normally when something is not expected (not connected, parameter out of range, ...)
reafactoring tests structure. We now call directly routes, instead of calling a URL. Tests are now disconnected to each other
Improved GetUsers tests to make them test everything. I can't see any more major issue with this class. Now uses database calls to test API calls. Also created superclass EtuUTTApiTestCase, which should provide a lot of useful methods in the future
Class EtuUTTApiTestCase now saves the 'test' user, created in the setUp method. Directory tests/users was renamed to tests/Users, to be more coherent with conventions used in the project
Added createUser method. The methods takes 4 parameters : $firstName, $lastName, $login and $role. Role is not mendatory. If not provided, it is defaulted to 'ROLE_USER'. It creates a user according to the parameters, put it in the database, and flush the data. In the future, if necessary, it could be useful to add a parameter to ask if we want to flush the database, because it could maybe be slow if we need to do that a lot.
testing normal call, when not connected, when the user does not exist, when no body is provided, sql injections and invalid field contents
testing normal calls, when user doesn't have the permissions, when the user is not connected, when the user does not exist and verified sql injections
changed version of dependency symfony/http-foundation. It's now possible to set a maximum/minimum of visible groups while seeding the Group table. lint. changed security of PATCH /groups/{slug}.
Translations where not present in the group:read:one group. changed the group admin voter. Added utility functions to EtuUTTApiTestCase. updated a few tests on /users routes
…hanges made to it it is now possible to call the backupDatabase method from the EtuUTTApiTestCase class to make a backup, and then call the assertDatabaseSameExcept method to assert the new database and the old one are the same. Expected differences can be specified in the arguments of the function
Added tests for each function of the route. Removed some prints. Added assertSameUserReadSome method in EtuUTTApiTestCase. Added order in the /users routes. Fixed a problem with skip_null_values (it now seems like it's necessary to explicit that parameter for each route that overrides normalizationContext). Added a way to generate a custom amount of users in the UserSeeder. Changed phone number generation in UserInfoVisibilitySeeder. Added a parameter in BrancheFiliereFormationSeeder to set the minimum amount of users with filiere that should be generated
…alid parameter values Added a test to test the parameter name. Added some tests to verify API responds correctly when an invalid value is passed to the parameters. Updated the testUEParameter test. Added parameter $flush to EtuUTTApiTestCase::createUser (defaults to false) to avoid the changes to be sent to the database (the new entity is still persisted)
SearchInNamesFilter and UEFilter both contained a SQL injection flaw. It has been fixed
UserDataVisibilityItemDataProvider was crashing when trying to provide data to a non-authenticated user. PATCH /users/{id} didn't have the skip_null_values parameter set to false. On creation,
UserInfos::$birthday was not set to a round day (it was set to the current time, not the current day)
Documented new folders in the tests/ folder in the README file. Documented class EtuUTTApiTestCase (and its methods). Documented test classes
Co-authored-by: Thomas Ritaine <[email protected]>
…y fakerphp/faker (#38)
…e current version Some code (in the filters) was outdated and not updated during the rebase
…breaking changes were fixed Most notable changes are the change of the version of Symfony, ApiPlatform and PHP
…ange in UserDataVisibilityItemDataProvider
…t passed to the voter
…oter was not applied here
e720954
to
25f178b
Compare
|
Rebase fait sur la dev |
|
Pour vos commandes, vous pouvez spécifier directement la mémoire souhaitée : |
Oui et non j'imagine. Oui dans le sens où il n'y aurait plus d'erreur, non au sens où si la quantité de mémoire utilisée est proportionnelle au nombre de tests, ça risque de vite partir en sucette. Mais ça reste utile de savoir ça (surtout si c'est pas possible de réduire la mémoire utilisée), merci ! |
…TTApiTestCase to create a new client
Description
Checklist
Test
Implementation
dd()|dump()Tools
srcfolder.Documentation
README.mdfile.