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
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?
There was a problem hiding this comment.
💀 Well, now main has a Test.Support.Generators.DateTime.random_time_range_date_time({start, stop}) function that returns a time between start & stop that I suppose would work here
| 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
joshlarson
force-pushed
the
jdl/system-status-alert-factory
branch
from
4a97d25 to
e58865d
Compare
joshlarson
force-pushed
the
jdl/system-status-alert-factory
branch
from
e58865d to
9579171
Compare
...in order to make sure that the informed entity set is consistent, since new/1 does some processing that gets bypassed by the factory
…#2347) * cleanup: Add more alert factories for use in SystemStatus.SubwayTests * fix: Use InformedEntitySet.new/1 rather than its factory ...in order to make sure that the informed entity set is consistent, since new/1 does some processing that gets bypassed by the factory
Follow-up to: