-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(PC-31471) test: add assert_num_queries in public #13820
Conversation
5208bc4
to
53e1b20
Compare
7c2f4bb
to
81c51b7
Compare
api/tests/routes/public/individual_offers/v1/get_offerer_venue_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'avoue n'avoir que survolé la seconde moitié, car c'est un peu répétitif à lire :)
num_queries += 1 # select stock | ||
num_queries += 1 # select venue | ||
with testing.assert_num_queries(num_queries): | ||
response = client.with_basic_auth(pro_user.email).get(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attention à pro_user.email
, n'est-ce pas la raison de la première requête ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ça ma étonné aussi mais non, je l'ai quand même sorti du assert_num_queries pour que visuellement on soit sûr qu'il n'ajoute pas de requête
api/tests/routes/public/booking_token/v2/get_booking_by_token_v2_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/booking_token/v2/get_booking_by_token_v2_test.py
Show resolved
Hide resolved
d84c4a4
to
c38798f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quel est exactement le but recherché pour les assert_num_queries
?
Il me semble qu'il serait préférable de ne pas appliquer les assert_num_queries
à tout les tests d'un endpoint parce que ça complexifie la lecture du test.
Je serais plutôt partisan d'avoir pour chaque endpoint un test dédié aux assert_num_queries
sur un certain nombre de cas nominaux (200, 400, 404, 401) qui s'appellerait quelque chose comme test_num_queries
. Ça permettrait d'alléger les autres tests qui s'occupent de tester la logique métier et de les rendre plus clairs. Ainsi si jamais ma modification a changé le nombre query, il n'y aurait qu'un test rouge et qu'un test à fixer (ça allègerait la PR).
api/tests/routes/public/booking_token/v2/get_booking_by_token_v2_test.py
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_collective_offers_categories_test.py
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_collective_offers_categories_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_collective_offers_educational_domains_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_collective_offers_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_educational_institutions_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_educational_institutions_test.py
Outdated
Show resolved
Hide resolved
api/tests/routes/public/collective/endpoints/get_list_subcategories_test.py
Show resolved
Hide resolved
api/tests/routes/public/individual_offers/v1/get_event_categories_test.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mise à part les tests qui peuvent être retirés (parce que j'avais oublié de le faire 😇) 👍🏻
3d7bed0
to
b14b32a
Compare
b14b32a
to
ffc4d7b
Compare
But de la pull request
Ticket Jira (ou description si BSR) : https://passculture.atlassian.net/browse/PC-31471