refactor(TransitDetail): use trip struct #2260
Conversation
| icon_name = Route.icon_atom(route) | ||
| date = Date.to_iso8601(itinerary.start) | ||
| url = schedule_path(DotcomWeb.Endpoint, :show, route, date: date, trip: trip_id) | ||
| url = schedule_path(DotcomWeb.Endpoint, :show, route, date: date) |
There was a problem hiding this comment.
This was just a legacy thing, as far as I can tell putting a trip in the query parameters doesn't render anything different on a schedule page nowadays...
thecristen
force-pushed
the
cbj/trip-planner-trips
branch
from
81aa708 to
c9ad182
Compare
| @@ -72,26 +72,27 @@ defmodule DotcomWeb.Components.TripPlanner.TripPlannerResultsSection do | |||
| }} | |||
| } = assigns | |||
| ) do | |||
| %{itineraries: itineraries, summary: summary} = results |> Enum.at(itinerary_group_index) | |||
| with %{itineraries: itineraries, summary: summary} <- | |||
| results |> Enum.at(itinerary_group_index) do | |||
There was a problem hiding this comment.
Is this change necessary?
I only ask because Anthony is currently working on a refactor that will overhaul (possibly remove or rename?) this file, and this could generate git conflicts.
There was a problem hiding this comment.
I put it there because I ran into an error where this returned nil and failed the pattern match. hopefully the refactor will clearly obviate this!
| @@ -226,13 +226,13 @@ defmodule DotcomWeb.Live.TripPlannerTest do | |||
|
|
|||
| view |> element("button", "Details") |> render_click() | |||
|
|
|||
| assert render_async(view) =~ trip_id_1 | |||
| refute render_async(view) =~ trip_id_2 | |||
| assert render_async(view) =~ trip_headsign_1 | |||
There was a problem hiding this comment.
The tests here have become pretty flaky. (Wild guess) about one in four times I run them, one of these headsign assertions fails.
I'm not 100% certain, but I'm guessing that sometimes the factoried-up route is "external", which makes <.transit_leg /> look for the route long-name instead of the trip headsign.
I think the solution here is to force the factoried-up route in this test to not be external.
It would also be good to have explicit coverage of the use case where the route is an external agency. Currently undecided whether that should be approval-blocking or not.
At some point, I'd like us as a team to revisit the explicit versus implicit data in tests question, because I think that's causing flakiness elsewhere as well.
There was a problem hiding this comment.
Great catch, your assessment sounds likely correct to me. Maybe this would be easier to pull off after your refactor, if we could maybe stub out segments in tests instead of piecing togeher ever more intricate itineraries... hmmm.
In the meantime I'll hack internal routes into these tests.
There was a problem hiding this comment.
🤔 I think I'd vote in favor of hacking internal routes into the tests and keeping it that way, since the segment abstraction that I came up with is still very much an implementation detail (and new enough that it's not particularly stable).
There was a problem hiding this comment.
The new version of the tests are still flaky (actually flakier than they were before - now we're at 50%). This time I think it's because you removed the start_time logic, so now trip_headsign_1 sometimes happens before trip_headsign_2, but sometimes they happen in the other order, which means that half the time, the test is expecting the trips to be swapped.
I think the easiest solution would be to restore the overall test structure back to what it was before, replace update_trip_id() with update_headsign(), and just hard-code something in base_itinerary that forces it to not be an external agency.
There was a problem hiding this comment.
dang, yeah I forgot the test expects the list to be ordered.
I made a change where the test itineraries now have sequential start datetimes, maybe that'll be enough?
There was a problem hiding this comment.
Ahh there's one more issue (I'll push up a fix for this, since you're out for the weekend).
I was so confused by this failure:
assert render_async(view) =~ trip_headsign_1 # <-- passes
refute render_async(view) =~ trip_headsign_2 # <-- failsThere's basically no way that the code should allow both headsigns to appear on the screen at the same time - if something was wrong with the ordering, then I would have expected the assert statement to fail.
Turns out:
We're using Faker.App.name() as the headsign. But we're also using it elsewhere in factories that get rendered on the screen, so if we end up with something like:
trip_headsign_2 = Faker.App.name() # returns "Zathon"
# Elsewhere, deep in some factory...
list_item = Faker.App.name() # also happens to returns "Zathon"Then list_item will get rendered on the screen as Zathon, and refute render_async(view) =~ trip_headsign_2 will fail.
I'm pushing up a fix to have the headsigns be "Headsign:#{Faker.App.name()}" instead. I tested that out by running the tests 20 times, and they passed every time, so I think that completely resolves it.
There was a problem hiding this comment.
Mostly looks good, but the test flakiness is a tad too much for me!
I think once that's resolved, I'll be ready to ✅
Co-authored-by: Josh Larson <[email protected]>
Changing the
%TransitDetail{}spec to have a trip rather than just a trip_id.This'll set us up for displaying the trip headsign and trip name directly from OTP.
Made some more updates. This PR now also updates the preview Trip Planner so we can see headsigns instead of trip IDs. It also improves the transit leg's text to better match the acceptance criteria in Trip Planner Preview | Details: Transit segment