feat: add repository integration tests for star/unstar songs

2026-08-01 07:21:17 +00:00 · 2026-05-22 15:43:03 -05:00 · 2026-05-22 15:43:03 -05:00 · 08c5193868
commit 08c5193868
parent fa4c9786aa
2 changed files with 331 additions and 0 deletions
--- a/docs/lab2-be-int-star-song.md
+++ b/docs/lab2-be-int-star-song.md
@ -0,0 +1,219 @@
+# Lab 2 — Integration Test Plan: BE-INT
+
+**Integration level:** 1
+**Boundary tested:** Backend ↔ Database (real persistence layer ↔ real migrated SQLite)
+**Scenario:** Star an existing song successfully.
+
+## Steps (scenario)
+
+1. Send an authenticated star request for an existing song ID.
+2. Read favorites back via the backend API / DB verification.
+
+**Expected result:** the song is starred and appears in the favorites/starred list.
+
+> **Constraint:** create new test files only — do not modify any existing test. This plan adds a
+> brand-new file and only *reads* the suite's already-seeded fixtures; nothing existing is changed.
+
+---
+
+## For readers new to Go / Go testing
+
+If you know software development in general but have never touched Go, Go's testing tools, or
+this codebase, this section gives you enough mental model to read the test. The ideas are the
+same ones you already know from other languages — only the names differ.
+
+### The big picture in one paragraph
+
+"Star a song" in Navidrome means *mark it as a favorite*. The backend stores that fact in a
+database table called `annotation` (one row per user + item, with a `starred` boolean and a
+`starred_at` timestamp). Our test asks a simple question: **if I tell the real backend code to
+star a real song in a real database, and then ask the backend to list my favorites, does the
+song show up?** That round-trip — write through real code, read back through real code — is what
+makes it an *integration* test rather than a unit test.
+
+### Go testing & Ginkgo, mapped to tools you already know
+
+Go has a built-in test runner (`go test`). On top of it, Navidrome uses **Ginkgo**, a
+behavior-style framework. If you've used JUnit, Jest, RSpec, or Mocha, this table will feel
+familiar:
+
+| In the test you'll see | What it means | Equivalent you may know |
+|------------------------|---------------|-------------------------|
+| `Describe("...", func(){...})` | Groups related test cases | `describe()` (Jest), test class (JUnit) |
+| `It("...", func(){...})` | One individual test case | `it()` / `test()` / `@Test` |
+| `BeforeEach(func(){...})` | Runs before *each* case — setup | `beforeEach` / `@BeforeEach` |
+| `AfterEach(func(){...})` | Runs after *each* case — teardown/cleanup | `afterEach` / `@AfterEach` |
+| `Expect(x).To(BeTrue())` | An assertion | `expect(x).toBe(true)` / `assertTrue(x)` |
+| `Expect(err).ToNot(HaveOccurred())` | Assert "no error happened" | `assertDoesNotThrow` |
+| `BeforeSuite` | One-time setup for the whole file/suite | `@BeforeAll` |
+
+`func(){...}` is just Go's syntax for an anonymous function (a lambda/closure). Ginkgo tests are
+written by passing these little functions into `Describe`/`It`/etc.
+
+In Go, **any file ending in `_test.go` is test code** and is compiled only when testing — it never
+ships in the product binary. That's why adding our file changes nothing about the running app.
+
+### The building blocks this test uses
+
+- **Repository** (`repo` in the code): an object that reads/writes one kind of data in the
+  database. Think DAO, or a single-table ORM repository. `MediaFileRepository` is "the songs
+  table, plus the favorites/ratings attached to songs." Its methods (`Get`, `GetAll`, `SetStar`)
+  are the *real backend code* — the same methods the web API calls in production.
+- **`SetStar(true, id)` / `SetStar(false, id)`**: the backend method that marks/unmarks a favorite.
+  It does a real SQL "update the row, or insert it if it doesn't exist yet" (an *upsert*) against
+  the `annotation` table.
+- **`Get(id)`**: fetch one song *with its favorite/rating info merged in* (via a SQL `JOIN`). So
+  `song.Starred` tells you whether the current user has favorited it.
+- **`GetAll(... starred = true ...)`**: fetch every song the current user has favorited — i.e. read
+  the favorites list. This is literally the query behind the app's "Starred / Favorites" screen.
+- **Fixtures / seed data**: before the tests run, the suite inserts a fixed set of sample songs and
+  users into the database (e.g. song `"1003"` = "Radioactivity", user `adminUser`). These are
+  pre-existing rows we can rely on — like a test database snapshot. We *read* them; we don't change
+  the setup.
+- **"The authenticated user"**: favorites are per-user. In the running app, login middleware figures
+  out *who* you are and attaches that to the request. In the test there's no HTTP login step;
+  instead we attach a known seeded user to a Go **`context`** (a request-scoped bag of values that
+  Go passes around). `request.WithUser(ctx, adminUser)` is the test's stand-in for "logged in as
+  this user." The backend code reads the user from that context exactly as it would in production.
+- **Real migrated database**: the test boots an actual SQLite database and runs Navidrome's real
+  schema migrations against it (in memory, so it's fast and disposable). No fake/hand-written
+  schema — the same tables the real app uses.
+
+### How to read the test, in plain steps
+
+Each test case follows the classic **Arrange → Act → Assert** shape:
+
+1. **Arrange** — get a repository wired to the real DB, acting as the seeded user; force a known
+   starting state (for the star case, make sure the song is *not* yet starred).
+2. **Act** — call the real backend method: `SetStar(true, songID)` to favorite it (or
+   `SetStar(false, songID)` to remove it).
+3. **Assert** — read it back two ways and check both agree: `Get(songID).Starred` is now `true`,
+   and the song appears in the favorites list returned by `GetAll(... starred=true ...)`.
+4. **Cleanup** — `AfterEach` un-stars the song again so the shared in-memory database is left clean
+   for the next test (tests must not leak state into each other).
+
+The unstar test (BE-INT-02) is the mirror image: arrange the song as *starred*, act with
+`SetStar(false, ...)`, then assert it's no longer starred and no longer in the favorites list.
+
+---
+
+## Context: what already exists vs. the real gap
+
+Three pieces of star/favorites test code exist today. None of them exercises the real
+backend persistence code against a real database, which is exactly the gap this test fills.
+
+| File | What it actually is | Real backend code? | Real DB? |
+|------|--------------------|--------------------|----------|
+| `server/subsonic/media_annotation_test.go` | **Unit** test — `tests.MockDataStore` + spy repo | Handler only | No (fully mocked) |
+| `tests/db/annotation_star_test.go` | **DB-only** test — raw SQL against a *hand-written* schema | No | A throwaway schema, not Navidrome's migrations |
+| `persistence/sql_annotations_test.go` | Real-DB test, but only covers the **filter SQL** + album rating | Filter helpers | Yes |
+
+So despite the `tests/db` file being *labeled* "integration", nothing currently exercises
+**real backend persistence code → real migrated SQLite, then reads it back**. That round-trip
+is the BE-INT-01 scenario and the legitimate value-add.
+
+---
+
+## Recommended approach — repository-level round-trip
+
+The `persistence` package test suite already boots a **real migrated DB** via `db.Init()` and
+seeds real fixtures in `BeforeSuite`: known songs (`"1001"` A Day In A Life, `"1003"`
+Radioactivity, etc.), albums, and users (`adminUser`, ID `"userid"`). We reuse that instead of
+hand-rolling a schema.
+
+**New file:** `persistence/star_song_integration_test.go` (lives in the `persistence` suite so it
+inherits the migrated DB + fixtures).
+
+```go
+ctx := request.WithUser(context.Background(), adminUser)   // the authenticated user
+repo := NewMediaFileRepository(ctx, GetDBXBuilder())
+```
+
+Steps mapped to the scenario:
+
+1. **Pre-condition** — pick a seeded existing song (e.g. ID `"1001"`); assert
+   `repo.Get("1001").Starred == false`.
+2. **Act (the "star request")** — `Expect(repo.SetStar(true, "1001")).To(Succeed())` → real
+   upsert into the `annotation` table.
+3. **Verify via DB read-back** (two complementary assertions):
+   - `repo.Get("1001")` → `.Starred == true` **and** `.StarredAt` is non-zero (proves the row +
+     timestamp persisted through the `LeftJoin annotation` query path).
+   - `repo.GetAll(model.QueryOptions{Filters: squirrel.Eq{"starred": true}})` — confirms `"1001"`
+     **appears in the starred/favorites list**. This is the *same query the `getStarred` Subsonic
+     endpoint runs* (`server/subsonic/album_lists.go:120` `getStarredItems` → `MediaFile.GetAll`
+     with the starred filter), so it is a faithful "read favorites" check.
+4. **Cleanup** — `AfterEach`: `repo.SetStar(false, "1001")` (or delete the annotation row) so the
+   shared in-memory DB stays clean for other specs.
+
+### Why this is valid / what it proves
+
+It exercises the real `sqlRepository.SetStar` → real squirrel SQL → real migrated `annotation`
+table, then reads back through the real annotated-query join — proving the backend's write and
+the backend's read agree against an actual database. The per-user scoping
+(`loggedUser(ctx).ID`) stands in for the authenticated user, which is exactly what the auth
+middleware produces downstream.
+
+### Value for the product
+
+Catches regressions that unit tests cannot: a broken migration, a wrong column name, a faulty
+upsert, or a starred-filter query that drifts from the write path. If this round-trip fails, the
+favorites feature is broken in production even if every unit test passes.
+
+---
+
+## Stronger (heavier) variant — full HTTP + auth
+
+For the literal "authenticated HTTP star request", wire a real `Router` with a real
+`persistence.New(...)` DataStore, seed a user *with a password*, then `router.ServeHTTP` a
+`GET /rest/star?u=...&t=...&s=...&id=1001` followed by `GET /rest/getStarred`. This adds the auth
+middleware + handler layers but costs noticeably more setup (real Router deps, password/token
+computation). Good as one "showcase" backend test; overkill for all five lab tests.
+
+---
+
+## How to run
+
+From the project root. Both BE-INT-01 (star) and BE-INT-02 (unstar) live in the same file,
+`persistence/star_song_integration_test.go`, under the `Star/Unstar an existing song` describe:
+
+```bash
+# Run both star and unstar specs
+go test -tags netgo,sqlite_fts5 -run TestPersistence ./persistence/ \
+  --ginkgo.focus="Star/Unstar an existing song"
+
+# Run only BE-INT-01 (star)
+go test -tags netgo,sqlite_fts5 -run TestPersistence ./persistence/ \
+  --ginkgo.focus="Star an existing song"
+
+# Run only BE-INT-02 (unstar)
+go test -tags netgo,sqlite_fts5 -run TestPersistence ./persistence/ \
+  --ginkgo.focus="Unstar a previously starred song"
+```
+
+Expected output: `ok  github.com/navidrome/navidrome/persistence`.
+
+### What each flag does
+
+- `-tags netgo,sqlite_fts5` — **required.** The DB migrations create an FTS5 virtual table; without
+  this build tag the SQLite driver lacks the `fts5` module and the schema fails to build
+  (`no such module: fts5`). These are the project's standard build tags (`GO_BUILD_TAGS` in the
+  Makefile).
+- `-run TestPersistence` — scopes the run to the real suite entry point, sidestepping the
+  pre-existing duplicate `RunSpecs` in `persistence/annotation_star_test.go` that otherwise fails
+  the whole package with "Rerunning Suite" (see the note below).
+- `--ginkgo.focus="..."` — runs only the specs whose description text matches the given pattern
+  (e.g. one of the three focuses shown above). Omit it to run every spec in the suite.
+
+To run this spec alongside the rest of the persistence suite, drop the focus flag:
+
+```bash
+go test -tags netgo,sqlite_fts5 -run TestPersistence ./persistence/
+```
+
+> **Pre-existing issue (not introduced by this test):** the file
+> `persistence/annotation_star_test.go` (committed in Lab 1, a stray duplicate of
+> `tests/db/annotation_star_test.go`) declares a *second* `RunSpecs` inside the `persistence`
+> package. Ginkgo allows only one `RunSpecs` per test binary, so a bare `go test ./persistence/`
+> fails with "Rerunning Suite". Scoping the run to `-run TestPersistence` (the real suite entry
+> point) sidesteps the duplicate and runs this spec cleanly. The proper fix is to remove/relocate
+> that stray file, but it is left untouched here per the "do not modify existing tests" constraint.
--- a/persistence/star_song_integration_test.go
+++ b/persistence/star_song_integration_test.go
@ -0,0 +1,112 @@
+package persistence
+
+import (
+	"context"
+
+	"github.com/Masterminds/squirrel"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/model/request"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+// Integration tests: Backend (real persistence layer) <-> Database (real migrated SQLite).
+//
+// No mocks, no stubs: SetStar issues real SQL against the migrated `annotation` table
+// seeded by the suite's BeforeSuite, and every verification reads it back through the
+// real annotated-query join — the same path the Subsonic getStarred endpoint uses.
+//
+// starredIDs returns the IDs currently in the favorites/starred list, using exactly the
+// filter that filter.ByStarred() applies in the getStarred endpoint.
+var _ = Describe("Star/Unstar an existing song (Backend<->DB integration)", func() {
+	var repo model.MediaFileRepository
+
+	starredIDs := func() []string {
+		starred, err := repo.GetAll(model.QueryOptions{Filters: squirrel.Eq{"starred": true}})
+		Expect(err).ToNot(HaveOccurred())
+		ids := make([]string, 0, len(starred))
+		for _, mf := range starred {
+			ids = append(ids, mf.ID)
+		}
+		return ids
+	}
+
+	BeforeEach(func() {
+		// The logged-in user scopes every annotation (loggedUser(ctx).ID). adminUser
+		// (ID "userid") is seeded and associated with the default test library, so it
+		// stands in for the authenticated user a real /rest/star request would carry.
+		ctx := request.WithUser(context.Background(), adminUser)
+		repo = NewMediaFileRepository(ctx, GetDBXBuilder())
+	})
+
+	// BE-INT-01 — Star an existing song successfully.
+	Describe("Star an existing song", func() {
+		// songRadioactivity (ID "1003") is an existing song seeded by BeforeSuite.
+		const songID = "1003"
+
+		BeforeEach(func() {
+			// Known starting state: the song must not be starred yet.
+			Expect(repo.SetStar(false, songID)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			// Leave the shared in-memory DB clean for other specs.
+			Expect(repo.SetStar(false, songID)).To(Succeed())
+		})
+
+		It("stars the song and returns it in the starred list", func() {
+			// Pre-condition: the existing song is reachable and not starred.
+			before, err := repo.Get(songID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(before.Starred).To(BeFalse())
+
+			// Act — the "star request": real upsert into the annotation table.
+			Expect(repo.SetStar(true, songID)).To(Succeed())
+
+			// Verify (1): re-reading the song through the real annotated query shows it
+			// persisted, with a starred_at timestamp.
+			after, err := repo.Get(songID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(after.Starred).To(BeTrue())
+			Expect(after.StarredAt).ToNot(BeNil())
+
+			// Verify (2): the song appears in the favorites/starred list.
+			Expect(starredIDs()).To(ContainElement(songID))
+		})
+	})
+
+	// BE-INT-02 — Unstar a previously starred song successfully.
+	Describe("Unstar a previously starred song", func() {
+		// songDayInALife (ID "1001") is an existing song seeded by BeforeSuite.
+		const songID = "1001"
+
+		BeforeEach(func() {
+			// Pre-condition for unstar: the song must already be starred.
+			Expect(repo.SetStar(true, songID)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			// Leave the shared in-memory DB clean for other specs.
+			Expect(repo.SetStar(false, songID)).To(Succeed())
+		})
+
+		It("unstars the song and removes it from the starred list", func() {
+			// Pre-condition: the existing song is reachable and currently starred.
+			before, err := repo.Get(songID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(before.Starred).To(BeTrue())
+
+			// Act — the "unstar request": real upsert into the annotation table.
+			Expect(repo.SetStar(false, songID)).To(Succeed())
+
+			// Verify (1): re-reading the song through the real annotated query shows it
+			// is no longer starred.
+			after, err := repo.Get(songID)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(after.Starred).To(BeFalse())
+
+			// Verify (2): the song no longer appears in the favorites/starred list.
+			Expect(starredIDs()).ToNot(ContainElement(songID))
+		})
+	})
+})