Refactor Smoove GBFS controller #1867

AntoineAugusti · 2021-10-20T09:23:44Z

Refactor du controlleur GBFS pour Smoove en suivant JC Decaux

Gestion de possibles erreurs HTTP
Ajout de tests
Ajout de la version GBFS

AntoineAugusti · 2021-10-20T09:45:28Z

apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

-      }
-    )
-    |> render("gbfs.json")
+    {:ok,


Il y a beaucoup de lignes de code modifiées mais c'est en réalité beaucoup de refactor en

{:ok, } |> render_response(conn)

AntoineAugusti · 2021-10-20T09:46:15Z

apps/gbfs/test/gbfs/controllers/smoove_controller_test.exs

@@ -0,0 +1,52 @@
+defmodule GBFS.SmooveControllerTest do


Un copier/coller quasi complet de https://github.com/etalab/transport-site/blob/master/apps/gbfs/test/gbfs/controllers/jcdecaux_test.exs

Idéalement il faudrait essayer d'adapter les tests pour éviter d'ajouter d'usage de use_cassette (cf #1853) et idem pour with_mock (car mock modifie la définition globale des modules, il n'y a pas de modification par processus jjh42/mock#129), au profit de Mox (https://github.com/dashbitco/mox).

Si tu en as l'énergie!

@thbar Tu aurais un exemple de PR où on a abandonné use_cassette et utilisé autre chose ?

Je n'ai pas trouvé facilement d'exemple précisément pour ça, mais on a quelque chose d'approchant dans le travail réalisé par @fchabouis dans #1863.

Je vais essayer de décrire rapidement la guideline générale pour enlever autant with_mock que use_cassette (il manquera des points à traiter oralement à un moment, mais ça suffira pour avancer) :

Identifier les points "frontière" (boundaries) dans le code, ceux qui font appel à la dépendance externe par un moyen ou un autre. Dans le cas de ce contrôleur, ça sera l'appel à HTTPoison.get, qui est ici:

transport-site/apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

Lines 107 to 109 in 8a2d13e

defp get_stations(url) do

with {:ok, %{status_code: 200, body: body}} <- HTTPoison.get(url),

body when not is_nil(body) <- :iconv.convert("iso8859-1", "latin1", body) do

Dégager une "behaviour" ou un wrapper (mais là pour httpoison le travail a déjà été fait ici et il se trouve que HTTPoison lui-même est une "behaviour", ce qui fait que Mox peut le mocker directement

Faire appel à ce wrapper dans le code du contrôleur (comme fait dans Utilisation d'un Mock pour tester url_extractor.ex #1863) plutôt que de faire l'appel direct à HTTPoison

Modifier les tests pour créer des assertions en utilisant Mox. La configuration de "test" (ici) substitue l'implémentation de httpoison_impl avec un Mock, et on peut asserter dessus ensuite (https://github.com/etalab/transport-site/pull/1863/files#diff-cd2bd95618a14fbcafc59bb740eece7ee030f2c29af4bd5ea0add7c3062bc171R8)

Attention, il est recommandé de faire un "verify" des mocks (ce qui n'est pas fait dans #1863 je le remarque) avec setup :verify_on_exit! ou avec verify!().

Voilà pour l'idée générale ; ça paraît un peu compliqué initialement, toutefois au final ça a le mérite de définir des boundaries claires et de rendre le code très testable et substituable.

À ta dispo si besoin de clarifications!

(Ref: https://hexdocs.pm/mox/Mox.html)

Merci beaucoup, fait dans le commit suivant ! Je pourrai faire de même pour JC Decaux dans une autre PR.

thbar · 2021-10-20T09:50:23Z

apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

+             fn a ->
+               %{
+                 "name" => Atom.to_string(a),
+                 "url" => apply(Routes, String.to_atom("#{contract_id}_url"), [conn, a])


Pour info on peut avoir à faire attention avec les conversions vers les atoms, si la donnée d'input est arbitraire / extérieure (https://til.hashrocket.com/posts/gkwwfy9xvw-converting-strings-to-atoms-safely).

Là dans ce cas ma compréhension est que la liste des contract_id est fixe et contrôlée par nous, donc pas de problème.

thbar · 2021-10-20T09:51:07Z

apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

-    conn
-    |> assign(:data, get_station_information(url))
-    |> render("gbfs.json")
+    conn.assigns.smoove_params.url |> get_station_information() |> render_response(conn)


Tiens curieux que le formateur Elixir laisse ça sous cette forme (et credo) derrière, je me serais attendu à ce qu'il impose un passage à la ligne.

thbar · 2021-10-20T09:57:08Z

apps/gbfs/test/gbfs/controllers/smoove_controller_test.exs

@@ -0,0 +1,52 @@
+defmodule GBFS.SmooveControllerTest do


Idéalement il faudrait essayer d'adapter les tests pour éviter d'ajouter d'usage de use_cassette (cf #1853) et idem pour with_mock (car mock modifie la définition globale des modules, il n'y a pas de modification par processus jjh42/mock#129), au profit de Mox (https://github.com/dashbitco/mox).

Si tu en as l'énergie!

AntoineAugusti · 2021-10-20T11:34:39Z

@thbar Merci de tes conseils précieux pour réécrire les tests ! J'ai supprimé la cassette et le mock dans ea25adf

apps/gbfs/lib/gbfs/controllers/smoove_controller.ex

thbar

Super ; merci d'avoir fait le refacto sur les tests, c'est top.

C'est bon pour moi ; je suggère de faire une requête après le déploiement pour vérifier que tout va bien (du fait du swap de l'implémentation par rapport aux tests), mais sinon tout est OK.

AntoineAugusti · 2021-10-20T13:40:05Z

J'ai pu tester les endpoints en local sans problème et j'ai supprimé la dépendance iconv dans le dernier commit 👌

AntoineAugusti · 2021-10-20T13:53:22Z

💆 ✅ 🚲

AntoineAugusti added 2 commits October 20, 2021 11:29

Refactor Smoove GBFS controller

df75f24

Ajout test pour erreur HTTP 500

d7d083c

AntoineAugusti force-pushed the smoove-gbfs-version branch from a4b932d to d7d083c Compare October 20, 2021 09:30

Merge branch 'master' into smoove-gbfs-version

8a2d13e

AntoineAugusti commented Oct 20, 2021

View reviewed changes

AntoineAugusti requested review from fchabouis and thbar October 20, 2021 09:46

AntoineAugusti marked this pull request as ready for review October 20, 2021 09:46

thbar reviewed Oct 20, 2021

View reviewed changes

Refactor des tests

ea25adf

AntoineAugusti requested a review from thbar October 20, 2021 11:34

thbar reviewed Oct 20, 2021

View reviewed changes

apps/gbfs/lib/gbfs/controllers/smoove_controller.ex Outdated Show resolved Hide resolved

thbar self-requested a review October 20, 2021 12:20

thbar approved these changes Oct 20, 2021

View reviewed changes

Suppression iconv

77fe355

AntoineAugusti force-pushed the smoove-gbfs-version branch from 56f4d95 to 77fe355 Compare October 20, 2021 13:27

AntoineAugusti merged commit 390f887 into master Oct 20, 2021

AntoineAugusti deleted the smoove-gbfs-version branch October 20, 2021 13:40

AntoineAugusti mentioned this pull request Oct 20, 2021

GBFS Smoove : modifier rental methods #1870

Closed

This was referenced Oct 22, 2021

Mise à jour comment désactiver page cache #1878

Merged

Améliorer et référencer plus de JDD vélos et trottinettes en libre-service #1882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Smoove GBFS controller #1867

Refactor Smoove GBFS controller #1867

AntoineAugusti commented Oct 20, 2021

AntoineAugusti Oct 20, 2021

AntoineAugusti Oct 20, 2021

thbar Oct 20, 2021

AntoineAugusti Oct 20, 2021

thbar Oct 20, 2021

thbar Oct 20, 2021

AntoineAugusti Oct 20, 2021

thbar Oct 20, 2021

thbar Oct 20, 2021

thbar Oct 20, 2021

AntoineAugusti commented Oct 20, 2021

thbar left a comment

AntoineAugusti commented Oct 20, 2021

AntoineAugusti commented Oct 20, 2021

	defp get_stations(url) do
	with {:ok, %{status_code: 200, body: body}} <- HTTPoison.get(url),
	body when not is_nil(body) <- :iconv.convert("iso8859-1", "latin1", body) do

Refactor Smoove GBFS controller #1867

Refactor Smoove GBFS controller #1867

Conversation

AntoineAugusti commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntoineAugusti commented Oct 20, 2021

thbar left a comment

Choose a reason for hiding this comment

AntoineAugusti commented Oct 20, 2021

AntoineAugusti commented Oct 20, 2021