Skip to content

Commit

Permalink
refactor: simplify dependency injection approach
Browse files Browse the repository at this point in the history
Rather than adding an app configuration key for every mockable
dependency of every module, we can standardize on a convention for
naming and defining the mocks, assisted by a small macro.
  • Loading branch information
digitalcora committed Nov 18, 2024
1 parent 452160d commit 5cfbec8
Show file tree
Hide file tree
Showing 14 changed files with 133 additions and 126 deletions.
20 changes: 0 additions & 20 deletions config/test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -159,26 +159,6 @@ config :screens, :screens_by_alert,
screens_last_updated_ttl_seconds: 2,
screens_ttl_seconds: 1

config :screens, Screens.V2.ScreenData,
config_cache_module: Screens.Config.MockCache,
parameters_module: Screens.V2.ScreenData.MockParameters

config :screens, Screens.V2.CandidateGenerator.DupNew, stop_module: Screens.Stops.MockStop

config :screens, Screens.V2.CandidateGenerator.Dup.Departures,
headways_module: Screens.MockHeadways

config :screens, Screens.V2.RDS,
departure_module: Screens.V2.MockDeparture,
route_pattern_module: Screens.RoutePatterns.MockRoutePattern,
stop_module: Screens.Stops.MockStop

config :screens, Screens.V2.CandidateGenerator.Elevator.Closures,
stop_module: Screens.Stops.MockStop,
facility_module: Screens.Facilities.MockFacility,
alert_module: Screens.Alerts.MockAlert,
route_module: Screens.Routes.MockRoute

config :screens, Screens.LastTrip,
trip_updates_adapter: Screens.LastTrip.TripUpdates.Noop,
vehicle_positions_adapter: Screens.LastTrip.VehiclePositions.Noop
26 changes: 26 additions & 0 deletions lib/screens/inject.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
defmodule Screens.Inject do
@moduledoc "Conveniences for dependency injection and mocking."

@doc """
When the Mix env is not `test`, resolves to the given module name. When it is, resolves to the
module name with `.Mock` appended.
The mock module is not automatically defined (see e.g. `test/support/mocks.ex`).
Example usage:
defmodule Screens.Foo do
import Screens.Inject
@dependency injected(Screens.Dependency)
def do_something, do: @dependency.fetch_data()
end
"""
defmacro injected(module) do
quote do
module = unquote(module)
if Mix.env() == :test, do: Module.concat(module, "Mock"), else: module
end
end
end
4 changes: 3 additions & 1 deletion lib/screens/v2/candidate_generator/dup/departures.ex
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ defmodule Screens.V2.CandidateGenerator.Dup.Departures do
alias ScreensConfig.V2.Departures.Query.Params
alias ScreensConfig.V2.Dup

@headways Application.compile_env(:screens, [__MODULE__, :headways_module], Screens.Headways)
import Screens.Inject

@headways injected(Screens.Headways)

def departures_instances(
%Screen{
Expand Down
8 changes: 3 additions & 5 deletions lib/screens/v2/candidate_generator/dup_new/header.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ defmodule Screens.V2.CandidateGenerator.DupNew.Header do
alias ScreensConfig.V2.Dup
alias ScreensConfig.V2.Header.{CurrentStopId, CurrentStopName}

@stop Application.compile_env(
:screens,
[Screens.V2.CandidateGenerator.DupNew, :stop_module],
Screens.Stops.Stop
)
import Screens.Inject

@stop injected(Screens.Stops.Stop)

@spec instances(Screen.t(), DateTime.t()) :: [NormalHeader.t()]
def instances(%Screen{app_params: %Dup{header: header_config}} = config, now) do
Expand Down
11 changes: 6 additions & 5 deletions lib/screens/v2/candidate_generator/elevator/closures.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,19 @@ defmodule Screens.V2.CandidateGenerator.Elevator.Closures do
require Logger

alias Screens.Alerts.{Alert, InformedEntity}
alias Screens.Facilities.Facility
alias Screens.Routes.Route
alias Screens.Stops.Stop
alias Screens.V2.WidgetInstance.ElevatorClosures
alias Screens.V2.WidgetInstance.Serializer.RoutePill
alias ScreensConfig.Screen
alias ScreensConfig.V2.Elevator

@stop Application.compile_env(:screens, [__MODULE__, :stop_module], Stop)
@facility Application.compile_env(:screens, [__MODULE__, :facility_module], Facility)
@alert Application.compile_env(:screens, [__MODULE__, :alert_module], Alert)
@route Application.compile_env(:screens, [__MODULE__, :route_module], Route)
import Screens.Inject

@alert injected(Alert)
@facility injected(Screens.Facilities.Facility)
@route injected(Route)
@stop injected(Stop)

@spec elevator_status_instances(Screen.t()) :: list(ElevatorClosures.t())
def elevator_status_instances(%Screen{app_params: %Elevator{elevator_id: elevator_id}}) do
Expand Down
14 changes: 5 additions & 9 deletions lib/screens/v2/rds.ex
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ defmodule Screens.V2.RDS do
into widgets by screen-specific code.
"""

import Screens.Inject

alias Screens.Lines.Line
alias Screens.RoutePatterns.RoutePattern
alias Screens.Routes.Route
Expand Down Expand Up @@ -40,15 +42,9 @@ defmodule Screens.V2.RDS do
defstruct []
end

@departure Application.compile_env(:screens, [__MODULE__, :departure_module], Departure)

@route_pattern Application.compile_env(
:screens,
[__MODULE__, :route_pattern_module],
RoutePattern
)

@stop Application.compile_env(:screens, [__MODULE__, :stop_module], Stop)
@departure injected(Departure)
@route_pattern injected(RoutePattern)
@stop injected(Stop)

@max_departure_minutes 90

Expand Down
13 changes: 3 additions & 10 deletions lib/screens/v2/screen_data.ex
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,11 @@ defmodule Screens.V2.ScreenData do
alias ScreensConfig.Screen
alias __MODULE__.{ParallelRunSupervisor, Layout}

import Screens.Inject
import Screens.V2.Template.Guards, only: [is_slot_id: 1, is_paged_slot_id: 1]

@config_cache Application.compile_env(
:screens,
[__MODULE__, :config_cache_module],
Screens.Config.Cache
)
@parameters Application.compile_env(
:screens,
[__MODULE__, :parameters_module],
Screens.V2.ScreenData.Parameters
)
@config_cache injected(Screens.Config.Cache)
@parameters injected(Screens.V2.ScreenData.Parameters)

@type t :: %{type: atom()}
@type simulation_data :: %{full_page: t(), flex_zone: [t()]}
Expand Down
7 changes: 2 additions & 5 deletions lib/screens/v2/screen_data/layout.ex
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,10 @@ defmodule Screens.V2.ScreenData.Layout do
alias Screens.V2.WidgetInstance
alias ScreensConfig.Screen

import Screens.Inject
import Template.Guards, only: [is_paged: 1, is_paged_slot_id: 1, is_non_paged_slot_id: 1]

@parameters Application.compile_env(
:screens,
[Screens.V2.ScreenData, :parameters_module],
Screens.V2.ScreenData.Parameters
)
@parameters injected(Screens.V2.ScreenData.Parameters)

@type t :: {Template.layout(), %{Template.slot_id() => WidgetInstance.t()}}
@type non_paged ::
Expand Down
10 changes: 6 additions & 4 deletions test/screens/v2/candidate_generator/dup/departures_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ defmodule Screens.V2.CandidateGenerator.Dup.DeparturesTest do
alias ScreensConfig.V2.Departures.Header, as: SectionHeader
alias ScreensConfig.V2.Departures.{Layout, Query, Section}
alias ScreensConfig.V2.Dup, as: DupConfig
alias Screens.MockHeadways
alias Screens.Predictions.Prediction
alias Screens.Routes.Route
alias Screens.Schedules.Schedule
Expand All @@ -19,9 +18,12 @@ defmodule Screens.V2.CandidateGenerator.Dup.DeparturesTest do
alias Screens.V2.WidgetInstance.Departures, as: DeparturesWidget
alias Screens.V2.WidgetInstance.OvernightDepartures

import Screens.Inject
import Mox
setup :verify_on_exit!

@headways injected(Screens.Headways)

defp put_primary_departures(widget, primary_departures_sections) do
%{
widget
Expand Down Expand Up @@ -1151,7 +1153,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.DeparturesTest do
])

now = ~U[2020-04-06T10:00:00Z]
expect(MockHeadways, :get_with_route, 2, fn "place-B", "test", ^now -> {12, 16} end)
expect(@headways, :get_with_route, 2, fn "place-B", "test", ^now -> {12, 16} end)

fetch_alerts_fn = fn
[
Expand Down Expand Up @@ -1615,7 +1617,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.DeparturesTest do
])

now = ~U[2020-04-06T10:00:00Z]
expect(MockHeadways, :get_with_route, 2, fn "place-kencl", "test", ^now -> {7, 13} end)
expect(@headways, :get_with_route, 2, fn "place-kencl", "test", ^now -> {7, 13} end)

fetch_alerts_fn = fn
[
Expand Down Expand Up @@ -2510,7 +2512,7 @@ defmodule Screens.V2.CandidateGenerator.Dup.DeparturesTest do
])

now = ~U[2020-04-06T10:00:00Z]
stub(MockHeadways, :get_with_route, fn "place-overnight", "Red", ^now -> {5, 8} end)
stub(@headways, :get_with_route, fn "place-overnight", "Red", ^now -> {5, 8} end)

fetch_schedules_fn = fn
_, nil ->
Expand Down
6 changes: 4 additions & 2 deletions test/screens/v2/candidate_generator/dup_new_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@ defmodule Screens.V2.CandidateGenerator.DupNewTest do
alias ScreensConfig.Screen
alias ScreensConfig.V2.{Alerts, Departures, EvergreenContentItem, Header, Schedule}
alias ScreensConfig.V2.Dup, as: DupConfig
alias Screens.Stops.MockStop
alias Screens.Util.Assets
alias Screens.V2.CandidateGenerator.DupNew
alias Screens.V2.WidgetInstance.{DeparturesNoData, EvergreenContent, NormalHeader}

import Screens.Inject
import Mox
setup :verify_on_exit!

@stop injected(Screens.Stops.Stop)

describe "candidate_instances/2" do
@config %Screen{
app_id: :dup_v2,
Expand All @@ -38,7 +40,7 @@ defmodule Screens.V2.CandidateGenerator.DupNewTest do

test "returns header with stop name determined from stop ID" do
config = put_in(@config.app_params.header, %Header.CurrentStopId{stop_id: "test_id"})
expect(MockStop, :fetch_stop_name, fn "test_id" -> "Test Name" end)
expect(@stop, :fetch_stop_name, fn "test_id" -> "Test Name" end)

instances = DupNew.candidate_instances(config, @now)

Expand Down
59 changes: 28 additions & 31 deletions test/screens/v2/candidate_generator/elevator/closures_test.exs
Original file line number Diff line number Diff line change
@@ -1,32 +1,35 @@
defmodule Screens.V2.CandidateGenerator.Elevator.ClosuresTest do
use ExUnit.Case, async: true

import Mox
setup :verify_on_exit!

alias Screens.Alerts.{Alert, MockAlert}
alias Screens.Facilities.MockFacility
alias Screens.Routes.{MockRoute, Route}
alias Screens.Stops.{MockStop, Stop}
alias Screens.Alerts.Alert
alias Screens.Routes.Route
alias Screens.Stops.Stop
alias Screens.V2.CandidateGenerator.Elevator.Closures, as: ElevatorClosures
alias ScreensConfig.Screen
alias ScreensConfig.V2.Elevator

import Screens.Inject
import Mox
setup :verify_on_exit!

@alert injected(Alert)
@facility injected(Screens.Facilities.Facility)
@route injected(Route)
@stop injected(Stop)

describe "elevator_status_instances/1" do
test "Only returns alerts with effect of :elevator_closure" do
expect(MockFacility, :fetch_stop_for_facility, fn "111" ->
{:ok, %Stop{id: "place-test"}}
end)
expect(@facility, :fetch_stop_for_facility, fn "111" -> {:ok, %Stop{id: "place-test"}} end)

expect(MockStop, :fetch_parent_station_name_map, fn ->
expect(@stop, :fetch_parent_station_name_map, fn ->
{:ok, %{"place-test" => "Place Test"}}
end)

expect(MockRoute, :fetch, fn %{stop_id: "place-test"} ->
expect(@route, :fetch, fn %{stop_id: "place-test"} ->
{:ok, [%Route{id: "Red", type: :subway}]}
end)

expect(MockAlert, :fetch_elevator_alerts_with_facilities, fn ->
expect(@alert, :fetch_elevator_alerts_with_facilities, fn ->
alerts = [
struct(Alert,
id: "1",
Expand Down Expand Up @@ -67,23 +70,21 @@ defmodule Screens.V2.CandidateGenerator.Elevator.ClosuresTest do
end

test "Groups outside closures by station" do
expect(MockFacility, :fetch_stop_for_facility, fn "111" ->
{:ok, %Stop{id: "place-test"}}
end)
expect(@facility, :fetch_stop_for_facility, fn "111" -> {:ok, %Stop{id: "place-test"}} end)

expect(MockStop, :fetch_parent_station_name_map, fn ->
expect(@stop, :fetch_parent_station_name_map, fn ->
{:ok, %{"place-haecl" => "Haymarket"}}
end)

expect(MockRoute, :fetch, 2, fn
expect(@route, :fetch, 2, fn
%{stop_id: "place-haecl"} ->
{:ok, [%Route{id: "Orange", type: :subway}]}

%{stop_id: "place-test"} ->
{:ok, [%Route{id: "Red", type: :subway}]}
end)

expect(MockAlert, :fetch_elevator_alerts_with_facilities, fn ->
expect(@alert, :fetch_elevator_alerts_with_facilities, fn ->
alerts = [
struct(Alert,
id: "1",
Expand Down Expand Up @@ -139,17 +140,15 @@ defmodule Screens.V2.CandidateGenerator.Elevator.ClosuresTest do
end

test "Filters alerts with no facilities or more than one facility" do
expect(MockFacility, :fetch_stop_for_facility, fn "111" ->
{:ok, %Stop{id: "place-test"}}
end)
expect(@facility, :fetch_stop_for_facility, fn "111" -> {:ok, %Stop{id: "place-test"}} end)

expect(MockStop, :fetch_parent_station_name_map, fn ->
expect(@stop, :fetch_parent_station_name_map, fn ->
{:ok, %{"place-haecl" => "Haymarket"}}
end)

expect(MockRoute, :fetch, fn _ -> {:ok, [%Route{id: "Red", type: :subway}]} end)
expect(@route, :fetch, fn _ -> {:ok, [%Route{id: "Red", type: :subway}]} end)

expect(MockAlert, :fetch_elevator_alerts_with_facilities, fn ->
expect(@alert, :fetch_elevator_alerts_with_facilities, fn ->
alerts = [
struct(Alert,
id: "1",
Expand Down Expand Up @@ -184,19 +183,17 @@ defmodule Screens.V2.CandidateGenerator.Elevator.ClosuresTest do
end

test "Return empty routes on API error" do
expect(MockFacility, :fetch_stop_for_facility, fn "111" ->
{:ok, %Stop{id: "place-test"}}
end)
expect(@facility, :fetch_stop_for_facility, fn "111" -> {:ok, %Stop{id: "place-test"}} end)

expect(MockStop, :fetch_parent_station_name_map, fn ->
expect(@stop, :fetch_parent_station_name_map, fn ->
{:ok, %{"place-test" => "Place Test"}}
end)

expect(MockRoute, :fetch, fn %{stop_id: "place-test"} ->
expect(@route, :fetch, fn %{stop_id: "place-test"} ->
:error
end)

expect(MockAlert, :fetch_elevator_alerts_with_facilities, fn ->
expect(@alert, :fetch_elevator_alerts_with_facilities, fn ->
alerts = [
struct(Alert,
id: "1",
Expand Down
Loading

0 comments on commit 5cfbec8

Please sign in to comment.