From 96836ae715c756dfebeaadb39ac513e263817c40 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Fri, 27 Mar 2026 15:46:31 +0000 Subject: [PATCH] Improve tests: better assertions, meaningful tests Track resulting principles in project conventions --- docs/project-conventions.md | 7 +++ test/music_library/artists/batch_test.exs | 24 ++++++-- test/music_library/barcode_scan_test.exs | 2 +- test/music_library/maintenance_test.exs | 13 +---- test/music_library/scrobble_activity_test.exs | 57 ++++++++----------- .../artist_refresh_all_discogs_data_test.exs | 7 +-- ...tist_refresh_all_musicbrainz_data_test.exs | 7 +-- ...artist_refresh_all_wikipedia_data_test.exs | 7 +-- .../artist_refresh_discogs_data_test.exs | 2 +- .../artist_refresh_wikipedia_data_test.exs | 4 +- .../worker/fetch_artist_image_test.exs | 3 +- .../record_generate_all_embeddings_test.exs | 7 +-- ...cord_refresh_all_musicbrainz_data_test.exs | 7 +-- .../record_refresh_music_brainz_data_test.exs | 2 +- .../worker/refresh_cover_test.exs | 6 +- .../worker/repo_optimize_test.exs | 11 ---- .../collection_controller_test.exs | 38 +++++-------- .../controllers/error_html_test.exs | 15 ----- .../controllers/error_json_test.exs | 12 ---- 19 files changed, 91 insertions(+), 140 deletions(-) delete mode 100644 test/music_library/worker/repo_optimize_test.exs delete mode 100644 test/music_library_web/controllers/error_html_test.exs delete mode 100644 test/music_library_web/controllers/error_json_test.exs diff --git a/docs/project-conventions.md b/docs/project-conventions.md index c3126030..077f3a56 100644 --- a/docs/project-conventions.md +++ b/docs/project-conventions.md @@ -73,6 +73,13 @@ Rules extracted from commit history that are specific to this project and not al - **Verify outcomes through context modules**, not just UI assertions. Delete tests assert both `refute has_element?` and `assert_raise Ecto.NoResultsError`. - **`render_hook/3`** for testing JS hook interactions. - Avoid starting test descriptions with "it". +- **No boilerplate-only tests.** Do not add test files that just verify Phoenix generator output (e.g., error view literal strings). Tests must exercise application behaviour. +- **Assert specific values, not just shape.** Prefer `assert data == expected` or `assert data["name"] == "Steven Wilson"` over `assert data != nil` or `assert {:ok, _} = result`. Wildcard matches (`_`) in assertions are a signal the test is too vague. +- **Worker tests that enqueue jobs must `assert_enqueued`.** `perform_job` returning `{:ok, []}` is not sufficient — verify the expected downstream workers were enqueued with correct args. +- **Do not test the same guard at every call site.** If a shared check (e.g., session key presence) is enforced in one place, test it once. Do not duplicate the same assertion across every function that calls the shared check. +- **Consolidate identical assertions across endpoints.** When multiple routes share the same plug/middleware behaviour (e.g., auth), test it once with a loop or parameterised approach, not N separate identical tests. +- **Error assertions must match the error type.** `assert {:error, _reason}` is too broad — match the specific error struct or atom (e.g., `%Req.TransportError{reason: :timeout}`, `:no_session_key`). +- **Do not test untestable operations.** If a database operation (e.g., VACUUM) cannot run in the test sandbox, do not write a test that asserts the sandbox error message. Delete or skip it. ## Tech Debt / Hygiene diff --git a/test/music_library/artists/batch_test.exs b/test/music_library/artists/batch_test.exs index ad0a6d7c..ea6a8f63 100644 --- a/test/music_library/artists/batch_test.exs +++ b/test/music_library/artists/batch_test.exs @@ -8,31 +8,43 @@ defmodule MusicLibrary.Artists.BatchTest do setup do record = record() artist = hd(record.artists) - _artist_info = artist_info(artist.musicbrainz_id) - :ok + artist_info = artist_info(artist.musicbrainz_id) + %{artist_info: artist_info} end describe "refresh_musicbrainz_data/0" do - test "enqueues refresh jobs for all artist infos" do + test "enqueues refresh jobs for all artist infos", %{artist_info: artist_info} do assert {:ok, []} = Batch.refresh_musicbrainz_data() + + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshMusicBrainzData, + args: %{id: artist_info.id} end end describe "refresh_discogs_data/0" do - test "enqueues refresh jobs for all artist infos" do + test "enqueues refresh jobs for all artist infos", %{artist_info: artist_info} do assert {:ok, []} = Batch.refresh_discogs_data() + + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshDiscogsData, + args: %{id: artist_info.id} end end describe "refresh_wikipedia_data/0" do - test "enqueues refresh jobs for all artist infos" do + test "enqueues refresh jobs for all artist infos", %{artist_info: artist_info} do assert {:ok, []} = Batch.refresh_wikipedia_data() + + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshWikipediaData, + args: %{id: artist_info.id} end end describe "refresh_lastfm_data/0" do - test "enqueues refresh jobs for all artist infos" do + test "enqueues refresh jobs for all artist infos", %{artist_info: artist_info} do assert {:ok, []} = Batch.refresh_lastfm_data() + + assert_enqueued worker: MusicLibrary.Worker.FetchArtistLastFmData, + args: %{id: artist_info.id} end end end diff --git a/test/music_library/barcode_scan_test.exs b/test/music_library/barcode_scan_test.exs index 2ecb5199..d6946ac2 100644 --- a/test/music_library/barcode_scan_test.exs +++ b/test/music_library/barcode_scan_test.exs @@ -115,7 +115,7 @@ defmodule MusicLibrary.BarcodeScanTest do Req.Test.transport_error(conn, :timeout) end) - assert {:error, _reason} = BarcodeScan.scan("5052205070023") + assert {:error, %Req.TransportError{reason: :timeout}} = BarcodeScan.scan("5052205070023") end end diff --git a/test/music_library/maintenance_test.exs b/test/music_library/maintenance_test.exs index d5196d42..97d2a88c 100644 --- a/test/music_library/maintenance_test.exs +++ b/test/music_library/maintenance_test.exs @@ -5,18 +5,9 @@ defmodule MusicLibrary.MaintenanceTest do alias MusicLibrary.Maintenance - describe "vacuum/0" do - test "delegates to Repo.vacuum/0" do - # VACUUM cannot run inside the Ecto sandbox transaction, - # so we verify it attempts the operation and returns the expected tuple shape. - assert {:error, %Exqlite.Error{message: "cannot VACUUM from within a transaction"}} = - Maintenance.vacuum() - end - end - describe "optimize/0" do - test "returns {:ok, _}" do - assert {:ok, _} = Maintenance.optimize() + test "runs PRAGMA optimize on the database" do + assert {:ok, %Exqlite.Result{command: :execute}} = Maintenance.optimize() end end diff --git a/test/music_library/scrobble_activity_test.exs b/test/music_library/scrobble_activity_test.exs index ff58ada4..53554b3b 100644 --- a/test/music_library/scrobble_activity_test.exs +++ b/test/music_library/scrobble_activity_test.exs @@ -57,14 +57,34 @@ defmodule MusicLibrary.ScrobbleActivityTest do end end - describe "scrobble_release/3 without session key" do + describe "all scrobble operations without session key" do setup [:stub_lastfm_success] - test "returns error" do - started_at = DateTime.utc_now() + test "scrobble_release/3 returns :no_session_key" do + assert {:error, :no_session_key} = + ScrobbleActivity.scrobble_release(@release, :started_at, DateTime.utc_now()) + end + + test "scrobble_medium/4 returns :no_session_key" do + assert {:error, :no_session_key} = + ScrobbleActivity.scrobble_medium(1, @release, :started_at, DateTime.utc_now()) + end + + test "scrobble_tracks/4 returns :no_session_key" do + track_ids = + @release + |> Release.tracks() + |> Enum.take(1) + |> Enum.map(& &1.id) + |> MapSet.new() assert {:error, :no_session_key} = - ScrobbleActivity.scrobble_release(@release, :started_at, started_at) + ScrobbleActivity.scrobble_tracks( + track_ids, + @release, + :started_at, + DateTime.utc_now() + ) end end @@ -114,17 +134,6 @@ defmodule MusicLibrary.ScrobbleActivityTest do end end - describe "scrobble_medium/4 without session key" do - setup [:stub_lastfm_success] - - test "returns error" do - started_at = DateTime.utc_now() - - assert {:error, :no_session_key} = - ScrobbleActivity.scrobble_medium(1, @release, :started_at, started_at) - end - end - describe "scrobble_tracks/4" do setup [:store_session_key, :stub_lastfm_success] @@ -189,24 +198,6 @@ defmodule MusicLibrary.ScrobbleActivityTest do end end - describe "scrobble_tracks/4 without session key" do - setup [:stub_lastfm_success] - - test "returns error" do - track_ids = - @release - |> Release.tracks() - |> Enum.take(1) - |> Enum.map(& &1.id) - |> MapSet.new() - - started_at = DateTime.utc_now() - - assert {:error, :no_session_key} = - ScrobbleActivity.scrobble_tracks(track_ids, @release, :started_at, started_at) - end - end - describe "scrobble construction" do setup [:store_session_key] diff --git a/test/music_library/worker/artist_refresh_all_discogs_data_test.exs b/test/music_library/worker/artist_refresh_all_discogs_data_test.exs index d1d4be7f..9cdabc3a 100644 --- a/test/music_library/worker/artist_refresh_all_discogs_data_test.exs +++ b/test/music_library/worker/artist_refresh_all_discogs_data_test.exs @@ -9,13 +9,12 @@ defmodule MusicLibrary.Worker.ArtistRefreshAllDiscogsDataTest do test "enqueues refresh jobs for all artist infos" do record = record() artist = hd(record.artists) - _artist_info = artist_info(artist.musicbrainz_id) + artist_info = artist_info(artist.musicbrainz_id) assert {:ok, []} = perform_job(ArtistRefreshAllDiscogsData, %{}) - end - test "succeeds with no artist infos" do - assert {:ok, []} = perform_job(ArtistRefreshAllDiscogsData, %{}) + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshDiscogsData, + args: %{id: artist_info.id} end end end diff --git a/test/music_library/worker/artist_refresh_all_musicbrainz_data_test.exs b/test/music_library/worker/artist_refresh_all_musicbrainz_data_test.exs index ffbfe2ce..c63f89d5 100644 --- a/test/music_library/worker/artist_refresh_all_musicbrainz_data_test.exs +++ b/test/music_library/worker/artist_refresh_all_musicbrainz_data_test.exs @@ -9,13 +9,12 @@ defmodule MusicLibrary.Worker.ArtistRefreshAllMusicBrainzDataTest do test "enqueues refresh jobs for all artist infos" do record = record() artist = hd(record.artists) - _artist_info = artist_info(artist.musicbrainz_id) + artist_info = artist_info(artist.musicbrainz_id) assert {:ok, []} = perform_job(ArtistRefreshAllMusicBrainzData, %{}) - end - test "succeeds with no artist infos" do - assert {:ok, []} = perform_job(ArtistRefreshAllMusicBrainzData, %{}) + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshMusicBrainzData, + args: %{id: artist_info.id} end end end diff --git a/test/music_library/worker/artist_refresh_all_wikipedia_data_test.exs b/test/music_library/worker/artist_refresh_all_wikipedia_data_test.exs index 29bd32c9..0231e39d 100644 --- a/test/music_library/worker/artist_refresh_all_wikipedia_data_test.exs +++ b/test/music_library/worker/artist_refresh_all_wikipedia_data_test.exs @@ -9,13 +9,12 @@ defmodule MusicLibrary.Worker.ArtistRefreshAllWikipediaDataTest do test "enqueues refresh jobs for all artist infos" do record = record() artist = hd(record.artists) - _artist_info = artist_info(artist.musicbrainz_id) + artist_info = artist_info(artist.musicbrainz_id) assert {:ok, []} = perform_job(ArtistRefreshAllWikipediaData, %{}) - end - test "succeeds with no artist infos" do - assert {:ok, []} = perform_job(ArtistRefreshAllWikipediaData, %{}) + assert_enqueued worker: MusicLibrary.Worker.ArtistRefreshWikipediaData, + args: %{id: artist_info.id} end end end diff --git a/test/music_library/worker/artist_refresh_discogs_data_test.exs b/test/music_library/worker/artist_refresh_discogs_data_test.exs index 7488b754..45e51763 100644 --- a/test/music_library/worker/artist_refresh_discogs_data_test.exs +++ b/test/music_library/worker/artist_refresh_discogs_data_test.exs @@ -24,7 +24,7 @@ defmodule MusicLibrary.Worker.ArtistRefreshDiscogsDataTest do assert {:ok, _} = perform_job(ArtistRefreshDiscogsData, %{"id" => artist_info.id}) updated = Artists.get_artist_info!(artist_info.id) - assert updated.discogs_data != nil + assert updated.discogs_data == ArtistFixture.get_artist() end test "returns ok when no discogs data is available" do diff --git a/test/music_library/worker/artist_refresh_wikipedia_data_test.exs b/test/music_library/worker/artist_refresh_wikipedia_data_test.exs index 060e28d5..bf9f9a7d 100644 --- a/test/music_library/worker/artist_refresh_wikipedia_data_test.exs +++ b/test/music_library/worker/artist_refresh_wikipedia_data_test.exs @@ -34,8 +34,8 @@ defmodule MusicLibrary.Worker.ArtistRefreshWikipediaDataTest do assert {:ok, _} = perform_job(ArtistRefreshWikipediaData, %{"id" => artist_info.id}) updated = Artists.get_artist_info!(artist_info.id) - assert updated.wikipedia_data != nil - assert updated.wikipedia_data != %{} + assert is_map(updated.wikipedia_data) + assert Map.has_key?(updated.wikipedia_data, "intro_html") end test "discards job when no wikidata_id exists in musicbrainz_data" do diff --git a/test/music_library/worker/fetch_artist_image_test.exs b/test/music_library/worker/fetch_artist_image_test.exs index 532a0f38..73ecb652 100644 --- a/test/music_library/worker/fetch_artist_image_test.exs +++ b/test/music_library/worker/fetch_artist_image_test.exs @@ -24,7 +24,8 @@ defmodule MusicLibrary.Worker.FetchArtistImageTest do assert :ok = perform_job(FetchArtistImage, %{"id" => artist_info.id}) updated = Artists.get_artist_info!(artist_info.id) - assert updated.image_data_hash != nil + assert is_binary(updated.image_data_hash) and byte_size(updated.image_data_hash) > 0 + assert MusicLibrary.Assets.get(updated.image_data_hash) != nil end test "cancels when no discogs data exists" do diff --git a/test/music_library/worker/record_generate_all_embeddings_test.exs b/test/music_library/worker/record_generate_all_embeddings_test.exs index 53c83c8b..9572f846 100644 --- a/test/music_library/worker/record_generate_all_embeddings_test.exs +++ b/test/music_library/worker/record_generate_all_embeddings_test.exs @@ -7,13 +7,12 @@ defmodule MusicLibrary.Worker.RecordGenerateAllEmbeddingsTest do describe "perform/1" do test "enqueues embedding generation jobs for all records" do - _record = record() + record = record() assert {:ok, []} = perform_job(RecordGenerateAllEmbeddings, %{}) - end - test "succeeds with no records" do - assert {:ok, []} = perform_job(RecordGenerateAllEmbeddings, %{}) + assert_enqueued worker: MusicLibrary.Worker.GenerateRecordEmbedding, + args: %{record_id: record.id} end end end diff --git a/test/music_library/worker/record_refresh_all_musicbrainz_data_test.exs b/test/music_library/worker/record_refresh_all_musicbrainz_data_test.exs index 2e901b7a..bbdf9569 100644 --- a/test/music_library/worker/record_refresh_all_musicbrainz_data_test.exs +++ b/test/music_library/worker/record_refresh_all_musicbrainz_data_test.exs @@ -7,13 +7,12 @@ defmodule MusicLibrary.Worker.RecordRefreshAllMusicBrainzDataTest do describe "perform/1" do test "enqueues refresh jobs for all records" do - _record = record() + record = record() assert {:ok, []} = perform_job(RecordRefreshAllMusicBrainzData, %{}) - end - test "succeeds with no records" do - assert {:ok, []} = perform_job(RecordRefreshAllMusicBrainzData, %{}) + assert_enqueued worker: MusicLibrary.Worker.RecordRefreshMusicBrainzData, + args: %{id: record.id} end end end diff --git a/test/music_library/worker/record_refresh_music_brainz_data_test.exs b/test/music_library/worker/record_refresh_music_brainz_data_test.exs index 99f5de1b..79cf5e40 100644 --- a/test/music_library/worker/record_refresh_music_brainz_data_test.exs +++ b/test/music_library/worker/record_refresh_music_brainz_data_test.exs @@ -24,7 +24,7 @@ defmodule MusicLibrary.Worker.RecordRefreshMusicBrainzDataTest do assert :ok = perform_job(RecordRefreshMusicBrainzData, %{"id" => record.id}) updated = Records.get_record!(record.id) - assert updated.musicbrainz_data != nil + assert updated.musicbrainz_data["title"] == "Marbles" end test "raises when record does not exist" do diff --git a/test/music_library/worker/refresh_cover_test.exs b/test/music_library/worker/refresh_cover_test.exs index 94bef1d9..d097bdde 100644 --- a/test/music_library/worker/refresh_cover_test.exs +++ b/test/music_library/worker/refresh_cover_test.exs @@ -18,8 +18,10 @@ defmodule MusicLibrary.Worker.RefreshCoverTest do assert :ok = perform_job(RefreshCover, %{"id" => record.id}) updated = Records.get_record!(record.id) - assert updated.cover_hash != nil - assert Assets.get(updated.cover_hash) != nil + assert is_binary(updated.cover_hash) and byte_size(updated.cover_hash) > 0 + + asset = Assets.get(updated.cover_hash) + assert asset.format == "image/jpeg" end test "raises when record does not exist" do diff --git a/test/music_library/worker/repo_optimize_test.exs b/test/music_library/worker/repo_optimize_test.exs deleted file mode 100644 index f55f8f25..00000000 --- a/test/music_library/worker/repo_optimize_test.exs +++ /dev/null @@ -1,11 +0,0 @@ -defmodule MusicLibrary.Worker.RepoOptimizeTest do - use MusicLibrary.DataCase - - alias MusicLibrary.Worker.RepoOptimize - - describe "perform/1" do - test "runs optimize on the repo" do - assert {:ok, %Exqlite.Result{}} = perform_job(RepoOptimize, %{}) - end - end -end diff --git a/test/music_library_web/controllers/collection_controller_test.exs b/test/music_library_web/controllers/collection_controller_test.exs index 4189e565..e07fe376 100644 --- a/test/music_library_web/controllers/collection_controller_test.exs +++ b/test/music_library_web/controllers/collection_controller_test.exs @@ -12,15 +12,23 @@ defmodule MusicLibraryWeb.CollectionControllerTest do |> Keyword.fetch!(:api_token) end + describe "authentication" do + test "all API endpoints require a bearer token", %{conn: conn} do + for path <- [ + ~p"/api/collection/latest", + ~p"/api/collection/random", + ~p"/api/collection", + ~p"/api/collection/on_this_day" + ] do + assert get(conn, path).status == 401, + "expected 401 for unauthenticated GET #{path}" + end + end + end + describe "GET /api/collection/latest" do setup [:create_record] - test "requires authentication", %{conn: conn} do - conn = get(conn, ~p"/api/collection/latest") - - assert conn.status == 401 - end - test "returns the latest record", %{conn: conn, record: record} do conn = conn @@ -34,12 +42,6 @@ defmodule MusicLibraryWeb.CollectionControllerTest do describe "GET /api/collection/random" do setup [:create_record] - test "requires authentication", %{conn: conn} do - conn = get(conn, ~p"/api/collection/random") - - assert conn.status == 401 - end - # We're not testing random here - the query is solid enough test "returns a random record", %{conn: conn, record: record} do conn = @@ -54,12 +56,6 @@ defmodule MusicLibraryWeb.CollectionControllerTest do describe "GET /api/collection" do setup [:create_record] - test "requires authentication", %{conn: conn} do - conn = get(conn, ~p"/api/collection") - - assert conn.status == 401 - end - test "returns a paginated list of records", %{conn: conn, record: record} do conn = conn @@ -78,12 +74,6 @@ defmodule MusicLibraryWeb.CollectionControllerTest do describe "GET /api/collection/on_this_day" do setup [:create_record] - test "requires authentication", %{conn: conn} do - conn = get(conn, ~p"/api/collection/on_this_day") - - assert conn.status == 401 - end - test "returns a list of records", %{conn: conn, record: record} do conn = conn diff --git a/test/music_library_web/controllers/error_html_test.exs b/test/music_library_web/controllers/error_html_test.exs deleted file mode 100644 index fffedf38..00000000 --- a/test/music_library_web/controllers/error_html_test.exs +++ /dev/null @@ -1,15 +0,0 @@ -defmodule MusicLibraryWeb.ErrorHTMLTest do - use MusicLibraryWeb.ConnCase, async: true - - # Bring render_to_string/4 for testing custom views - import Phoenix.Template - - test "renders 404.html" do - assert render_to_string(MusicLibraryWeb.ErrorHTML, "404", "html", []) == "Not Found" - end - - test "renders 500.html" do - assert render_to_string(MusicLibraryWeb.ErrorHTML, "500", "html", []) == - "Internal Server Error" - end -end diff --git a/test/music_library_web/controllers/error_json_test.exs b/test/music_library_web/controllers/error_json_test.exs deleted file mode 100644 index b0373859..00000000 --- a/test/music_library_web/controllers/error_json_test.exs +++ /dev/null @@ -1,12 +0,0 @@ -defmodule MusicLibraryWeb.ErrorJSONTest do - use MusicLibraryWeb.ConnCase, async: true - - test "renders 404" do - assert MusicLibraryWeb.ErrorJSON.render("404.json", %{}) == %{errors: %{detail: "Not Found"}} - end - - test "renders 500" do - assert MusicLibraryWeb.ErrorJSON.render("500.json", %{}) == - %{errors: %{detail: "Internal Server Error"}} - end -end