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

[BUGFIX] Le InMemoryTemporaryStorage ne suit pas la TemporaryStorage API et est utilisé à tort dans les tests d'intégration (PIX-12551) #8954

Merged

Conversation

lego-technix
Copy link
Contributor

@lego-technix lego-technix commented May 17, 2024

🦄 Problème

Le InMemoryTemporaryStorage ne suit pas la TemporaryStorage API et est utilisé à tort dans les tests d'intégration.

Une conséquence de cela est que le PoleEmploiOidcAuthenticationService et le FwbOidcAuthenticationService sont testés avec une mauvaise implémentation et leurs comportements sont dysfonctionnels concernant la méthode getRedirectLogoutUrl qui ne fournit pas un'id_token_hint.

🤖 Proposition

  1. Faire suivre la TemporaryStorage API au InMemoryTemporaryStorage en rendant les méthodes du second asynchrones comme le sont celles du premier
  2. Pour les tests d'intégration utiliser le RedisTemporaryStorage à la place du InMemoryTemporaryStorage, car les tests d'intégration servent à tester les composants dans le cadre d'un environnement où l'infrastructure est la plus proche possible de celle de la production

🌈 Remarques

Il faudrait également que les tests d'intégration mettant en jeu le LearningContentCache utilisent Redis au lieu d'utiliser uniquement le InMemoryCache.

Pour cela il faudrait mettre dans api/src/shared/config.js :

config.caching.redisUrl = process.env.TEST_REDIS_URL;

Et dans api/tests/acceptance/application/cache/cache-controller_test.js il faudrait utiliser config.caching.redisUrl au lieu d'utiliser process.env.TEST_REDIS_URL directement.

Mais cette correction s'est avérée difficile et on n'a pas réussi à en venir à bout dans un temps limité et nous avons abandonné cette correction qui n'est pas dans le scope de l'équipe Accès.

💯 Pour tester

  1. Vérifier que la CI s'exécute sans erreur
  2. Vérifier aussi en local que les tests s'exécutent sans erreur
    • Vérifier en local que les tests unitaires utilisent le InMemoryTemporaryStorage (pour cela on peut arrêter le conteneur de Redis – docker stop <container_id> – et constater que les tests unitaires continuent de fonctionner sans erreur)
    • Vérifier en local que les tests d'intégration et d'acceptance utilisent le RedisTemporaryStorage (pour cela on peut arrêter le conteneur de Redis – docker stop <container_id> – et constater que les tests d'intégration sont en erreur)
  3. Vérifier que la déconnexion pour France Travail et la FWB déconnecte bien l'utilisateur du SSO du fournisseur d'identité

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@lego-technix lego-technix marked this pull request as ready for review May 17, 2024 06:57
@lego-technix lego-technix marked this pull request as draft May 17, 2024 09:52
@lego-technix lego-technix force-pushed the pix-12551-fix-id-token-hint-retrieval-regression branch from 288ca86 to c6b5c2b Compare May 17, 2024 10:25
@lego-technix lego-technix changed the title [BUGFIX] Le InMemoryTemporaryStorage ne suis pas la TemporaryStorage API et est utilisé à tort dans les tests d'intégration (PIX-12551) [BUGFIX] Le InMemoryTemporaryStorage ne suit pas la TemporaryStorage API et est utilisé à tort dans les tests d'intégration (PIX-12551) May 17, 2024
@lego-technix lego-technix force-pushed the pix-12551-fix-id-token-hint-retrieval-regression branch 2 times, most recently from 372180f to 3186f4b Compare May 20, 2024 17:41
@lego-technix lego-technix marked this pull request as ready for review May 20, 2024 18:16
@EmmanuelleBonnemay
Copy link
Contributor

En ce qui concerne les tests unitaires, j'ai vérifié de la même manière que pour les tests d'intégration (arrêter le conteneur Redis et vérifier que les tests unitaires n'ont pas besoin de ce conteneur pour s'exécuter). Est-ce que cela suffit? Il n'y a pas de log spécifique au InMemoryTemporaryStorage en console :

> pix-api@4.146.0 test:api:unit
> TEST_DATABASE_URL=postgres://should.not.reach.db.in.unit.tests TEST_REDIS_URL= npm run test:api:path -- 'tests/**/unit/**/*test.js'

@lego-technix
Copy link
Contributor Author

lego-technix commented May 21, 2024

En ce qui concerne les tests unitaires, j'ai vérifié de la même manière que pour les tests d'intégration (arrêter le conteneur Redis et vérifier que les tests unitaires n'ont pas besoin de ce conteneur pour s'exécuter). Est-ce que cela suffit? Il n'y a pas de log spécifique au InMemoryTemporaryStorage en console :

Tout à fait. Il n'y a pas de logs spécifiques au InMemoryTemporaryStorage en console. Donc on peut soit tester en arrêtant/redémarrant les conteneurs Docker, soit en ajoutant soi-même des logs dans le InMemoryTemporaryStorage et le RedisTemporaryStorage.

Copy link
Contributor

@EmmanuelleBonnemay EmmanuelleBonnemay left a comment

Choose a reason for hiding this comment

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

Lu et testé fonctionnellement en local avec succès sur Firefox.

@pix-service-auto-merge pix-service-auto-merge force-pushed the pix-12551-fix-id-token-hint-retrieval-regression branch 2 times, most recently from b8b59ee to 0ee8204 Compare May 22, 2024 07:51
lego-technix and others added 5 commits May 22, 2024 07:59
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com>
Co-authored-by: LEGO Technix <109212476+lego-technix@users.noreply.github.com>
Co-authored-by: Eric Lim <eric.lim@pix.fr>
Co-authored-by: Emmanuelle Bonnemay <emmanuelle.bonnemay@pix.fr>
Co-authored-by: Marianne Bost <marianne.bost@pix.fr>
Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com>
Co-authored-by: LEGO Technix <109212476+lego-technix@users.noreply.github.com>
Co-authored-by: Eric Lim <eric.lim@pix.fr>
Co-authored-by: Emmanuelle Bonnemay <emmanuelle.bonnemay@pix.fr>
Co-authored-by: Marianne Bost <marianne.bost@pix.fr>
wrt the REDIS_URL environment variable.
rather use RedisTemporaryStorage instead, which is the true infrastructure
component used in production.

Co-authored-by: Ismael Gorissen <ismael.gorissen@gmail.com>
Co-authored-by: LEGO Technix <109212476+lego-technix@users.noreply.github.com>
Co-authored-by: Eric Lim <eric.lim@pix.fr>
Co-authored-by: Emmanuelle Bonnemay <emmanuelle.bonnemay@
@pix-service-auto-merge pix-service-auto-merge force-pushed the pix-12551-fix-id-token-hint-retrieval-regression branch from 0ee8204 to b2f47d6 Compare May 22, 2024 07:59
@pix-service-auto-merge pix-service-auto-merge merged commit 26e8f32 into dev May 22, 2024
5 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-12551-fix-id-token-hint-retrieval-regression branch May 22, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants