Improve tests: better assertions, meaningful tests

Track resulting principles in project conventions

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
 

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)
+    artist_info = artist_info(artist.musicbrainz_id)
-    :ok
+    %{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

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
 

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
+    test "runs PRAGMA optimize on the database" do
-      assert {:ok, _} = Maintenance.optimize()
+      assert {:ok, %Exqlite.Result{command: :execute}} = Maintenance.optimize()
     end
   end
 

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
+    test "scrobble_release/3 returns :no_session_key" do
-      started_at = DateTime.utc_now()
+      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]
 

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_enqueued worker: MusicLibrary.Worker.ArtistRefreshDiscogsData,
-      assert {:ok, []} = perform_job(ArtistRefreshAllDiscogsData, %{})
+                      args: %{id: artist_info.id}
     end
   end
 end

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_enqueued worker: MusicLibrary.Worker.ArtistRefreshMusicBrainzData,
-      assert {:ok, []} = perform_job(ArtistRefreshAllMusicBrainzData, %{})
+                      args: %{id: artist_info.id}
     end
   end
 end

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_enqueued worker: MusicLibrary.Worker.ArtistRefreshWikipediaData,
-      assert {:ok, []} = perform_job(ArtistRefreshAllWikipediaData, %{})
+                      args: %{id: artist_info.id}
     end
   end
 end

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

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 is_map(updated.wikipedia_data)
-      assert 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

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

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_enqueued worker: MusicLibrary.Worker.GenerateRecordEmbedding,
-      assert {:ok, []} = perform_job(RecordGenerateAllEmbeddings, %{})
+                      args: %{record_id: record.id}
     end
   end
 end

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_enqueued worker: MusicLibrary.Worker.RecordRefreshMusicBrainzData,
-      assert {:ok, []} = perform_job(RecordRefreshAllMusicBrainzData, %{})
+                      args: %{id: record.id}
     end
   end
 end

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

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 is_binary(updated.cover_hash) and byte_size(updated.cover_hash) > 0
-      assert Assets.get(updated.cover_hash) != nil
+
+      asset = Assets.get(updated.cover_hash)
+      assert asset.format == "image/jpeg"
     end
 
     test "raises when record does not exist" do

test/music_library/worker/repo_optimize_test.exs

@@ -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

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

test/music_library_web/controllers/error_html_test.exs

@@ -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

test/music_library_web/controllers/error_json_test.exs

@@ -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