cleanup: Add more alert factories for use in SystemStatus.SubwayTests #2347
Conversation
joshlarson
force-pushed
the
jdl/system-status-alert-factory
branch
from
5d53205 to
f852999
Compare
| 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 | ||
|
|
There was a problem hiding this comment.
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
| alias Test.Support.Factories.Alerts.InformedEntitySet | ||
|
|
||
| @high_priority_effects [:delay, :shuttle, :station_closure, :suspension] |
There was a problem hiding this comment.
It'd be nice to not have to duplicate this from its source of truth.
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
Wait, what is giving the subway status feature the "high priority" alerts then?? 😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah okay! In that case, the link behind "here" is the source of truth I'm referring to.
If there were a function there that exposes that list, say Dotcom.SystemStatus.Alerts.relevant_effects(), you could call that in this module where needed.
joshlarson
force-pushed
the
jdl/system-status-alert-factory
branch
3 times, most recently
from
f587772 to
8bf0570
Compare
| @@ -4,7 +4,7 @@ defmodule Dotcom.SystemStatus.SubwayTest do | |||
|
|
|||
| alias Dotcom.SystemStatus.Alerts | |||
| alias Dotcom.SystemStatus.Subway | |||
| alias Test.Support.Factories.Alerts.Alert | |||
| import Test.Support.Factories.Alerts.Alert | |||
There was a problem hiding this comment.
import and alias statements are separated and alphabetized. but, i wouldn't import here because it is difficult to know what build references
There was a problem hiding this comment.
Yeah - it got mildly annoying to keep calling Alert.<whatever>, but that is better than the bizarre "Where did build come from setup that using import here provides.
| 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 |
There was a problem hiding this comment.
these are general enough to be moved somewhere else
There was a problem hiding this comment.
Thoughts on where, specifically? A DateTime factory?
| end | ||
|
|
||
| def with_high_priority_effect(alert) do | ||
| %{alert | effect: Faker.Util.pick(Dotcom.SystemStatus.Alerts.service_effects())} |
There was a problem hiding this comment.
why are we calling the factory priority_effect when the function is called service_effects?
There was a problem hiding this comment.
Old naming scheme meet new naming scheme!
Would fix this, but it made more sense to delete instead ❌
| InformedEntitySet.build(:informed_entity_set, | ||
| route: route_id, | ||
| entities: [ | ||
| InformedEntity.build(:informed_entity, route: route_id) |
There was a problem hiding this comment.
i would move this to the informed entity set factory since they need to be in agreement
joshlarson
force-pushed
the
jdl/system-status-alert-factory
branch
from
cd4e54e to
9798a8e
Compare
| alerts = [current_alert(route_id: affected_route_id, time: time, effect: effect)] | ||
|
|
||
| alerts = [ | ||
| Alert.build(:alert_for_route, route_id: affected_route_id, effect: effect) |
There was a problem hiding this comment.
I'm on the fence about the name route_id here, since if you look down the factory call stack, it winds up in a field on the InformedEntity called route, not route_id.
I'm torn between two thoughts:
- On the one hand, the thing being passed in is not a route object - it's just a route ID.
- This is why I started out naming it
route_id.
- This is why I started out naming it
- On the other hand, this factory interaction is the tests' entrypoint into the Alert ecosystem, and in the Alert ecosystem, that field is called
route, notroute_id, so for consistency, it ought to be calledrouteeverywhere in the Alert ecosystem. It's kind of arbitrary that (right now), the moment where it switches fromroute_idtorouteis in theInformedEntityfactory.- This is why I'm leaning towards changing the name to
route, and having the place where it switches fromroute_idtoroutebe the place where the factory is first invoked.
- This is why I'm leaning towards changing the name to
Follow-up to: