Projet

Général

Profil

Development #67126

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

Ajouté par Paul Marillonnet il y a plus d'un an. Mis à jour il y a plus d'un an.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Catégorie:
-
Version cible:
-
Début:
07 juillet 2022
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

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.


Fichiers

Historique

#1

Mis à jour par Paul Marillonnet il y a plus d'un an

#2

Mis à jour par A. Berriot il y a plus d'un an

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

#3

Mis à jour par Benjamin Dauvergne il y a plus d'un an

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

Mis à jour par A. Berriot il y a plus d'un an

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

Mis à jour par A. Berriot il y a plus d'un an

  • Statut changé de Solution proposée à Solution validée
#6

Mis à jour par Paul Marillonnet il y a plus d'un an

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

Mis à jour par Benjamin Dauvergne il y a plus d'un an

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

Mis à jour par A. Berriot il y a plus d'un an

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

Mis à jour par Paul Marillonnet il y a plus d'un an

  • Statut changé de Solution validée à 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.

Formats disponibles : Atom PDF