Structure du talk

[julien-topcu]

Déroulé du talk

0. Prérequis

Liens de référence:

• Slides
• Speaker view

Étapes de préparation pré-live-coding:

• Mettre VSCode en thème "fond clair".

• Installer Snippet - Visual Studio Marketplace pour VSCode.

• Créer snippet "basic functional test" avec Snippet - Visual Studio Marketplace:

  contenu du snippet

  // @ts-check
  
  // run with: $ npx mocha test/functional/createPlaylist.functional.tests.js
  
  const assert = require('assert');
  const { createPlaylist } = require('../../app/domain/features');
  
  describe('playlist', () => {
    it('should be created by a user without playlist', async () => {
      const playlistName = 'summer mega mix 2022';
      const playlist = await createPlaylist('userWithoutPlaylist', playlistName);
      assert.equal(playlist.id, 0);
      assert.equal(playlist.name, playlistName);
    });
  
    it('should be created by a user with playlist', async () => {
      const playlistName = 'summer mega mix 2023';
      const playlist = await createPlaylist('userWithPlaylist', playlistName);
      assert.equal(playlist.id, 1);
      assert.equal(playlist.name, playlistName);
    });
  });

• Préparer l'environnement de live coding, dans le terminal de VSCode:

  nvm use 
  npm install
  git checkout migration-start # ancienne version: migration-start-devoxx-2022
  docker compose up -d mongo
  . ./env-vars-testing.sh 
  export MONGODB_PORT=27117 
  npm run test-reset

1. Faire une démo d'OpenWhyd

Adrien

• La démo
• Décrire le problème : Les contributeurs ont lâché l'affaire car trop fragile et trop difficile à comprendre
• Survol de la code base :
  • Partir de Application.js
  • préciser que: pas de typage
  • parler du framework maison => fichier app.route, notamment les api --> subdir --> controllers/api/post.js --> fonction intéressante: insertion d'un morceau de musique. (post Controller sur la méthode insert)

Objectifs:

• Permettre à quelqu'un qui n'a pas d'expérience sur la code base d'openWhyd d'être autonome dans la correction de bugs.
• Le but étant de maintenir l'existant, on veut améliorer sur place ! On ne veut pas faire une fuite en avant, en créant un module from scratch qui paraitrait plus simple à faire mais qui rajouterait de la complexité de maintenance sur le legacy

Donc => Migration in situ

2. Préparation du terrain opératoire: Biopsies Clean Codiennes

But : Y voir plus claire sur la code base
But caché (intention) : Montrer la fragilité de la code base

1. Expliquer que l'on vise de migrer l'action insert de controllers/api/post
2. Exécuter les tests d'intégration pour montrer qu'on en a :

npm run test:integration:post # avant, on lançait le serveur, puis: $ npm run test:integration

1. Changer dans controllers/api/post, p puis uId uNm de l'objet q dans insert. Et expliquer qu'on est en train de changer la structure en base de données.
2. (redémarrer serveur puis) Réexécuter les tests d'intégration et montrer que les messages d'erreurs ne permettent pas de voir ce qu'on a cassé

3. Ajout des approval tests

git checkout migration-start-with-approval-tests
npm install

1. Expliquer le principe des Approval tests, théorie + pratique.
2. Montrer un fichier d'approval de When posting a track
3. Ouvrir approval.tests.js et expliquer le test correspondant
4. Lancer le test :

npm run test:approval

1. Montrer dans les logs que l'on a un diff des approvals.
2. Supprimer la modification de userName et userId

4. Amener plus de lisibilité dans le code

Lisibilité

Adrien:

1. Survol de insert jusqu'à process playlist en expliquant le code
2. Montrer que insert a beaucoup de responsabilité => décider que la partie createPlaylist est déjà un scope suffisant à refactorer

Jordan:

1. Extraction de extractPlaylistRequest
2. Passage du bloc try-catch en pure fonction
3. postRequest.pl = extractPlaylistRequest(...) et dire que ça pourrait être fait directement à l'initialisation de postQuery.
4. Remplacer l'initialisation à undefined par extractPlaylistRequest(...)
5. Extraction needToCreatePlaylist dans le if
6. Comment rendre la séquentialité de createPlaylist plus explicite ?

Adrien:

1. Wrapper dans une Promise createPlaylist()
2. Virer le actualInsert() superflu

Diagnostic

• Julien : Mais il fait quoi ce createPlaylist ?
• Adrien : Présente userModel.createPlaylist()
• Jordan & Julien s'étonnent et commentent du couplage du métier dans la couche de persistence

5. Modéliser le domaine

1. Créer le répertoire domain
2. Créer le contrat d'entrée du domaine via l'api. Le domaine doit dépendre que des objets du domaine, mais il n'y pas de typage fort en JS.

• Julien : On peut migrer TypeScript ?
• Adrien : Trop compliqué mais y'a un autre moyen... Tu veux quoi comme types ?

4. Création du type CreatePlaylist dans api.ts
5. Création du type Playlistdans types.ts

• Julien : Mais du coup c'est du TypeScript, comment on va raccrocher les wagon sans migrer de langage ?
• Adrien : Avec la JSDoc

6. Refactorer l'appel de la Promise(createPlaylist) pour bien détourer la déclaration de la fonction createPlaylist

const createPlaylist = (userId, playlistName) =>
      new Promise((resolve) =>
        userModel.createPlaylist(userId, playlistName, resolve)
);

7. Appliquer le type CreatePlaylist via JSDoc puis activer la vérification avec ts-check
8. Montrer que l'on bénéficie du typage dans vscode via l'autocomplétion sur playlist et les warnings de "compilation" si on fait un playlist.toto
9. Extract de createPlaylist dans features.js dans le domaine.
10. Appliquer le ts-check dans features
11. Lancer les approval tests

6. Extraire la logique métier de la couche de persistence

1. Remontrer le code de userModel.createPlaylist() pour caractériser le métier qu'on veut migrer.
2. ⚠️ Créer un test fonctionnel (ex: test/functional/playlist.functional.test.js) pour figer les comportements que l'on a compris et expliquer qu'on ne s'appuie pas sur les Approvals, car le domaine est testé en isolation pure.
3. Appliquer le snippet basic functional test (clic droit + "insert snippet") et expliquer les tests
4. Supprimer la dépendance userModel dans features.createPlaylist() en la commentant
5. Résumer le code de userModel.createPlaylist() et écrire un "algo" dans features

  await fetch()
  playlist = {
    id:
    name: 
  }
  await save()
  return playlist

6. Créer spi.ts
7. Expliquer que les playlists sont stockées dans les utilisateurs, ce qui nécessite de créer un type UserRepository.
8. Ajouter une fonction getUserById qui retourne une Promise de User
9. Définir User dans types.ts
10. features a maintenant besoin d'une instance de UserRepository. Comme on ne veut pas se coupler avec le repository, on crée une factory createFeatures` pour d'injecter une instance.
11. Typer le paramètre userRepository de la factory
12. Appeler userRepository.getUserbyId dans createPlaylist
13. Retourner dans userModel et expliquer la logique de création d'un id de playlist, la copier-coller dans features et la nettoyer.
14. Julien à Adrien : est-ce qu'on doit sauvegarder tout le user pour sauvegarder une playlist ?
15. Définir une fonction insertPlaylist dans la SPI dans le UserRepository
16. L'appeler dans features
17. Replugguer dans le test fonctionnel via createFeatures
18. Expliquer la nécessité de créer un Stub de UserRepository
19. Créer le stub userRepository et la typer avec la SPI. Générer le squelette au moyen de l'IDE.
20. Créer une base d'utilisateurs dans le test pour initier le repository en reprenant les ids de user dans les tests fonctionnels
21. Implémenter les fonctions du Stubs
22. Enrichir les tests pour vérifier la persistance de la playlist

    //premier test
    assert.deepEqual(users[0].playlists, [playlist]);
   //deuxieme test
    assert.deepEqual(users[1].playlists[1], playlist);

23. Faire passer les tests ($ npx mocha test/functional/playlist.functional.test.js)

7. Recabler le domaine avec la persistance

1. Créer un répertoire infrastructure avec UserCollection.js
2. Typer UserCollection avec la SPI et générer le squelette
3. Retourner dans le controller et rebinder le domain avec une instance de UserCollection
4. Exécution des tests approvals pour vérifier que le câblage fonctionne
5. Implémenter UserCollection.getUserById()
6. Lancer les approvals montrer que ça casse à cause d'un problème de length sur les playlists. Expliquer la différence de modélisation entre la base et le domaine (playlists vs pl).
7. Définir UserDocument dans infrastructure/type.ts, typer le retour de la requête mongo dans getUserbyId et tenter de retourner le userDocument.
8. Créer la fonction de mapping mapToUser et expliquer le rôle de l'anti-corruption layer
9. Relancer les tests d'approval pour montrer que le problème n'est plus là
10. Implémenter insertPlaylist
11. Montrer que les tests d'approval ne fonctionnent toujours pas à cause de l'inconsistance des données en base (prefs, mid...).

Jordan reprend la main du copilote.

• Jordan : Où est-ce qu'on fait ça ?
• Adrien montre d'où ça vient via le fetch et le save dans userModel
• Montrer que la migration de données est faite de manière implicite et n'est pas très robuste (s'il n'y a jamais de save après un save).

13. Appliquer la migration de données

git cherry-pick migrate-db # --> commit: 96d94f5ee3e5d4488615ca054a45b34c8e742a9a

Si le cherry-pick ne passe pas, vider l'index de git
14. Relancer les approvals

8. Découplage du controller

1. Commencer par le controller post en expliquant qu'on sort du fichier les instanciations. Commenter les require de features et userCollection
2. Faire passer features à la stack d'appel de insert jusqu'à Application.js
3. Ne pas oublier subdir.js en montrant app.route
4. Déplacer la création des objets dans attachLegacyRoutesFromFile
5. Relancer les tests d'approval et montrer que ça fonctionne

9. Décommisionnement des Approval tests (optionnel)

git checkout main

Aller dans les tests d'intégration de post et de l'infra mongo

---

TODO:

• Voir comment définir les types en JSDoc de manière globale pour éviter les imports

---

[julien-topcu]

Point du 10/02/2022

choix / décisions

• refactoring in-place: on modifie le code existant
• partir du principe que ça fair un bail que les serveurs web ne retournent plus d'HTML => on fix une API qui retourne du JSON (pas de présentation)
• focus sur une feature avec use case d'écriture/modification (ex: ajouter un morceau à son profil openwhyd: https://github.com/openwhyd/openwhyd/blob/master/test/integration/post.api.tests.js)

cas typiques qu'on aimerait couvrir

• faire émerger le domaine depuis une codebase legacy
• couplage métier + technique
• code metier entre controlleurs et persitance, à séparer
• transitionner progressivement de JS à TS
• "poison pill" pl.id == 'create' (https://github.com/openwhyd/openwhyd/blob/master/test/integration/post.api.tests.js#L47) => cas à traiter par adaptateur, pour pas que create soit transmis au domaine
• manque d'IOC => introduire Nest ?

TODO

• Adrien fournit une cartographie rapide des fichiers principaux à refacto
• Adrien fait le ménage dans openwhyd => garder seulement:
  • features "post track", "profile" et "playlist"
  • tests api + cypress correspondants
• Voir comment on insère du TypeScript dans cette codebase JS

[julien-topcu]

STRATÉGIE DE MIGRATION:

• Mesure / Visualisation du coverage sur la feature à refacto:

  • Test E2E (GUI)
  • Test d'integration (API / Controller)

• Analyse du coverage, pour mettre en évidence les cas fonctionnels non couvert

• Analyse des tests de plus haut niveau pour voir si des usecases serait plus explicite à ce niveau là

• Remboursement de la couverture fonctionnelle:

  • Analyse les usecases couvert par les tests E2E / pour les redescendre potentiellement vers l'intégration
  • Exploration fonctionnelle pour re-comprendre, les critères d'acceptance manquant
  • Ecrire des tests au niveau intégration (controller)

• Inversion de controle entre la persistence et le controller /Rendre explicite le DTO coté base de donnée

  • Approval tests (DUMPDB MONGO):
    • https://github.com/approvals/Approvals.NodeJS#approvalsverifyasjson
  • Objectif: Controller <- Persistence (Mapping -> créer DTO dédiée (Voir pour l'introduction de typescript pour modeliser les types))

• Introduction de la couche domaine:

  • Aller plus loin avec l'inversion entre le controller et le domaine
    • Objectif: Controller -> Domaine <- Persistence
  • Définition du modele métier / ACL -> Mapping
    • Objectif: Controller(DTO / Mapping(ACL) -> Domaine(Modele dedié) <- Persistence

• Constat -> Modele métier actuel est anémique:

  • Remonter la logique métier de la persistence vers le domaine
  • Pousser la loguque métier du controller vers le domaine

• Introduction des tests de critére d'acceptance en isolation (TDD avec l'étape précèdent)

• Introduction des tests d'intégration vis a vis de notre persistence

[julien-topcu]

Remboursement de la couverture fonctionnelle - Etapes

Execution de test E2E

1. lancement

docker-compose up -d
open http://localhost:8080

2. login avec "admin" / "admin"
3. poster une track en collant une URL (ex: https://file-examples-com.github.io/uploads/2017/11/file_example_MP3_700KB.mp3) dans la barre recherche, puis la retrouver sur son profil openwhyd
4. ouvrir chrome dev tools
5. cliquer sur le "edit" de la track, changer le nom, valider
6. récupérer la requete HTTP emise pour éditer le titre ("copy as node.js fetch")
7. coller dans test/integration/post.api.tests.js, puis finir d'écrire le test => https://gitlab.com/shodo.io/public/hexagonal-architecture-nodejs-migration/-/commit/08bcc1ac83e37cefc54efe1227e4f2d6e60e5689
8. execution du test:

• dans un premier terminal:

. ./env-vars-testing.sh
export MONGODB_PORT=27117
export MONGODB_DATABASE=openwhyd_test
npm start

• dans un second terminal:

. ./env-vars-testing.sh
export MONGODB_PORT=27117
export MONGODB_DATABASE=openwhyd_test
npm run test-reset && npm run test:integration

Next steps / TODO

• Jordan & Adrien: fournir une liste de scenarios à écrire sous forme de tests d'intégration API
• Julien: implémenter certain de ces tests dans test/integration/post.api.tests.js (cf TODOs dans ce fichier)
• Adrien: fournir outil de dump MongoDB pour approval tests, si possible compatible avec https://github.com/approvals/Approvals.NodeJS
• Jordan: automatiser la procédure d'execution des tests de test/integration/post.api.tests.js

[julien-topcu]

Montrer lors de la conf l'intérêt de faire des tests FIRST !

Repartir du test should re-add a track into a new playlist et montrer que son introduction fait péter une demi-douzaine d'autres tests car ils sont liés entre eux.

[JKratus]

Montrer l'évolution de la couverture du code liée aux fonctionnalités autour du concept de post, après le remboursement de la dette (test de controller post)

• dans un terminal:

docker-compose up -d mongo # starts mongo on port 27117
source ./env-vars-testing-local.sh
npm run test:integration:legacy-post:cov

• puis, dans un terminal séparé:

source ./env-vars-testing-local.sh
npm run test:integration:post:cov 

Les 2 commandes produisent un rapport qui peuvent être comparé

[JKratus]

Pour s'assurer d'être iso par rapport à la persistence d'un post, lancer les tests avec une approche approval testing basé sur des dumps de la db

$ docker-compose up -d mongo
$ npm run -s test:approval:coverage

(les tests approval lancent le serveur et nettoient la DB programmatiquement)

[JKratus]

Pour visualiser directement le coverage dans vscode:

• Installer Gutter for vscode: https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters
• Gutter se base sur le fichier coverage/lcov.info
• Les commandes npm run test:integration:post:cov et npm run -s test:approval:coverage supprime le repertoire coverage pour avoir la couverture uniquement de la partie souhaitée.
• Un fois un coverage généré, cliqué sur la pastille 'Watch' de vscode

• Le coverage s'affiche directement sur les fichiers dans l'IDE

[adrienjoly]

11 mars

Rappel de strat

1. virer le dead code + solidifier calcul de coverage
2. inverser dependance entre controleur et persistence
3. faire emerger le domaine

TODO

• que faire du src ? (decision que c'est du dead code mais snapshoté dans les tests approval)
• ajouter couverture pour img
• passer en await l'appel à createPlaylist, pour rendre plus lisibles la séquentialité et le retour de fonction => https://github.com/openwhyd/openwhyd-solo/pull/13/files#diff-db419e2f985ae41f450c2e0c2589debe9b177a41d6fc3c26f23c648adf4e1c5eR114

Modélisation domaine

Jordan nous présente la ré-application de ce qu'on avait montré en live-stream:

• en fait, User est un agrégat de Playlists

Elements de story-telling

1. reproduire rapidement ce qui s'est passé pendant le live => aller dans le mur, conclusion: need tests, moar coverage

2. ajouter des tests => surprise: découverte de side effects entre tests (violation de FIRST) => besoin de structure sûre pour nos prochains tests

   • compute la couverture totale: approval + integration (pas forcément à montrer)

3. grace au coverage, on se rend compte qu'il y a pas mal de use cases couverts par fonction, et pas mal de lignes non couvertes

   • charge cognitive: on collapse les parties du code qui sont pas appelées
   • clean code: renommer les params, extraction de fonctions, fonctions pures, pour améliorer lisibilité
   • en cas de test échoué, lancer seulement un type de test, sans coverage (pour pas niquer les numéros de ligne)
   • => en CI, envoyer la coverage totale de npm run test:post:coverage sur codacy
   • montrer que une partie de createPlaylist n'est pas couverte (cf https://app.codacy.com/gh/openwhyd/openwhyd/file/68022651853/coverage?bid=17325385&fileBranchId=17325385) => écriture d'un test integration (à montrer en live)

4. on choisit de commencer par injecter createPlaylist (sous forme de domaine service) car il est deja relativement bien isolé, mais aussi pour avancer vers le retrait du poison pill pl.id=="create"

   • wip: refactor: Inject domain + track coverage on CI #14

5. soit on va vers typescript, soit on continue d'extraire le domaine.

Topo sur sessions effectuées jusque là

• 10/02 soir: discussion / propositions / planification => https://gitlab.com/shodo.io/public/hexagonal-architecture-nodejs-migration/-/issues/1
• 16/02 matin: première tentative de refacto, en stream live sur Twitch
• 25/02 matin: proposition de stratégie avec remboursement de tests => https://gitlab.com/shodo.io/public/hexagonal-architecture-nodejs-migration/-/issues/1#note_855177045, puis déplacement de gitlab vers github => https://github.com/openwhyd/openwhyd-solo (pour réutilisation des workflows de CI)
• 28/02 aprem: écriture de tests
• 7/03 matin: écriture de tests (2)
• 11/03 matin: séance de refacto en mob, en physique => PR refactor: Clean code + branche refactor: extract domain by adrienjoly · Pull Request #14 · openwhyd/openwhyd-solo
• 23/03 matin: cf Structure du talk #7 (comment), Jordan et Julien se sont retrouvé au SHODO HQ, Adrien était en distanciel
• 25/03 matin: Adrien a simplifié l'implem du repository/adapter, cf fix: Simplify implementation of userCollection MongoDB adapter #18 (jour commu au lieu de destaff contenu, suite à l'impossibilité d'aller au Froiminaire)
• Mardi 29/03: Julien et Adrien vont passer la journée à construire le déroullement du talk sous forme de commits. (cf Structure du talk #7 (comment))
• Jeudi 7/04 aprem: Julien et Adrien vont terminer le déroullé
• Vendredi 8/04 journée: répétitions entre nous
• Mercredi 13/04: répétition générale au SHODAY
• Mercredi 20/04: talk donné à Devoxx

[adrienjoly]

Matin du mercredi 23 mars

• Julien a présenté les étapes d'extraction du domaine depuis le code de createPlaylist => étranglement progressive du legacy, de gauche (APIs) à droite (mongodb)
• On est d'accord pour dire que le scope du talk est cet etranglement du use case "creation de playlist lors d'un post de track"
• on va prétendre qu'on a executé une migration de données (cf refactor: Hexagonalify create playlist without stowaway pattern #17 (comment))
  • à terme, notre implem de repository pourra appeler directement mongodb
  • Adrien: ajouter les champs manquants aux données insérées dans nos tests
  • virer le fetchUserById() de UserCollection.insertPlaylist() (cf code commenté)
  • virer aussi la dépendance vers models/users en utilisant le driver mongo directement dans getUserById
  • puis update les snapshots
• prochaine séance à prévoir: 1 journée pour construire le cheminement du live coding sous forme de commits

[adrienjoly]

Mardi 29 Mars (journée)

• objectif de la séance: définir le déroulé du talk => écrire les commits dans une branche, avec annotations de ce qu'on devra dire/présenter le jour J (sous forme de commentaires GitHub dans une PR associée à cette même branche)

Matin

• Julien a complété la description de ce ticket pour récapituler les grandes étapes
• on a décidé de partir des tests approval => création du tag migration-start-with-approval-tests
• pendant la pause dej, Adrien a proposé de compléter ces approval tests pour qu'ils couvrent aussi les réponses de l'API
  

Aprem

• on a complété la branche de déroulé (=> fix: migration #19) jusqu'à l'étape "6. Extraire la logique métier de la couche de persistence"
• on a décidé de décaler l'injection de dépendances à la fin, pour réduire la charge cognitive du public pendant la phase d'extraction de logique métier => ça a pris plus de temps que prévu => on pose notre Jeudi après midi de la semaine prochaine pour finir ça avant la journée de répétition du Vendredi 8
• il ne reste plus qu'à finir le déroulé (commits) pour les parties "7. Injection de dépendance", "8. Migration de donnée" et "9. Décommisionnement des Approval tests", en sachant que Jordan a proposé d'avancer de son côté sur l'écriture de tests DB pour la partie 9.

[adrienjoly]

Jeudi 7 Avril (aprem, Julien + Adrien)

Objectif: finir déroullé ? (cf #19)

• 6. Extraire la logique métier de la couche de persistence

  • Créer un test fonctionnel
  • Définir une spi et une implémentation Stub pour les tests.
  • Implémenter la logique métier dans le domaine.

• 7. Injection de dépendance

  • Inversion de dépendance entre le domaine et la persistence :
    • Suppression de la dépendance vers userModel au profit de l'adaptateur Mongo
    • Migration de donnés, pour qu'on puisse s'appuyer sur nos tests approval
    • Définir les structures de bases de donnée et faire l'anti-corruption layer
    • Instancier le domaine et la persistance dans le controller.
  • Injecter le domaine dans le controller à partir de l'Application (composition root)
    • Supprimer la fonction createPlaylist de userModel

• 9 8. Décommisionnement des Approval tests

  • Ajout des tests d'intégration de controller
  • Ajout du test de l'adaptateur Mongo

• corriger le bug d'incrementation d'id

• TODO: Réintégrer dans la branche migration les infrastructures permettant de simplifier le lancement les tests et le coverage

  • 9016390
  • 9b3a385
  • Toutes modifications faites dans le répertoire test de https://github.com/openwhyd/openwhyd-solo/pull/14/files