entrouvert
/
chrono
14
1
Fork 0
Code Issues Pull Requests 7 Releases Wiki Activity

tests: allow tests to run in random order on multiple processes (#89097) #243

Merged
yweber merged 3 commits from wip/89097-random-test-order-multiple-processes into main 2024-04-15 15:21:41 +02:00
Conversation 0 Commits 3 Files Changed 3 +13 -2
yweber commented 2024-04-04 14:35:19 +02:00
Owner
Copy Link
No description provided.
yweber added 3 commits 2024-04-04 14:35:19 +02:00
yweber force-pushed wip/89097-random-test-order-multiple-processes from e567b8064b to d6d8f81b73 2024-04-04 14:41:03 +02:00 Compare
yweber changed title from WIP: tests: allow tests to run in random order on multiple processes to tests: allow tests to run in random order on multiple processes 2024-04-04 14:45:54 +02:00
yweber changed title from tests: allow tests to run in random order on multiple processes to tests: allow tests to run in random order on multiple processes (#89097) 2024-04-04 14:46:06 +02:00
bdauvergne requested changes 2024-04-04 15:08:57 +02:00
Dismissed
tests/api/conftest.py Outdated
@ -32,2 +32,4 @@
def time_zone(request, settings):
settings.TIME_ZONE = request.param
yield request.param
get_default_timezone.cache_clear()
bdauvergne commented 2024-04-04 15:08:53 +02:00
Owner
Copy Link

Je préférerai qu'on utilise la même solution que dans le code de Django:

from django.core.signals import setting_changed

@receiver(setting_changed)
def update_connections_time_zone(**kwargs):
    if kwargs['setting'] == 'TIME_ZONE':
        # Reset local time zone cache
        chrono.utils.timezone.get_default_timezone.cache_clear()
Je préférerai qu'on utilise la même solution que dans le code de Django: ``` from django.core.signals import setting_changed @receiver(setting_changed) def update_connections_time_zone(**kwargs): if kwargs['setting'] == 'TIME_ZONE': # Reset local time zone cache chrono.utils.timezone.get_default_timezone.cache_clear() ```
yweber commented 2024-04-04 15:29:06 +02:00
Author
Owner
Copy Link

Bonne idée ! Merci pour la relecture, et encore merci pour le démêlage de des erreurs de ce ticket :)

Bonne idée ! Merci pour la relecture, et encore merci pour le démêlage de des erreurs de ce ticket :)
yweber force-pushed wip/89097-random-test-order-multiple-processes from d6d8f81b73 to 5469af9b17 2024-04-04 15:27:45 +02:00 Compare
bdauvergne approved these changes 2024-04-04 15:31:43 +02:00
Dismissed
yweber force-pushed wip/89097-random-test-order-multiple-processes from 5469af9b17 to cee359e47c 2024-04-04 15:32:57 +02:00 Compare
yweber force-pushed wip/89097-random-test-order-multiple-processes from cee359e47c to 3be5981394 2024-04-04 15:34:24 +02:00 Compare
fpeters reviewed 2024-04-04 15:36:31 +02:00
tests/api/conftest.py Outdated
@ -29,3 +29,3 @@
@pytest.fixture(params=['Europe/Brussels', 'Asia/Kolkata', 'Brazil/East'])
@pytest.fixture(params=['Europe/Brussels', 'Asia/Kolkata', 'America/Sao_Paulo'])
fpeters commented 2024-04-04 15:36:31 +02:00
Owner
Copy Link

Mais ça décale ou pousse sous le tapis un problème, ça; non ?

Mais ça décale ou pousse sous le tapis un problème, ça; non ?
yweber commented 2024-04-04 15:43:27 +02:00
Author
Owner
Copy Link

Je n'en suis pas certain, j'avais plutôt l'impression que les tests étaient trop permissifs et réussissaient à charger les infos d'une timezone non supporté par zoneinfo.

La, avec plusieurs processus et sans le --dist loadfile les tests partent en erreur avec zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Brazil/East' et cette erreur me semblait "légitime" dans le sens ou, effectivement, la zone Brazil/East n'existe pas dans /usr/share/zoneinfo

Mais peut être (sûrement) qu'il y a quelque-chose qui m'échappe.

Je n'en suis pas certain, j'avais plutôt l'impression que les tests étaient trop permissifs et réussissaient à charger les infos d'une timezone non supporté par zoneinfo. La, avec plusieurs processus et sans le `--dist loadfile` les tests partent en erreur avec `zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Brazil/East'` et cette erreur me semblait "légitime" dans le sens ou, effectivement, la zone `Brazil/East` n'existe pas dans `/usr/share/zoneinfo` Mais peut être (sûrement) qu'il y a quelque-chose qui m'échappe.
fpeters commented 2024-04-04 15:46:19 +02:00
Owner
Copy Link

La, avec plusieurs processus et sans le --dist loadfile les tests partent en erreur avec zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Brazil/East' et cette erreur me semblait "légitime" dans le sens ou, effectivement, la zone Brazil/East n'existe pas dans /usr/share/zoneinfo

Ok c'est une erreur qui n'était pas notée plus haut. (et je pense que ça signifie qu'en local il te manque tzdata-legacy).

> La, avec plusieurs processus et sans le --dist loadfile les tests partent en erreur avec zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Brazil/East' et cette erreur me semblait "légitime" dans le sens ou, effectivement, la zone Brazil/East n'existe pas dans /usr/share/zoneinfo Ok c'est une erreur qui n'était pas notée plus haut. (et je pense que ça signifie qu'en local il te manque tzdata-legacy).
yweber commented 2024-04-04 15:51:08 +02:00
Author
Owner
Copy Link

(et je pense que ça signifie qu'en local il te manque tzdata-legacy).

Bien vu !

Du coup d'après toi c'est quoi la solution à privilégier ?

  • laisser Brazil/East et ajouter tzdata-legacy dans les Build-Depends
  • changer en America/Sao_Paulo
> (et je pense que ça signifie qu'en local il te manque tzdata-legacy). Bien vu ! Du coup d'après toi c'est quoi la solution à privilégier ? - laisser Brazil/East et ajouter tzdata-legacy dans les Build-Depends - changer en America/Sao_Paulo
fpeters commented 2024-04-04 16:08:49 +02:00
Owner
Copy Link

Avec le fin de mot de l'histoire je dirais que le commit peut être laissé en l'état, en mentionnant dans son message que c'est parce que America/Sao_Paulo n'est plus disponible par défaut.

Avec le fin de mot de l'histoire je dirais que le commit peut être laissé en l'état, en mentionnant dans son message que c'est parce que America/Sao_Paulo n'est plus disponible par défaut.
bdauvergne commented 2024-04-04 16:14:37 +02:00
Owner
Copy Link

Mais ça décale ou pousse sous le tapis un problème, ça; non ?

L'absence d'invalidation du cache faisait que le problème était invisible sauf en lançant le test dans plusieurs process ou là chrono.utils.timezone.get_default_timezone() tentait bien de charger Brazil/East mais il y avait bien deux problèmes. C'était invisible pour tous les appels via l'infrastructure de Django (django.utils.timezone.get_default_timezone()) qui utilise pytz qui contient sa propre base de timezone.

> Mais ça décale ou pousse sous le tapis un problème, ça; non ? L'absence d'invalidation du cache faisait que le problème était invisible sauf en lançant le test dans plusieurs process ou là chrono.utils.timezone.get_default_timezone() tentait bien de charger Brazil/East mais il y avait bien deux problèmes. C'était invisible pour tous les appels via l'infrastructure de Django (django.utils.timezone.get_default_timezone()) qui utilise pytz qui contient sa propre base de timezone.
yweber commented 2024-04-04 16:25:15 +02:00
Author
Owner
Copy Link

Merci à tous les deux :)

Merci à tous les deux :)
yweber marked this conversation as resolved
yweber reviewed 2024-04-04 15:36:32 +02:00
tests/conftest.py Outdated
@ -25,1 +29,4 @@
}
@receiver(setting_changed)
yweber commented 2024-04-04 15:36:32 +02:00
Author
Owner
Copy Link

J'ai testé en lançant les tests avec tox -e py3-django32-codestyle-coverage -- tests/api/ -n 16, à priori le receiver est bien pris en compte. Du coup j'imagine que c'est mieux qu'il soit actif pour l'ensemble des tests non ?

J'ai testé en lançant les tests avec `tox -e py3-django32-codestyle-coverage -- tests/api/ -n 16`, à priori le receiver est bien pris en compte. Du coup j'imagine que c'est mieux qu'il soit actif pour l'ensemble des tests non ?
bdauvergne commented 2024-04-04 16:11:52 +02:00
Owner
Copy Link

Oui le plus tôt possible effectivement.

Oui le plus tôt possible effectivement.
yweber commented 2024-04-04 16:25:00 +02:00
Author
Owner
Copy Link

Ok, merci !

Ok, merci !
yweber marked this conversation as resolved
yweber force-pushed wip/89097-random-test-order-multiple-processes from 3be5981394 to 37ad42c2b2 2024-04-04 16:19:25 +02:00 Compare
yweber force-pushed wip/89097-random-test-order-multiple-processes from 37ad42c2b2 to 81b92bcc35 2024-04-04 16:21:11 +02:00 Compare
yweber force-pushed wip/89097-random-test-order-multiple-processes from 81b92bcc35 to 77bda29128 2024-04-04 16:24:38 +02:00 Compare
yweber requested review from bdauvergne 2024-04-04 16:31:38 +02:00
bdauvergne approved these changes 2024-04-04 17:09:35 +02:00
yweber force-pushed wip/89097-random-test-order-multiple-processes from 77bda29128 to 5a90c4851b 2024-04-15 15:08:11 +02:00 Compare
yweber merged commit 5a90c4851b into main 2024-04-15 15:21:41 +02:00
yweber deleted branch wip/89097-random-test-order-multiple-processes 2024-04-15 15:21:41 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
bdauvergne
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/chrono#243
No description provided.
Delete Branch "wip/89097-random-test-order-multiple-processes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?