search

openwhyd / openwhyd-solo

Single-user version of openwhyd.org
MIT License
5 stars 0 forks source link

Structure du talk #7

Open julien-topcu opened 2 years ago

julien-topcu commented 2 years ago

Déroulé du talk

0. Prérequis

Liens de référence:

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

1. Faire une démo d'OpenWhyd

Adrien

Objectifs:

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
  3. 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.
  4. (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
  5. Montrer dans les logs que l'on a un diff des approvals.
  6. 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

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 ?
  3. Création du type CreatePlaylist dans api.ts
  4. 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
  5. 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)
);
  6. Appliquer le type CreatePlaylist via JSDoc puis activer la vérification avec ts-check
  7. 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
  8. Extract de createPlaylist dans features.js dans le domaine.
  9. Appliquer le ts-check dans features
  10. 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 factorycreateFeatures` 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.

  1. 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

  2. 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:

julien-topcu commented 2 years ago

Point du 10/02/2022

choix / décisions

cas typiques qu'on aimerait couvrir

TODO

julien-topcu commented 2 years ago

STRATÉGIE DE MIGRATION:

julien-topcu commented 2 years ago

Remboursement de la couverture fonctionnelle - Etapes

Execution de test E2E

  1. lancement
docker-compose up -d
open http://localhost:8080
  1. login avec "admin" / "admin"
  2. 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
  3. ouvrir chrome dev tools
  4. cliquer sur le "edit" de la track, changer le nom, valider
  5. récupérer la requete HTTP emise pour éditer le titre ("copy as node.js fetch")
  6. 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
  7. execution du test:

Next steps / TODO

julien-topcu commented 2 years ago

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 commented 2 years ago

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)

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

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

JKratus commented 2 years ago

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 commented 2 years ago

Pour visualiser directement le coverage dans vscode:

Screenshot 2022-03-08 at 12 38 15

adrienjoly commented 2 years ago

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

Modélisation domaine

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

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

    • [x] 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
      • [x] 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)
      • [x] => en CI, envoyer la coverage totale de npm run test:post:coverage sur codacy
    • [x] 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"

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

Topo sur sessions effectuées jusque là

adrienjoly commented 2 years ago

Matin du mercredi 23 mars

adrienjoly commented 2 years ago

Mardi 29 Mars (journée)

Matin

Aprem

adrienjoly commented 2 years ago

Jeudi 7 Avril (aprem, Julien + Adrien)

Objectif: finir déroullé ? (cf https://github.com/openwhyd/openwhyd-solo/pull/19)

adrienjoly commented 2 years ago

Vendredi 8 Avril

Répétition avec Julien et Jordan

=> branche migration-2022-04-08

adrienjoly commented 2 years ago

Jeudi 13 Avril

Répétition pendant SHODAY.

=> branche: https://github.com/openwhyd/openwhyd-solo/tree/migration-2022-04-13-shoday

Anti-sèche

adrienjoly commented 2 years ago

Vendredi 15 Avril

Je viens de créer deux snippets avec l'extension Snippet - Visual Studio Marketplace:

image

TODO

=> branche: migration-2022-04-15

adrienjoly commented 2 years ago

Pour référence, voici:

adrienjoly commented 1 year ago

Update suite à la répète de Lundi:

Note: je n'ai pas modifié le tag checkout migration-start-with-approval-tests car il me semble qu'on ne lance plus ces tests d'intégration après la migration. => on aura probablement envie de le faire, si jamais on trouve un moyen simple et fluide d'exécuter les nouveaux tests d'intégration (requêtes db via SPI) après la migration.

adrienjoly commented 1 year ago

Notes de 2ème répète:

diff --git a/app/controllers/api/post.js b/app/controllers/api/post.js
index 3728376..e32f36d 100644
--- a/app/controllers/api/post.js
+++ b/app/controllers/api/post.js
@@ -63,6 +63,7 @@ exports.actions = {
       uId: httpParams.uId,
       uNm: httpParams.uNm,
       text: httpParams.text || '',
+      pl: extractPlaylistRequest(httpParams),
       // fields that will be ignored by rePost():
       name: httpParams.name,
       eId: httpParams.eId,
@@ -105,23 +106,18 @@ exports.actions = {
       }
     }

-    const playlistRequest = extractPlaylistRequest(httpParams);
-
-    if (needToCreatePlaylist(playlistRequest)) {
+    if (needToCreatePlaylist(postQuery.pl)) {
       const playlist = await features.createPlaylist(
         httpParams.uId,
-        playlistRequest.name
+        postQuery.pl.name
       );
       if (playlist) {
-        postQuery.pl = { id: playlist.id, name: playlistRequest.name };
+        postQuery.pl.id = playlist.id;
         // console.log('playlist was created', q.pl);
       }
     } else {
-      postQuery.pl = {
-        id: parseInt(playlistRequest.id),
-        name: playlistRequest.name,
-      };
-      if (isNaN(playlistRequest.id)) delete postQuery.pl; //q.pl = null;
+      postQuery.pl.id = parseInt(postQuery.pl.id);
+      if (isNaN(postQuery.pl.id)) delete postQuery.pl; //q.pl = null;
     }

     actualInsert();