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

Ne plus stopper la BEAM pendant les tests #4417

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

thbar
Copy link
Contributor

@thbar thbar commented Jan 16, 2025

Le correctif introduit dans #4377 (#4405) arrête complètement le programme dans certaines conditions. Or je n'avais pas vu que le plug correspondant est sous tests (#4399).

La conséquence est que la suite de test est interrompue au "moment où ce test tourne", et le tout avec un code d'erreur zéro, qui fait que ça ne se remarque que difficilement.

Par conséquent, dans cette PR :

  • Je remplace l'appel à System.stop() par un raise avec un message clair, uniquement pendant les tests
  • Je modifie le test associé, et j'ajoute de la documentation, car ce comportement est important pour le bon fonctionnement de la prod
  • Je modifie (y compris en production mais je ne pense pas que ça portera à conséquence et j'aurais gagné à faire ça en premier lieu dans Hot-fix: shutdown du worker si les jobs ne sont plus traités #4405) l'exit code associé, pour retourner un non-zero plutôt que zero, de façon à ce que ça soit détecté si jamais ça devenait revenir, dès la CI

À merger dès que possible, car concrètement ça veut dire qu'une partie des tests sont bypassés dans nos PR actuelles !

Also, return a non-zero exit code to provide a extra hint which would have helped me here.
@thbar thbar requested a review from a team as a code owner January 16, 2025 10:08
@ptitfred
Copy link
Contributor

Ah! ça explique pourquoi je n'avais plus le résultat des tests. Je pensais avoir cassé quelque chose dans mon setup pendant les vacances.

else
# Also make sure to return with a non-zero exit code, to more clearly
# indicate that this is not the normal output
System.stop(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-ce que Clever-Cloud est "cool" avec un exit code != 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'y ai réfléchi ; ça pourrait avoir un effet (du type monitoring qui déclenche autre chose), toutefois ils n'ont pas évoqué un fonctionnement de ce type.

Je peux tout à fait leur mettre une note (déjà pour leur dire que ce système fonctionne, il y a eu un redémarrage unique cette semaine), et leur demander, si on veut s'assurer de ça.

@ptitfred ptitfred self-assigned this Jan 16, 2025
@thbar
Copy link
Contributor Author

thbar commented Jan 16, 2025

Ah! ça explique pourquoi je n'avais plus le résultat des tests. Je pensais avoir cassé quelque chose dans mon setup pendant les vacances.

Mes excuses ! J'ai compris en ayant besoin du coverage, et en ne le voyant pas se rafraîchir...

@thbar
Copy link
Contributor Author

thbar commented Jan 16, 2025

Concernant l'exit-code : si le non-zero pose problème, je serai quand même notifié via PagerDuty, et je modifierai.

Merci @ptitfred pour la review !

@thbar thbar added this pull request to the merge queue Jan 16, 2025
Merged via the queue into master with commit 5b1f768 Jan 16, 2025
4 checks passed
@thbar thbar deleted the fix/do-no-halt-test-suite branch January 16, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants