Project

General

Profile

Development #67126

tests : revoir les valeurs de timeout sur exécution en parallèle des tests

Added by Paul Marillonnet 7 months ago. Updated about 2 months ago.

Status:
Fermé
Priority:
Normal
Category:
-
Target version:
-
Start date:
07 July 2022
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Je tourne en local les tests unitaires avec NUMPROCESSES=4 (sur mon CPU i3 2017 2.3GHz un peu maigrichon), j’imagine que ça augmente le temps d’exécution de certaines parties de tests unitaires car l’une des valeurs de timeout testées dans le code ne semble plus adaptée :

    @pytest.mark.parametrize('encoding', ['utf-8-sig', 'cp1252', 'iso-8859-15'])
    def test_user_import(encoding, transactional_db, app, admin, ou1, admin_ou1):
        # […]
        response = response.click('Users Import')
        response = response.forms['action-form'].submit(name='execute')

        execute = list(report for report in _import.reports if not report.simulate)[0]
        uuid = execute.uuid

        response = response.follow()
>       response = assert_timeout(2, wait_finished)

tests/test_user_manager.py:544: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

duration = 2, wait_function = <function test_user_import.<locals>.wait_finished at 0x7f3c0e893400>

    def assert_timeout(duration, wait_function):
        start = time.time()
        while True:
            result = wait_function()
            if result is not None:
                return result
>           assert time.time() - start < duration, '%s timed out after %s seconds' % (wait_function, duration)
E           AssertionError: <function test_user_import.<locals>.wait_finished at 0x7f3c0e893400> timed out after 2 seconds
E           assert (1657193683.4672816 - 1657193681.4553237) < 2
E            +  where 1657193683.4672816 = <built-in function time>()
E            +    where <built-in function time> = time.time

Peut-être sans partir dans une valeur calculée en fonction de NUMPROCESSES, je serais pour simplement augmenter un peu, par exemple passer à quatre secondes au lieu de deux.


Files

History

#1

Updated by Paul Marillonnet 7 months ago

#2

Updated by Agate Berriot 7 months ago

pas évident de tester sur des durées de processing sur des machines avec des perfs et une charge variable :x

#3

Updated by Benjamin Dauvergne 7 months ago

Agate Berriot a écrit :

pas évident de tester sur des durées de processing sur des machines avec des perfs et une charge variable :x

On peut mettre quelque chose d'assez long, normalement ça s'arrête bien avant, mais on pourrait commencer par un time.sleep(0.1) et changer le time.sleep(0.001) dans la boucle en time.sleep(1) pour éviter une attente trop active et charger encore plus la machine.

#4

Updated by Agate Berriot 7 months ago

Benjamin Dauvergne a écrit :

Agate Berriot a écrit :

pas évident de tester sur des durées de processing sur des machines avec des perfs et une charge variable :x

On peut mettre quelque chose d'assez long, normalement ça s'arrête bien avant, mais on pourrait commencer par un time.sleep(0.1) et changer le time.sleep(0.001) dans la boucle en time.sleep(1) pour éviter une attente trop active et charger encore plus la machine.

j'ai peut être mal compris l'intention derrière ce test. Est-ce qu'on cherche à s'assurer que la vue appelée répond suffisamment vite? Si oui, augmenter le timeout réduit fortement l'intérêt du test.

Pour moi ce genre de questions de performances se mesurent plutôt à un niveau micro (par exemple avec un assert sur le nombre de requêtes SQL, auquel cas la charge de la machine ne change rien), soit de bout en bout, avec un outil comme Sentry, Datadog, New Relic, qui permet d'évaluer les évolutions de performances en prod et sur des jeux de données réalistes.

(mais bon, là c'est plus une discussion sur le fond, sur la forme, ta solution me semble okay pour que les tests arrêtent de planter)

#5

Updated by Agate Berriot 7 months ago

  • Status changed from Solution proposée to Solution validée
#6

Updated by Paul Marillonnet 7 months ago

Agate Berriot a écrit :

j'ai peut être mal compris l'intention derrière ce test. Est-ce qu'on cherche à s'assurer que la vue appelée répond suffisamment vite? Si oui, augmenter le timeout réduit fortement l'intérêt du test.

Non l’intention ici c’était de modérer par une valeur que l’on estime toujours acceptable pour le test de cet import, tout en prévenant l’échec du test en local sur une machine ancienne un peu (sur)chargée. Mais l’approche est paresseuse je le reconnais. Sinon tout aussi paresseusement je peux passer une turbine dernière génération en note de frais :)

Pour moi ce genre de questions de performances se mesurent plutôt à un niveau micro (par exemple avec un assert sur le nombre de requêtes SQL, auquel cas la charge de la machine ne change rien), soit de bout en bout, avec un outil comme Sentry, Datadog, New Relic, qui permet d'évaluer les évolutions de performances en prod et sur des jeux de données réalistes.

Oui le nombre de requêtes pourrait être une autre approche, cependant ici pour l’import csv il a tout plein d’objets qui sont créés en mémoire (voir les classes Csv* de authentic2.csv_import) et qui échappent à ça. Il faudrait un autre indicateur que le simple nombre de requêtes.

#7

Updated by Benjamin Dauvergne 7 months ago

Paul Marillonnet a écrit :

Agate Berriot a écrit :

j'ai peut être mal compris l'intention derrière ce test. Est-ce qu'on cherche à s'assurer que la vue appelée répond suffisamment vite? Si oui, augmenter le timeout réduit fortement l'intérêt du test.

Non l’intention ici c’était de modérer par une valeur que l’on estime toujours acceptable pour le test de cet import, tout en prévenant l’échec du test en local sur une machine ancienne un peu (sur)chargée. Mais l’approche est paresseuse je le reconnais. Sinon tout aussi paresseusement je peux passer une turbine dernière génération en note de frais :)

Oui enfin là c'est surtout que l'import est lancé dans un thread (avec forcément le mode transactional_db) et donc on ne peut pas faire grand chose que d'attendre que le thread soit fini, un peu mochement en pollant l'interface web comme le ferait un utilisateur.

On pourrait monkeypatcher threading.Thread pour faire autre chose (garder la fonction à appeler dans un coin puis la lancer dans le test pour que tout soit "monothread"), ou alors complètement simulé l'import pour rendre ça plus unitaire (et tester la fonction d'import ailleurs pareil unitairement). Certains tests sont plus des tests d'intégration qu'unitaire parce que des fois c'est juste plus simple de tester un truc de bout en bout que de découper en petit morceau; mais sûr un refactoring est possible.

#8

Updated by Agate Berriot 7 months ago

Non l’intention ici c’était de modérer par une valeur que l’on estime toujours acceptable pour le test de cet import, tout en prévenant l’échec du test en local sur une machine ancienne un peu (sur)chargée. Mais l’approche est paresseuse je le reconnais. Sinon tout aussi paresseusement je peux passer une turbine dernière génération en note de frais :)

C'est plus clair merci !

#9

Updated by Paul Marillonnet about 2 months ago

  • Status changed from Solution validée to Fermé

Finalement il s’est passé des choses côté travail d’exécution parallèle des tests, et je ne reproduis plus en local. Je ferme le ticket.

Also available in: Atom PDF