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

✅ Airflow tests e2e: utilitaires & preuve de concept #1396

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

maxcorbeau
Copy link
Contributor

@maxcorbeau maxcorbeau commented Feb 27, 2025

✅ Airflow tests e2e: utilitaires & preuve de concept

Carte Notion: Airflow: tests automatisés DAG de bout-en-bout

🗺️ contexte: DAGs airflow qui se complexifient, on casse souvent les DAGs car les tests unitaires sont insuffisants

💡 quoi: utilitaires & preuve de concept de tests e2e airflow

🎯 pourquoi: améliorer la robustesse de nos tests

🤔 comment:

  • Changements dans dags:
    • utils/django.py: modifs pour retourner les infos DB (port, host, name) quand on fait django_setup_full ce qui m'a permis de tester que la connexion DB test marchait bien
    • templates/dags/template_read_db.py: un template de DAG pour démontrer comment ça marche et valider tous les utilitaires e2e (puisque ce template réspecte l'arborescence du projet)
  • Changements dans dags_unit_tests/:
    • Sous dossier e2e/ dédié/isolé: car les tests airflow e2e prennent du temps, ceci va permettre en dev & ci de lancer uniquement les tests souhaités dans l'ordre souhaités
    • e2e/utils.py : utilitaires pour travailler avec Airflow (init de l'environement, récupérer un DAG, une instance de tâche...)
    • e2e/test_e2e_dag_*.py : préfixer tous les tests de cette manière pour les distinguer & les chercher dans l’IDE plus facilement

🖼️ Preuve de concept

Simple mais suffisante:

  1. Création d'acteurs hors Airflow via nos models Django qfdmo.models
  2. Airflow capables de lire ces données via django_setup_full puis les mêmes modèles
    • 🔴 A noter que le XCom Airflow à des limitations niveau sérialization (ex: échanger des queryset entre les tâches ne fonctionne pas), donc bien tester le XCOM pour voir ce qui passe ou pas
    • 🔵 On montre comment utiliser dag.test(run_conf=) pour passer une config de DAG, notamment des valeurs qui seront disponibles dans le params des tâches Airflow

📆 Prochaines PR

Documentation: j'ai enlevé des explications de code que je mettre dans des PRS de docs/

Soumettre des tests e2e pour DAGs en cours surtout avant de leur apporter des changements importants:

@maxcorbeau maxcorbeau added python Pull requests that update Python code technical labels Feb 27, 2025
@maxcorbeau maxcorbeau requested a review from a team as a code owner February 27, 2025 10:30
@maxcorbeau maxcorbeau requested review from kolok and fabienheureux and removed request for a team February 27, 2025 10:30
Copy link
Contributor

@kolok kolok left a comment

Choose a reason for hiding this comment

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

Quelques commentaires pour commencer :)

Comment on lines +21 to +24
"""Adds Django project root to sys.path, based on
current directory structure:
- core/ = Django root
- dags/utils/django.py"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Adds Django project root to sys.path, based on
current directory structure:
- core/ = Django root
- dags/utils/django.py"""
"""
Adds Django project root to sys.path, based on current file path
"""

django_add_to_sys_path()
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "core.airflow_settings")

import django

django.setup()

from django.conf import settings
Copy link
Contributor

Choose a reason for hiding this comment

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

on peut mettre cet import dans django_settings_to_dict

django_add_to_sys_path()
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "core.airflow_settings")

import django

django.setup()

from django.conf import settings

return django_settings_to_dict(settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pas sur que ce soit util de renvoyer ces infos au load

Pourquoi ne pas appeler directement django_settings_to_dict là où tu as besoin de ces info ?

Comment on lines 29 to 32
def django_settings_to_dict(settings) -> dict:
"""Returns useful information from settings
as a JSON-compatible dict to help us debug
(e.g. when setting up e2e Airflow tests)"""
Copy link
Contributor

Choose a reason for hiding this comment

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

En vrai, ici, tu retourne les info de la connexion à la DB uniquement

Suggested change
def django_settings_to_dict(settings) -> dict:
"""Returns useful information from settings
as a JSON-compatible dict to help us debug
(e.g. when setting up e2e Airflow tests)"""
def get_django_db_connexion_settings(settings) -> dict:
"""
Returns database connection informations from Django settings as a dict to help us debug
(e.g. when setting up e2e Airflow tests)
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui, mais c'est une abstraction pour avoir à renommer en permanence la fonction au fur et à mesure qu'on lui ajoute des infos

Copy link
Contributor

Choose a reason for hiding this comment

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

Plutôt que mettre les test e2e dans dags_unit_tests, est-ce qu'on a pas intérêt à les mettre dans dags_e2e_tests ?

Plus globalement, ce serait pas mal de répartir les DAGs dans les sous-dossiers. Mettre es test des dags dans le dossier dags qui sera renommer bientôt ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests that update Python code technical
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants