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

cleanup: Add more alert factories for use in SystemStatus.SubwayTests #2347

Draft
wants to merge 1 commit into
base: jdl/system-status-flowchart
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions test/dotcom/system_status/subway_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ defmodule Dotcom.SystemStatus.SubwayTest do
doctest Dotcom.SystemStatus.Subway

alias Dotcom.SystemStatus.Subway
alias Test.Support.Factories.Alerts.Alert
import Test.Support.Factories.Alerts.Alert
alias Test.Support.Factories.Alerts.InformedEntity
alias Test.Support.Factories.Alerts.InformedEntitySet

Expand Down Expand Up @@ -33,7 +33,10 @@ defmodule Dotcom.SystemStatus.SubwayTest do
affected_route_id = Faker.Util.pick(@lines_without_branches)
time = time_today()
effect = Faker.Util.pick(@effects)
alerts = [current_alert(route_id: affected_route_id, time: time, effect: effect)]

alerts = [
build(:alert, effect: effect) |> active_during(time) |> for_route(affected_route_id)
]

# Exercise
groups = Subway.subway_status(alerts, time)
Expand Down Expand Up @@ -565,7 +568,7 @@ defmodule Dotcom.SystemStatus.SubwayTest do
effect = opts[:effect] || Faker.Util.pick(@effects)
active_period = opts |> Keyword.fetch!(:active_period)

Alert.build(:alert,
build(:alert,
effect: effect,
informed_entity:
InformedEntitySet.build(:informed_entity_set,
Expand Down
39 changes: 39 additions & 0 deletions test/support/factories/alerts/alert.ex
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ defmodule Test.Support.Factories.Alerts.Alert do
use ExMachina

alias Alerts.{Alert, Priority}
alias Test.Support.Factories.Alerts.InformedEntity
alias Test.Support.Factories.Alerts.InformedEntitySet

@high_priority_effects [:delay, :shuttle, :station_closure, :suspension]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to not have to duplicate this from its source of truth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.... What IS its source of truth?

Also I just realized this point might be moot, since SystemStatus.Subway.subway_status/2 doesn't actually do anything with the effect now. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, what is giving the subway status feature the "high priority" alerts then?? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In alerts.ex, the filter_relevant/1 function ensures that only certain alerts are returned, and which effects are "relevant" is defined here.

In this PR, the results from alerts.ex are passed into subway_status/2.

That's how it works now - I'm not overly attached to that thought - there's probably a better way.


def alert_factory do
%Alert{
id: :rand.uniform(999) |> Integer.to_string(),
Expand All @@ -28,4 +31,40 @@ defmodule Test.Support.Factories.Alerts.Alert do
url: Faker.Internet.url()
}
end

def for_route(alert, route_id) do
%{
alert
| informed_entity:
InformedEntitySet.build(:informed_entity_set,
route: route_id,
entities: [
InformedEntity.build(:informed_entity, route: route_id)
]
)
}
end

Comment on lines +35 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: If you adjust this function to be its own factory, you wouldn't need to create an alert and pass it to for_route/2. Say you want to call build(:alert_with_route, %{route_id: "Blue"}), that could work with something like this....

def alert_with_route_factory(%{route_id: route_id} = attrs) do
   informed_entity = InformedEntitySet.build(:informed_entity_set,
      route: route_id,
      entities: InformedEntity.build_list(1, :informed_entity, route: route_id)
    )
   build(:alert, informed_entity: informed_entity)
end

def with_high_priority_effect(alert) do
%{alert | effect: Faker.Util.pick(@high_priority_effects)}
end
Comment on lines +48 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, this could be

def high_priority_alert_factory do
  build(:alert, effect: Faker.Util.pick(@high_priority_effects))
end

to enable high_priority_alerts = build_list(4, :high_priority_alert)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment and this one kind of go together to show why I didn't do it that way though, because I wanted to be able to chain them together, like 👇

build(:alert, effect: effect) |> active_during(time) |> for_route(affected_route_id)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd save you a chain or two, e.g. build(:alert_for_route, route_id: affected_route_id) |> active_during(time). But if you like each property being a separate step in the chain that's a choice.... but if you're doing the same steps over and over it might be more useful to make a factory tailored to that use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's fair - I guess I wasn't sure where to draw the line between what should be chained and what should be "top-level".

I was loosely following this


def active_during(alert, time) do
%{alert | active_period: [{time_before(time), time_after(time)}]}
end

# Returns a random time during the day today before the time provided.
defp time_before(time) do
between(Timex.beginning_of_day(time), time)
end

# Returns a random time during the day today after the time provided.
defp time_after(time) do
between(time, Timex.end_of_day(time))
end

# Returns a random time between the times provided in the Eastern time zone.
defp between(time1, time2) do
Faker.DateTime.between(time1, time2) |> Timex.to_datetime("America/New_York")
end
end