Rewrite predictions pubsub tests #2129
Conversation
| {registry_key, predictions} = GenServer.call(server, {:subscribe, stream_topic}) | ||
| {registry_key, predictions} = GenServer.call(__MODULE__, {:subscribe, stream_topic}) |
There was a problem hiding this comment.
question: why not leave __MODULE__ as a default argument to this function? Then we can inject different dependencies to test if we'd like.
There was a problem hiding this comment.
Because we are moving away from DI and towards using Mox. We don't need to inject variables in dynamically. That is, it will never be called with a second argument.
| def init(_) do | ||
| Phoenix.PubSub.subscribe(@predictions_phoenix_pub_sub, "predictions") |
There was a problem hiding this comment.
question: why are we making this compile-time instead of a runtime option? It's conventional for callbacks like init to actually configure the process they're starting dynamically from the options being passed in.
There was a problem hiding this comment.
Because we're using Mox and determining the implementation at compile time.
| def clear(keys) do | ||
| GenServer.cast(__MODULE__, {:remove, Enum.map(fetch(keys), & &1.id)}) | ||
| end |
There was a problem hiding this comment.
suggestion: I would also suggest making __MODULE__ an argument to this function.
There was a problem hiding this comment.
Again, we aren't doing DI. We will never need to message a different GenServer.
| @@ -0,0 +1,24 @@ | |||
| defmodule Predictions.Store.Behaviour do | |||
There was a problem hiding this comment.
question: could we come up with a different name for this behaviour? I find it confusing to read code with @behaviour Behaviour everywhere, it makes it seem like this is some kind of generic library code for behaviours but it seems like it's actually a Predictions Store Behaviour.
There was a problem hiding this comment.
I actually think it is pretty straightforward. Predictions.Store implements the Predictions.Store.Behaviour. Makes it so that any time I want to find the custom behaviour being implemented, I know right where to look.
| {Phoenix.PubSub, [name: Predictions.PubSub]}, | ||
| {Phoenix.PubSub, [name: @predictions_phoenix_pub_sub]}, |
There was a problem hiding this comment.
same suggestion re: dynamic vs static configuration
There was a problem hiding this comment.
We are moving away from DI and toward using Mox. That's 50% of the purpose of this PR.
|
The tests around starting streams seem flaky. I'm going to write a helper that does assertions on a retry basis so we don't have to try to sleep for the 'right' amount of time. |
No ticket, this is just the test rewrite to go along with not showing predictions with no departure time.
Please reference: https://www.notion.so/mbta-downtown-crossing/Mocking-External-Dependencies-625665802fb84c8fa6e6edfa030d273f