refactor: pattern-match over conditionals in Navigation and InsertOptimizer

Swaps `if`/`cond` branches for multi-clause function heads or head-pattern
matching, making the control flow visible in the function signatures:

- Navigation.determine_map_location: resolve_location/4 now dispatches on
  the shape of the geolocation input and whether URL params are explicit.
- Navigation.handle_callsign_tracking: per-case clauses for empty callsign
  and explicit-URL-params short-circuits, plus a final clause that does the
  DB lookup and rescues once.
- Navigation.handle_callsign_search: dispatch_callsign_search/3 separates
  the valid/invalid callsign branches.
- InsertOptimizer.optimize_based_on_performance: pattern-match on
  [_, _, _ | _] to require ≥3 samples without calling length/1.
- InsertOptimizer.calculate_insert_options and throughput helpers split
  into guarded clauses instead of inline `if`.
- maybe_emit_optimization uses same-var pattern matching to skip the
  telemetry hit when nothing changed.

Adds unit tests covering every branch of both modules plus SignalHandler,
LocaleHook, and the InsertOptimizer's :noproc fallback path. Coverage
64.69% → 65.72%.
This commit is contained in:
Graham McIntire 2026-04-23 13:38:19 -05:00
parent cbf9a9bb5c
commit c5a82b77c1
No known key found for this signature in database
GPG key ID: F4ABF488E6029E59
6 changed files with 457 additions and 87 deletions

View file

@ -75,14 +75,11 @@ defmodule Aprsme.Performance.InsertOptimizer do
@impl true
def handle_cast({:record_metrics, batch_size, duration_ms, success_count}, state) do
# Calculate throughput (packets per second)
throughput = if duration_ms > 0, do: success_count / (duration_ms / 1000), else: 0
metric = %{
batch_size: batch_size,
duration_ms: duration_ms,
success_count: success_count,
throughput: throughput,
throughput: throughput(success_count, duration_ms),
timestamp: System.monotonic_time(:millisecond)
}
@ -92,6 +89,11 @@ defmodule Aprsme.Performance.InsertOptimizer do
{:noreply, %{state | performance_history: new_history}}
end
# Packets-per-second, or 0 when the batch completed instantly (avoid div-by-0).
defp throughput(_count, 0), do: 0
defp throughput(_count, duration_ms) when duration_ms < 0, do: 0
defp throughput(count, duration_ms), do: count / (duration_ms / 1000)
@impl true
def handle_info(:optimize_settings, state) do
new_state = optimize_based_on_performance(state)
@ -103,43 +105,44 @@ defmodule Aprsme.Performance.InsertOptimizer do
Process.send_after(self(), :optimize_settings, @optimization_check_interval)
end
defp optimize_based_on_performance(state) do
if length(state.performance_history) >= 3 do
# Analyze recent performance
recent_metrics = Enum.take(state.performance_history, 5)
total_throughput = recent_metrics |> Enum.map(& &1.throughput) |> Enum.sum()
avg_throughput = total_throughput / length(recent_metrics)
total_duration = recent_metrics |> Enum.map(& &1.duration_ms) |> Enum.sum()
avg_duration = total_duration / length(recent_metrics)
# Require at least three samples before doing anything — match directly on the
# list prefix instead of calling length/1 so we short-circuit for short lists.
defp optimize_based_on_performance(%{performance_history: [_, _, _ | _] = history} = state) do
recent_metrics = Enum.take(history, 5)
sample_size = length(recent_metrics)
avg_throughput = average(recent_metrics, & &1.throughput, sample_size)
avg_duration = average(recent_metrics, & &1.duration_ms, sample_size)
# Adjust batch size based on performance
new_batch_size = calculate_optimal_batch_size(avg_throughput, avg_duration, state.current_batch_size)
new_batch_size = calculate_optimal_batch_size(avg_throughput, avg_duration, state.current_batch_size)
new_insert_options = calculate_insert_options(avg_duration)
# Adjust INSERT options based on system load
new_insert_options = calculate_insert_options(avg_duration)
Logger.debug(
"INSERT optimization: batch_size=#{new_batch_size}, avg_throughput=#{Float.round(avg_throughput, 2)} pps"
)
Logger.debug(
"INSERT optimization: batch_size=#{new_batch_size}, avg_throughput=#{Float.round(avg_throughput, 2)} pps"
)
:telemetry.execute([:aprsme, :insert_optimizer, :batch_size], %{value: new_batch_size}, %{})
:telemetry.execute([:aprsme, :insert_optimizer, :throughput], %{value: avg_throughput}, %{})
:telemetry.execute([:aprsme, :insert_optimizer, :duration], %{value: avg_duration}, %{})
maybe_emit_optimization(new_batch_size, state.current_batch_size)
# Emit telemetry for monitoring
:telemetry.execute([:aprsme, :insert_optimizer, :batch_size], %{value: new_batch_size}, %{})
:telemetry.execute([:aprsme, :insert_optimizer, :throughput], %{value: avg_throughput}, %{})
:telemetry.execute([:aprsme, :insert_optimizer, :duration], %{value: avg_duration}, %{})
if new_batch_size != state.current_batch_size do
:telemetry.execute([:aprsme, :insert_optimizer, :optimizations], %{count: 1}, %{})
end
%{
state
| current_batch_size: new_batch_size,
insert_options: new_insert_options,
last_optimization: System.monotonic_time(:millisecond)
}
else
%{
state
end
| current_batch_size: new_batch_size,
insert_options: new_insert_options,
last_optimization: System.monotonic_time(:millisecond)
}
end
defp optimize_based_on_performance(state), do: state
defp average(items, getter, count) do
items |> Enum.map(getter) |> Enum.sum() |> Kernel./(count)
end
defp maybe_emit_optimization(same, same), do: :ok
defp maybe_emit_optimization(_new, _old) do
:telemetry.execute([:aprsme, :insert_optimizer, :optimizations], %{count: 1}, %{})
end
defp calculate_optimal_batch_size(avg_throughput, avg_duration, current_batch_size) do
@ -158,21 +161,17 @@ defmodule Aprsme.Performance.InsertOptimizer do
end
end
defp calculate_insert_options(avg_duration) do
base_options = default_insert_options()
# If INSERTs are taking too long, use more aggressive optimizations
if avg_duration > 3000 do
Keyword.merge(base_options,
returning: false,
on_conflict: :nothing,
timeout: 30_000
)
else
base_options
end
# When INSERTs are slow (>3s avg), use a longer timeout; otherwise stick with defaults.
defp calculate_insert_options(avg_duration) when avg_duration > 3_000 do
Keyword.merge(default_insert_options(),
returning: false,
on_conflict: :nothing,
timeout: 30_000
)
end
defp calculate_insert_options(_avg_duration), do: default_insert_options()
defp default_insert_options do
[
# Don't return IDs unless needed

View file

@ -21,25 +21,32 @@ defmodule AprsmeWeb.MapLive.Navigation do
def determine_map_location(params, session) do
{url_center, url_zoom} = UrlParams.parse_map_params(params)
has_explicit_url_params = UrlParams.has_explicit_url_params?(params)
geo = session["ip_geolocation"]
Logger.info(
"determine_map_location: session ip_geolocation=#{inspect(session["ip_geolocation"])}, has_explicit_url_params=#{has_explicit_url_params}"
"determine_map_location: session ip_geolocation=#{inspect(geo)}, has_explicit_url_params=#{has_explicit_url_params}"
)
case session["ip_geolocation"] do
%{"lat" => lat, "lng" => lng} when is_number(lat) and is_number(lng) ->
if has_explicit_url_params do
Logger.info("Using URL params over geolocation: center=#{inspect(url_center)}, zoom=#{url_zoom}")
{url_center, url_zoom, false}
else
Logger.info("Using IP geolocation: lat=#{lat}, lng=#{lng}, zoom=11")
{%{lat: lat, lng: lng}, 11, true}
end
resolve_location(geo, has_explicit_url_params, url_center, url_zoom)
end
_ ->
Logger.info("No IP geolocation in session, using defaults")
{url_center, url_zoom, !has_explicit_url_params}
end
# Explicit URL params always win over geolocation.
defp resolve_location(_geo, true, url_center, url_zoom) do
Logger.info("Using URL params over geolocation: center=#{inspect(url_center)}, zoom=#{url_zoom}")
{url_center, url_zoom, false}
end
# Valid geolocation + no explicit URL params → use the geolocation.
defp resolve_location(%{"lat" => lat, "lng" => lng}, false, _url_center, _url_zoom)
when is_number(lat) and is_number(lng) do
Logger.info("Using IP geolocation: lat=#{lat}, lng=#{lng}, zoom=11")
{%{lat: lat, lng: lng}, 11, true}
end
# No usable geolocation → fall back to URL/defaults.
defp resolve_location(_geo, has_explicit_url_params, url_center, url_zoom) do
Logger.info("No IP geolocation in session, using defaults")
{url_center, url_zoom, !has_explicit_url_params}
end
@doc """
@ -47,27 +54,20 @@ defmodule AprsmeWeb.MapLive.Navigation do
Returns {final_map_center, final_map_zoom}.
"""
@spec handle_callsign_tracking(binary(), map(), integer(), boolean()) :: {map(), integer()}
def handle_callsign_tracking(tracked_callsign, map_center, map_zoom, has_explicit_url_params) do
if tracked_callsign != "" and not has_explicit_url_params do
try do
case Packets.get_latest_packet_for_callsign(tracked_callsign) do
%{lat: lat, lon: lon} when is_number(lat) and is_number(lon) ->
{%{lat: lat, lng: lon}, 12}
def handle_callsign_tracking("", map_center, map_zoom, _), do: {map_center, map_zoom}
def handle_callsign_tracking(_, map_center, map_zoom, true), do: {map_center, map_zoom}
_ ->
{map_center, map_zoom}
end
rescue
# Handle database connection errors gracefully (especially in tests)
DBConnection.OwnershipError ->
{map_center, map_zoom}
def handle_callsign_tracking(tracked_callsign, map_center, map_zoom, false) do
case Packets.get_latest_packet_for_callsign(tracked_callsign) do
%{lat: lat, lon: lon} when is_number(lat) and is_number(lon) ->
{%{lat: lat, lng: lon}, 12}
_ ->
{map_center, map_zoom}
end
else
{map_center, map_zoom}
_ ->
{map_center, map_zoom}
end
rescue
# Handle database connection errors gracefully (especially in tests).
_error -> {map_center, map_zoom}
end
@doc """
@ -77,12 +77,15 @@ defmodule AprsmeWeb.MapLive.Navigation do
def handle_callsign_search("", socket), do: {:noreply, socket}
def handle_callsign_search(callsign, socket) do
if ParamUtils.valid_callsign?(callsign) do
# Navigate to the new URL structure with the callsign as a path param
{:noreply, LiveView.push_navigate(socket, to: "/#{callsign}")}
else
{:noreply, LiveView.put_flash(socket, :error, "Invalid callsign format")}
end
dispatch_callsign_search(ParamUtils.valid_callsign?(callsign), callsign, socket)
end
defp dispatch_callsign_search(true, callsign, socket) do
{:noreply, LiveView.push_navigate(socket, to: "/#{callsign}")}
end
defp dispatch_callsign_search(false, _callsign, socket) do
{:noreply, LiveView.put_flash(socket, :error, "Invalid callsign format")}
end
@doc """

View file

@ -0,0 +1,153 @@
defmodule Aprsme.Performance.InsertOptimizerTest do
use ExUnit.Case, async: true
alias Aprsme.Performance.InsertOptimizer
defp fresh_state do
%{
current_batch_size: 200,
insert_options: [returning: false, on_conflict: :nothing, timeout: 15_000],
performance_history: [],
last_optimization: System.monotonic_time(:millisecond)
}
end
defp metric(throughput, duration_ms) do
%{
batch_size: 200,
duration_ms: duration_ms,
success_count: 100,
throughput: throughput,
timestamp: System.monotonic_time(:millisecond)
}
end
describe "GenServer callbacks" do
test "handle_call :get_batch_size returns current batch size" do
state = fresh_state()
assert {:reply, 200, ^state} =
InsertOptimizer.handle_call(:get_batch_size, self(), state)
end
test "handle_call :get_insert_options returns insert_options" do
state = fresh_state()
opts = state.insert_options
assert {:reply, ^opts, ^state} = InsertOptimizer.handle_call(:get_insert_options, self(), state)
end
test "handle_cast records a new metric in performance_history" do
state = fresh_state()
{:noreply, new_state} =
InsertOptimizer.handle_cast({:record_metrics, 200, 1_000, 100}, state)
assert [entry | _] = new_state.performance_history
assert entry.batch_size == 200
assert entry.duration_ms == 1_000
assert entry.success_count == 100
# Throughput = 100 / (1000/1000) = 100 pps
assert entry.throughput == 100.0
end
test "handle_cast with zero duration avoids div-by-0, throughput is 0" do
state = fresh_state()
{:noreply, new_state} =
InsertOptimizer.handle_cast({:record_metrics, 100, 0, 50}, state)
assert [entry | _] = new_state.performance_history
assert entry.throughput == 0
end
test "performance history is capped at 20 entries" do
state = fresh_state()
final =
Enum.reduce(1..25, state, fn _, s ->
{:noreply, s2} = InsertOptimizer.handle_cast({:record_metrics, 200, 1000, 100}, s)
s2
end)
assert length(final.performance_history) == 20
end
end
describe "handle_info :optimize_settings" do
test "with fewer than 3 metrics, state is unchanged except for re-scheduled timer" do
state = %{fresh_state() | performance_history: [metric(100, 1000), metric(90, 1100)]}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
# Batch size shouldn't have moved.
assert new_state.current_batch_size == state.current_batch_size
assert new_state.performance_history == state.performance_history
end
test "grows batch size when throughput is high and duration low" do
# With avg_throughput > 200 and avg_duration < 2000, batch size should scale up
# from 200 → min(@max, round(200 * 1.2)) = 240.
history =
Enum.map(1..5, fn _ -> metric(300, 1000) end)
state = %{fresh_state() | performance_history: history}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
assert new_state.current_batch_size == 240
# Fast INSERTs keep default options.
assert Keyword.get(new_state.insert_options, :timeout) == 15_000
end
test "shrinks batch size when throughput is low and duration high" do
# avg_throughput < 50 and avg_duration > 5000 → batch_size drops by 20%.
history = Enum.map(1..5, fn _ -> metric(10, 6000) end)
state = %{fresh_state() | performance_history: history}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
assert new_state.current_batch_size == 160
# Slow INSERTs bump timeout.
assert Keyword.get(new_state.insert_options, :timeout) == 30_000
end
test "keeps batch size when performance is middling" do
# 100 pps at 3000ms falls in neither bucket → no change to batch size.
history = Enum.map(1..5, fn _ -> metric(100, 3000) end)
state = %{fresh_state() | performance_history: history}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
assert new_state.current_batch_size == 200
end
test "never drops below the minimum batch size" do
state = %{
fresh_state()
| current_batch_size: 110,
performance_history: Enum.map(1..5, fn _ -> metric(5, 10_000) end)
}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
assert new_state.current_batch_size == 100
end
test "never exceeds the maximum batch size" do
state = %{
fresh_state()
| current_batch_size: 750,
performance_history: Enum.map(1..5, fn _ -> metric(500, 500) end)
}
{:noreply, new_state} = InsertOptimizer.handle_info(:optimize_settings, state)
assert new_state.current_batch_size == 800
end
end
describe "client API when GenServer isn't running" do
test "get_optimal_batch_size falls back to base size on :noproc" do
# The supervised instance isn't started in the app tree, so this should
# hit the catch clause and return the base batch size (200).
assert InsertOptimizer.get_optimal_batch_size() == 200
end
test "get_insert_options falls back to default options on :noproc" do
opts = InsertOptimizer.get_insert_options()
assert Keyword.get(opts, :returning) == false
assert Keyword.get(opts, :on_conflict) == :nothing
assert Keyword.get(opts, :timeout) == 15_000
end
end
end

View file

@ -0,0 +1,16 @@
defmodule Aprsme.SignalHandlerTest do
use ExUnit.Case, async: true
alias Aprsme.SignalHandler
describe "handle_info/2" do
test "ignores unrecognized messages without changing state" do
state = %{some: :state}
assert SignalHandler.handle_info(:random, state) == {:noreply, state}
assert SignalHandler.handle_info({:other, "msg"}, state) == {:noreply, state}
end
# We can't safely exercise the :sigterm clause here — it calls
# Aprsme.ShutdownHandler.shutdown() which would actually shut the app down.
end
end

View file

@ -0,0 +1,87 @@
defmodule AprsmeWeb.LocaleHookTest do
# on_mount/4 mutates the process-wide Gettext locale and reads app env;
# serialize with anything else that does the same.
use ExUnit.Case, async: false
alias AprsmeWeb.LocaleHook
alias Phoenix.LiveView.Socket
setup do
original_env = Application.get_env(:aprsme, :env)
original_locale = Gettext.get_locale(AprsmeWeb.Gettext)
on_exit(fn ->
Application.put_env(:aprsme, :env, original_env)
Gettext.put_locale(AprsmeWeb.Gettext, original_locale)
end)
:ok
end
defp bare_socket do
%Socket{
assigns: %{__changed__: %{}},
private: %{}
}
end
describe "on_mount/4 in production" do
setup do
Application.put_env(:aprsme, :env, :prod)
:ok
end
test "sets a supported locale from session" do
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{"locale" => "es"}, bare_socket())
assert socket.assigns.locale == "es"
assert Gettext.get_locale(AprsmeWeb.Gettext) == "es"
end
test "falls back to 'en' for unsupported session locale" do
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{"locale" => "zz"}, bare_socket())
assert socket.assigns.locale == "en"
end
test "falls back to 'en' when session has no locale" do
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{}, bare_socket())
assert socket.assigns.locale == "en"
end
test "accepts all declared supported locales" do
for loc <- ["en", "es", "de", "fr"] do
{:cont, socket} =
LocaleHook.on_mount(:set_locale, %{}, %{"locale" => loc}, bare_socket())
assert socket.assigns.locale == loc
end
end
end
describe "on_mount/4 in non-prod" do
test "leaves locale assign as nil" do
Application.put_env(:aprsme, :env, :dev)
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{"locale" => "es"}, bare_socket())
assert socket.assigns.locale == nil
end
end
describe "on_mount/4 map_page assign" do
test "map_page is false for non-MapLive socket" do
Application.put_env(:aprsme, :env, :dev)
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{}, bare_socket())
assert socket.assigns.map_page == false
end
test "map_page is true when the socket's view is MapLive.Index" do
Application.put_env(:aprsme, :env, :dev)
socket = %Socket{
assigns: %{__changed__: %{}},
private: %{phoenix_live_view: %{view: AprsmeWeb.MapLive.Index}}
}
{:cont, socket} = LocaleHook.on_mount(:set_locale, %{}, %{}, socket)
assert socket.assigns.map_page == true
end
end
end

View file

@ -0,0 +1,112 @@
defmodule AprsmeWeb.MapLive.NavigationTest do
# Logger.info calls are fine during tests; we don't silence them.
use ExUnit.Case, async: true
alias AprsmeWeb.MapLive.Navigation
alias Phoenix.LiveView.Socket
describe "determine_map_location/2" do
test "explicit URL params win over IP geolocation" do
params = %{"lat" => "40.0", "lng" => "-95.0", "z" => "7"}
session = %{"ip_geolocation" => %{"lat" => 10.0, "lng" => 20.0}}
assert {%{lat: 40.0, lng: -95.0}, 7, false} =
Navigation.determine_map_location(params, session)
end
test "valid IP geolocation used when no explicit URL params" do
session = %{"ip_geolocation" => %{"lat" => 45.5, "lng" => -122.6}}
assert {%{lat: 45.5, lng: -122.6}, 11, true} = Navigation.determine_map_location(%{}, session)
end
test "missing or invalid geolocation falls back to URL defaults" do
# No geolocation and no URL params → default center + skip_initial_url_update=true
assert {%{lat: _, lng: _}, _zoom, true} = Navigation.determine_map_location(%{}, %{})
# Geolocation present but with non-numeric values → fall back
session = %{"ip_geolocation" => %{"lat" => "not-a-number", "lng" => "nope"}}
assert {%{lat: _, lng: _}, _zoom, true} = Navigation.determine_map_location(%{}, session)
end
test "URL params win even with malformed geolocation" do
session = %{"ip_geolocation" => %{"lat" => "oops", "lng" => "oops"}}
params = %{"lat" => "10.0"}
{_, _, skip?} = Navigation.determine_map_location(params, session)
assert skip? == false
end
end
describe "handle_callsign_tracking/4" do
@default_center %{lat: 10.0, lng: 20.0}
@default_zoom 5
test "empty callsign returns unchanged center and zoom" do
assert Navigation.handle_callsign_tracking("", @default_center, @default_zoom, false) ==
{@default_center, @default_zoom}
assert Navigation.handle_callsign_tracking("", @default_center, @default_zoom, true) ==
{@default_center, @default_zoom}
end
test "when explicit URL params are set, tracking is bypassed" do
assert Navigation.handle_callsign_tracking("K5ABC", @default_center, @default_zoom, true) ==
{@default_center, @default_zoom}
end
test "returns defaults when the database lookup raises" do
# With a callsign that won't match any test data and no DB ownership,
# the rescue clause returns the defaults.
assert Navigation.handle_callsign_tracking(
"NONEXISTENT-99",
@default_center,
@default_zoom,
false
) == {@default_center, @default_zoom}
end
end
describe "handle_callsign_search/2" do
test "empty string returns the socket untouched" do
socket = %Socket{assigns: %{__changed__: %{}}}
assert {:noreply, ^socket} = Navigation.handle_callsign_search("", socket)
end
test "invalid callsign puts a flash error" do
socket = %Socket{assigns: %{__changed__: %{}, flash: %{}}}
{:noreply, result} = Navigation.handle_callsign_search("HELLO WORLD", socket)
assert result.assigns.flash["error"] == "Invalid callsign format"
end
test "valid callsign triggers push_navigate" do
socket = %Socket{
assigns: %{__changed__: %{}, flash: %{}},
redirected: nil
}
{:noreply, result} = Navigation.handle_callsign_search("K5ABC-9", socket)
# push_navigate/2 sets the `redirected` field on the socket.
assert result.redirected
end
end
describe "update_and_zoom_to_location/4" do
test "sets map_center and map_zoom assigns, and pushes a zoom event" do
socket = %Socket{assigns: %{__changed__: %{}}}
result = Navigation.update_and_zoom_to_location(socket, 30.5, -95.0, 8)
assert result.assigns.map_center == %{lat: 30.5, lng: -95.0}
assert result.assigns.map_zoom == 8
end
end
describe "zoom_to_current_location/1" do
test "pushes a zoom_to_location event using the socket's center/zoom" do
socket = %Socket{
assigns: %{__changed__: %{}, map_center: %{lat: 1.0, lng: 2.0}, map_zoom: 10}
}
# The function returns the socket (with push_event recorded).
result = Navigation.zoom_to_current_location(socket)
assert %Socket{} = result
end
end
end