Skip to content
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

Retirer pytest-timeout #3805

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Retirer pytest-timeout #3805

merged 1 commit into from
Mar 20, 2024

Conversation

francoisfreitag
Copy link
Contributor

Pourquoi ?

Xavier vient de perdre au moins une journée à cause de pytest-timeout qui crashait les workers sur GitHub actions sans information par rapport à un setup trop long.

Nous avons déjà augmenté plusieurs fois les timeouts pour essayer de les faire marcher sur les Macs, finalement c’est à chacun de définir sa limite en fonction de sa machine.

D’autres ont rencontré le problème :

L’investissement dev ne vaut la (faible) sécurité ajoutée.

Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
44836382e21bf306686926041f47bae5

@rsebille
Copy link
Contributor

+0.

Pour l'historique ça avais été ajouté car on avais un certain nombre de test qui faisait des appels réseaux (avec parfois un @tenacity.retry ou équivalent) et donc on tapais les 5-10 secondes de timeout réseau en boucle, et c'était resté pendant un certain temps car personne ne s'amuse à regarder le temps individuel des tests ni n'est trop choqué quand la suite de test prend un peu plus longtemps que dans ses souvenirs vu qu'on ajoute toujours des tests et que ça peux être passager.

Mais on n'est effectivement assez peu susceptible d'avoir des boucles infinies ou du code bloquant, par contre l'avantage pour nous c'est que ça limitais le temps maximum du setup de la DB mais au final on a plutôt augmenté la valeur dans la conf plutôt que squash les migrations quand ça arrivais 😅.

It was originally added to prevent tests from taking unnecessarily long,
as some would `@tenacity.retry` for unreachable targets.
However, the originally short timeout that would reveal such issues was
bumped multiple times, as it started failing on slow developers
machines, and became less useful.

The plugin now prevents the test suite from running by default on Macs,
adding friction to the development setup. It caused GitHub actions on
the [Django 5.0
PR](#3753) to fail
without hints at timeouts being reached. It took a few hours to
understand what caused the failure.

In the time it’s been there, it likely hasn’t caught an infinite loop.
If such problem were to happen, GitHub actions have timeouts. To
troubleshoot issues locally, it’s easy enough to set `-vv` and see what
test is hanging.
@francoisfreitag francoisfreitag added this pull request to the merge queue Mar 20, 2024
@francoisfreitag francoisfreitag removed this pull request from the merge queue due to a manual request Mar 20, 2024
@francoisfreitag francoisfreitag added this pull request to the merge queue Mar 20, 2024
Merged via the queue into master with commit 421ecc0 Mar 20, 2024
8 checks passed
@francoisfreitag francoisfreitag deleted the ff/pytest-timeout branch March 20, 2024 09:17
@francoisfreitag francoisfreitag added the no-changelog Ne doit pas figurer dans le journal des changements. label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Ne doit pas figurer dans le journal des changements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants