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.

Sign up for GitHub

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

fix(Schedules): Fix alerts not showing up for stops with inbound trains #2029

Merged
merged 4 commits into from
May 9, 2024
Merged

fix(Schedules): Fix alerts not showing up for stops with inbound trains #2029

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d46c3fa
fix(Schedules): Fix alerts not showing up for stops with inbound trains
kotva006 May 3, 2024
74bae7d
Added a test
kotva006 May 3, 2024
bda0fac
Fixed test to use only mox
kotva006 May 7, 2024
4996beb
Rewokred test, and addressed feedback
kotva006 May 8, 2024
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
33 changes: 22 additions & 11 deletions lib/predictions/repo.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ defmodule Predictions.Repo do
opts
|> add_all_optional_params()
|> cache_fetch()
|> filter_by_min_time(Keyword.get(opts, :min_time))
|> filter_predictions(Keyword.get(opts, :min_time))
|> load_from_other_repos
end

def all_no_cache(opts) when is_list(opts) and opts != [] do
opts
|> add_all_optional_params()
|> fetch()
|> filter_predictions(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing nil here you can just make it a default argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using a default argument of nil rather than passing in nil makes both the function definition and it's application much clearer and cleaner.

|> load_from_other_repos
end

Expand All @@ -53,6 +54,18 @@ defmodule Predictions.Repo do
end
end

@spec filter_predictions([Parser.record()] | {:error, any}, DateTime.t() | nil) ::
[Parser.record()] | {:error, any}
defp filter_predictions({:error, error}, _) do
{:error, error}
end

defp filter_predictions(predictions, min_time) do
Enum.filter(predictions, fn prediction ->
has_departure_time?(prediction) && after_min_time?(prediction, min_time)
end)
end

defp fetch(params) do
_ = Logger.info("predictions_repo_all_cache=cache_miss")

Expand Down Expand Up @@ -98,19 +111,17 @@ defmodule Predictions.Repo do
[]
end

@spec filter_by_min_time([Parser.record()] | {:error, any}, DateTime.t() | nil) ::
[Parser.record()] | {:error, any}
defp filter_by_min_time({:error, error}, _) do
{:error, error}
defp has_departure_time?(
{_id, _trip_id, _stop_id, _route_id, _direction_id, _arrival, departure, _time,
_stop_sequence, _schedule_relationship, _track, _status, _departing?,
_vehicle_id} = _prediction
) do
departure != nil
end

defp filter_by_min_time(predictions, nil) do
predictions
end
defp has_departure_time?(_), do: false

defp filter_by_min_time(predictions, %DateTime{} = min_time) do
Enum.filter(predictions, &after_min_time?(&1, min_time))
end
defp after_min_time?(_, nil), do: true

defp after_min_time?(
{
Expand Down
62 changes: 62 additions & 0 deletions test/predictions/repo_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ defmodule Predictions.RepoTest do
use ExUnit.Case, async: false
@moduletag :external

import Mock
Copy link
Contributor

Choose a reason for hiding this comment

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

We are removing Mock and replacing it with Mox. So, we shouldn't be adding new Mock instances to tests. We especially shouldn't use both in the same test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am aware, please see my comment in slack

import Mox

alias Predictions.Repo
Expand Down Expand Up @@ -58,6 +59,67 @@ defmodule Predictions.RepoTest do
end
end

test "filtes out predictions with no departure" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor

Choose a reason for hiding this comment

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

This typo is still here.

five_minutes_in_future = DateTime.add(Timex.now(), 5, :minute)

{:ok, five_minutes_in_future_string} =
Timex.format(five_minutes_in_future, "{ISO:Extended:Z}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you don't need the :ok and an error would just cause the test to fail you can do:

departure_time =
  Timex.now()
  |> DateTime.add(5, :minute)
  |> Timex.format!("{ISO:Extended:Z}")

expect(MBTA.Api.Mock, :get_json, fn _, _ ->
%JsonApi{
data: [
%JsonApi.Item{
attributes: %{
"departure_time" => nil,
"arrival_time" => five_minutes_in_future_string,
"direction_id" => 1
},
relationships: %{
"route" => [
%{
id: "Red"
}
],
"trip" => [],
"vehicle" => [],
"stop" => [
%{id: "StopID"}
]
}
},
%JsonApi.Item{
attributes: %{
"departure_time" => five_minutes_in_future_string,
"arrival_time" => five_minutes_in_future_string,
"direction_id" => 1
},
relationships: %{
"route" => [
%{
id: "Red"
}
],
"trip" => [],
"vehicle" => [],
"stop" => [
%{id: "StopID"}
]
}
}
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these attributes necessary for exercising the test? Would it work just setting the departure_time? These two structs are exactly the same except for that one attribute, so there should be a shorter and clearer way to communicate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all these fields are required. The parsing errors out if any of these fields are missing. Ideally I'd like to mock away some of that, but it seemed outside the scope of this fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

These two structs are exactly the same except for that one attribute, so there should be a shorter and clearer way to communicate that.

Because these two structs are identical except for one attribute, you could define it once and then override that attribute for the second. It will make the test shorter. That also helps to communicate the difference between the two.

end)

with_mocks([
{Stops.Repo, [:passthrough], get_parent: fn _ -> %Stops.Stop{id: "Parent"} end},
{Routes.Repo, [:passthrough], get: fn _ -> Routes.MockRepoApi.get("Red") end}
]) do
predictions = Repo.all(route: "39")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a magic string? What happens if we do Repo.all(route: "38")?

Copy link
Contributor Author

@kotva006 kotva006 May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nope, its just a random number


assert Kernel.length(predictions) == 1
end
end

test "returns a list even if the server is down" do
expect(MBTA.Api.Mock, :get_json, fn _, _ ->
{:error, %HTTPoison.Error{reason: :econnrefused}}
Expand Down
Loading