test(data): add Testcontainers integration tests for LeaderboardRepository

[yanmoyy]

Description:

This PR introduces Testcontainers integration tests for the LeaderboardRepository using an in-memory SQLite container, addressing #30. The tests cover:

• TestSaveScore: Verifies that a score is saved with a valid ID.
• TestAllScores: Tests retrieving and sorting multiple marathon scores by score DESC, time ASC.

The implementation is inspired by the structure outlined in the Testcontainers for Go getting started guide.

Please let me know if this aligns with your testing vision or if there are specific edge cases you’d like to see added.

Summary by CodeRabbit

• Chores
  • Upgraded Go toolchain to 1.24 and refreshed linting/tooling configurations.
  • Updated dependencies to current versions.
• Tests
  • Added containerized integration tests for leaderboard persistence.
  • Tightened assertions across test suites.
  • Ensured Docker setup and verification on macOS in CI.
• Refactor
  • Adopted context-aware database operations.
  • Simplified internal loops and predicates for clarity.
• Continuous Integration
  • Updated lint, test, and release workflows for newer environments and reliability.

[Broderick-Westrope]

@yanmoyy this PR is increasing the Go version in go.mod. I don't mind this, but you will need to update the version in all the github workflows and other places. You should simply be able to search for 1.23 in the project and update it.

[yanmoyy]

Thanks, I will fix the version.

[yanmoyy]

@Broderick-Westrope
I'm updating the Go version (1.23 -> 1.24), because the latest Testcontainers release (v0.39.0) requires a minimum of Go 1.24 for compatibility.

While making this change, I noticed that Testcontainers relies on Docker to run tests, so $task test passes with Docker running but fails without it, and this could affect Github Actions since Docker isn't pre-installed.

I'd like your guidance on handling this, I'm considering two options:

1. Setting up Docker in Github workflow to enable integration test.
2. Skipping Testcontainers tests in CI by detecting the environment variable, relying on local Docker testing.

I lean toward Option 1 for consistent integration testing, but I’m open to your preference based on the project’s goals.
Please let me know which approach you’d prefer or if you have another suggestion.

[Broderick-Westrope]

@yanmoyy thanks for raising this. It seems here that we should be able to run test containers in the workflow without further configuration. This page is for DotNet but I think the workflow requirements would be the same. Could you please push the other fixes and we will see if there is any issues with the workflow test containers.

[yanmoyy]

@Broderick-Westrope It seems like every tests on each OS failed because of no Docker.

(in macos)
--- FAIL: TestLeaderboardTestSuite (0.01s)
    docker_host.go:91: test panicked: rootless Docker not found

I checked the documentation, which says:

Azure Pipelines

Microsoft-hosted agents come with Docker pre-installed, there is no need for any additional configuration.

GitHub Actions

GitHub-hosted runners have the same configuration as Microsoft-hosted agents

I will keep trying to figure this out. If you have any comments, please let me know.

[coderabbitai]

Walkthrough

Bumps Go toolchain to 1.24.x across CI and tooling, updates golangci-lint action/configs, adjusts dependencies in go.mod, introduces context-aware DB calls, adds a testcontainers-backed SQLite test suite and utility, refactors minor Tetris logic, and standardizes tests to assert.Equal.

Changes

Cohort / File(s)	Summary
CI Workflows .github/workflows/lint.yaml, .github/workflows/release.yaml, .github/workflows/test.yaml	Go version updated to 1.24.x; golangci-lint action moved to v8 with golangci-lint v2.1.0; test matrix bumped; macOS runners now install and verify Docker before Go setup.
Lint Configurations .golangci.yaml, .golangci.bck.yaml	Replaced/added comprehensive golangci-lint configs: set version "2", explicit enables, tuned per-linter settings, exclusions, formatter settings; added backup config with detailed enable/disable catalog and rules.
Toolchain & Build go.mod, Taskfile.yaml	go.mod toolchain to 1.24.0; upgraded testify to v1.11.1; added testcontainers-go; refreshed indirect deps; Taskfile GOLANG_CROSS_VERSION to v1.24.
Data Layer: Context use internal/data/data.go, internal/data/leaderboard.go	Switched DB operations to ExecContext/QueryContext with context.TODO(); imports updated accordingly.
Data Tests & Test Utility internal/data/leaderboard_test.go, internal/data/testutils/container.go	Added SQLite container utility using testcontainers; new leaderboard test suite initializes container/DB, runs save/list tests, and cleans up.
Test Assertion Standardization internal/tui/views/single_test.go, pkg/tetris/matrix_test.go, pkg/tetris/next_queue_test.go, pkg/tetris/tetrimino_test.go	Replaced assert.EqualValues with assert.Equal across tests; no logic changes.
Tetris Logic Refactors pkg/tetris/matrix.go, pkg/tetris/modes/single/single.go	Simplified isLineComplete using slices.ContainsFunc; refactored ghost move loop to condition-based form; behavior unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant TS as LeaderboardTestSuite
  participant TU as CreateSQLiteContainer
  participant TC as testcontainers
  participant DB as SQLite (container)
  participant Repo as Leaderboard Repository

  Dev->>TS: go test
  rect rgb(240,248,255)
    note right of TS: Suite setup
    TS->>TU: CreateSQLiteContainer(ctx)
    TU->>TC: Start container (nouchka/sqlite3)
    TC-->>TU: Container ready (log: "SQLite version")
    TU-->>TS: {Container, DSN}
  end

  rect rgb(245,255,240)
    note right of TS: Test: Save
    TS->>Repo: Save(score) using DSN
    Repo->>DB: ExecContext(INSERT)
    DB-->>Repo: result (id)
    Repo-->>TS: id
  end

  rect rgb(255,248,240)
    note right of TS: Test: List
    TS->>Repo: All(mode)
    Repo->>DB: QueryContext(SELECT … ORDER BY score DESC, time ASC)
    DB-->>Repo: rows
    Repo-->>TS: []Score
  end

  TS->>TC: Terminate container (teardown)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

In burrows of code I twitch my nose,
New Go seeds sprout where the pipeline flows.
Lints hop higher, tests in containers spin—
Context whispers as queries begin.
Ghost blocks glide with quieter grace,
I thump approval—ship this race! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "test(data): add Testcontainers integration tests for LeaderboardRepository" succinctly and accurately summarizes the primary change: adding Testcontainers-based integration tests for the leaderboard repository, using a conventional commit-style prefix and scope and avoiding extraneous details. It is clear and specific enough for a reviewer scanning history to understand the main intent of the PR.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/tetris/matrix_test.go (1)

37-39: Swap assert.EqualError args.

Current call compares tc.wantErr to err.Error(); pass the actual error first.

-assert.EqualError(t, tc.wantErr, err.Error())
+assert.EqualError(t, err, tc.wantErr.Error())

🧹 Nitpick comments (13)

pkg/tetris/modes/single/single.go (1)

295-296: Idiomatic loop; consider a brief comment to clarify the empty body

Switch to condition-based loop is a clean, idiomatic improvement. To appease linters and future readers, add a short comment inside the empty body.

-	for g.ghostTet.MoveDown(g.matrix) {
-	}
+	for g.ghostTet.MoveDown(g.matrix) {
+		// keep dropping ghost until blocked
+	}

.github/workflows/release.yaml (2)

23-23: Go 1.24.x bump in release pipeline looks good

Consider enabling module cache to speed releases: add with.cache: true and cache-dependency-path: '**/go.sum' to actions/setup-go.

-      - name: Setup Go
-        uses: actions/setup-go@v5
-        with:
-          go-version: 1.24.x
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: 1.24.x
+          cache: true
+          cache-dependency-path: '**/go.sum'

---

29-31: Verify secret name for GoReleaser

Ensure secrets.GH_PAT exists; otherwise prefer the built-in GITHUB_TOKEN which already has contents: write in this job.

-        env:
-          GITHUB_TOKEN: ${{secrets.GH_PAT}}
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

internal/data/data.go (1)

4-4: Avoid context.TODO in production paths

Prefer Background (or a timeout) over TODO for table creation.

-	_, err := db.ExecContext(
-		context.TODO(),
+	_, err := db.ExecContext(
+		context.Background(),
 		`CREATE TABLE IF NOT EXISTS leaderboard 
 (id INTEGER PRIMARY KEY, game_mode TEXT, name TEXT, time INTEGER, score INTEGER, lines INTEGER, level INTEGER)`,
 	)

Optionally thread a ctx into EnsureTablesExist and NewDB to allow caller‑controlled timeouts.

Also applies to: 27-31

pkg/tetris/matrix_test.go (1)

223-224: Fix misleading subtest names.

Case names say “true/false; left/right” but expected values are opposite for the right-boundary cases, which is confusing in CI output.

Apply:

-  "true; right": {
+  "false; right (col 9 in-bounds)": {
     9, false,
   },
-  "false; right": {
+  "true; right (col 10 out-of-bounds)": {
     10, true,
   },

Also applies to: 200-216

.golangci.yaml (1)

1-1: Config looks consistent with v2; watch for redundant complexity linters.

Enabling both cyclop and gocyclo can yield duplicate findings; consider keeping one.

Also applies to: 3-3, 68-154, 181-192

internal/data/leaderboard.go (2)

54-57: Same for INSERT: switch to ? placeholders.

Keeps driver-agnostic and avoids subtle binding issues.

-res, err := r.db.ExecContext(context.TODO(),
-    "INSERT INTO leaderboard (game_mode, name, time, score, lines, level) VALUES ($1, $2, $3, $4, $5, $6)",
-    score.GameMode, score.Name, score.Time, score.Score, score.Lines, score.Level,
-)
+res, err := r.db.ExecContext(context.Background(),
+    "INSERT INTO leaderboard (game_mode, name, time, score, lines, level) VALUES (?, ?, ?, ?, ?, ?)",
+    score.GameMode, score.Name, score.Time, score.Score, score.Lines, score.Level,
+)

Also applies to: 61-66

---

28-31: Propagate context from callers (follow-up).

Prefer method signatures All(ctx context.Context, gameMode string) and Save(ctx context.Context, score *Score) instead of context.TODO().

I can draft the repo-wide refactor if you want it in this PR or a follow-up.

Also applies to: 54-57

.github/workflows/lint.yaml (1)

12-12: CI note re Testcontainers across OSes.

Docker isn’t available on GitHub macOS/Windows runners. Run integration tests only on ubuntu-latest or guard with if: runner.os == 'Linux'.

internal/data/testutils/container.go (1)

22-25: Flakiness risk: startup timeout too low.

Even if you keep this, 5s is tight on CI pulls; bump to ≥60s.

internal/data/leaderboard_test.go (2)

36-45: TearDown should not log.Fatalf; also don’t depend on container.

Prefer suite.Require().NoError and remove container termination if you drop it.

-err := suite.repo.db.Close()
-if err != nil {
-  log.Fatalf("error closing db: %s", err)
-}
-err = suite.container.Terminate(suite.ctx)
-if err != nil {
-  log.Fatalf("error terminating SQLite container: %s", err)
-}
+suite.Require().NoError(suite.repo.db.Close())

---

98-115: Add assertions for Rank and game-mode filtering; test tie-breakers.

You already verify order; also assert Rank fields and add a case where scores tie and times decide.

I can extend the suite with these cases if you want.

.golangci.bck.yaml (1)

1-8: Consider removing backup config from repo root.

Keeping both .golangci.yaml and .golangci.bck.yaml may confuse contributors; store backups outside the repo or as a Gist.

Also applies to: 182-255

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5439210 and c836cc9.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (17)

• .github/workflows/lint.yaml (1 hunks)
• .github/workflows/release.yaml (1 hunks)
• .github/workflows/test.yaml (1 hunks)
• .golangci.bck.yaml (1 hunks)
• .golangci.yaml (1 hunks)
• Taskfile.yaml (1 hunks)
• go.mod (2 hunks)
• internal/data/data.go (2 hunks)
• internal/data/leaderboard.go (3 hunks)
• internal/data/leaderboard_test.go (1 hunks)
• internal/data/testutils/container.go (1 hunks)
• internal/tui/views/single_test.go (1 hunks)
• pkg/tetris/matrix.go (2 hunks)
• pkg/tetris/matrix_test.go (4 hunks)
• pkg/tetris/modes/single/single.go (1 hunks)
• pkg/tetris/next_queue_test.go (1 hunks)
• pkg/tetris/tetrimino_test.go (9 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

internal/data/leaderboard_test.go (3)

internal/data/testutils/container.go (2)

• SQLiteContainer (13-16)
• CreateSQLiteContainer (19-38)

internal/data/leaderboard.go (3)

• LeaderboardRepository (20-22)
• NewLeaderboardRepository (24-26)
• Score (9-18)

internal/data/data.go (1)

• NewDB (11-23)

internal/data/testutils/container.go (1)

internal/tui/testutils/testutils.go (1)

• SetupInMemoryDB (34-41)

internal/tui/views/single_test.go (1)

internal/data/leaderboard.go (1)

• Score (9-18)

🔇 Additional comments (13)

pkg/tetris/modes/single/single.go (1)

3-8: CI/toolchain aligned — Go 1.24 and Docker present for Testcontainers
go.mod declares go 1.24.0; .github workflows use actions/setup-go with go-version 1.24.x; the test workflow verifies Docker and sets up Docker on macOS; testcontainers-go v0.39.0 is required and math/rand/v2 is used in code and tests.

pkg/tetris/matrix.go (1)

6-6: Simpler line-complete check using slices.ContainsFunc

Change is correct and preserves semantics (ghost cells still count as empty). Nice cleanup.

Also applies to: 60-61

pkg/tetris/next_queue_test.go (1)

75-76: Tightening assertions to Equal

Good switch from EqualValues to Equal for stricter type checks.

Also applies to: 82-83

pkg/tetris/tetrimino_test.go (1)

75-75: Consistent move to assert.Equal across tests

LGTM. Improves type strictness without changing behavior.

Also applies to: 164-164, 252-252, 396-396, 1117-1117, 1917-1918, 3404-3405, 3545-3545, 3651-3653, 3664-3666

internal/tui/views/single_test.go (1)

238-247: Stricter struct equality in leaderboard handoff

Equal over EqualValues is appropriate here. 👍

Taskfile.yaml (1)

4-4: GOLANG_CROSS_VERSION updated to v1.24

LGTM; automated sandbox search couldn't confirm absence of 1.23 references (ripgrep reported "No files were searched"). Confirm there are no remaining "1.23", "go1.23", or "v1.23" occurrences across the repo by running locally: git grep -nE '(\b1.23\b|go1.23|v1.23)' || rg -nP '(\b1.23\b|go1.23|v1.23)'.

pkg/tetris/matrix_test.go (3)

194-195: LGTM on equality change.

Switch to assert.Equal is correct for matrix structural equality.

---

380-381: LGTM on equality change.

assert.Equal here verifies full matrix state post-mutation.

---

456-457: LGTM on equality change.

Keeps removal semantics strict.

.github/workflows/lint.yaml (1)

17-17: LGTM; matches prior guidance on versions.

Go 1.24.x and golangci-lint action v8/v2.1.0 look right.

Also applies to: 23-26

.golangci.yaml (1)

184-192: Don't use a top-level formatters.goimports — use formatters.enable + formatters.settings (golangci-lint v2).

golangci-lint v2 (including v2.1.0) rejects a top-level formatters.goimports; enable formatters via formatters.enable and put formatter options under formatters.settings.

Likely an incorrect or invalid review comment.

go.mod (1)

16-18: Verify ecosystem upgrade impact (testcontainers + testify)

• Good adds.
• go.mod declares go 1.24.0 and workflows (.github/workflows/lint.yaml, release.yaml, test.yaml) target Go 1.24.x; no Dockerfiles reference older golang.
• No module-caching steps detected in workflows — add/update actions/cache or use setup-go's cache input to cache Go modules and speed CI.

internal/data/leaderboard.go (1)

41-44: leaderboard.time: schema is INTEGER — ensure scan/insert units match

internal/data/data.go creates leaderboard.time as INTEGER; scanning into time.Duration is safe only if inserts store an integer nanosecond count. Actionable options:

• If inserts write d.Nanoseconds() (int64) — keep scanning but prefer scanning into an int64 and converting: var ns int64; rows.Scan(..., &ns, ...); s.Time = time.Duration(ns).
• If inserts store milliseconds or TEXT — change read/write accordingly (use milliseconds→time.Duration(ms)*time.Millisecond or scan to string then parse).

Locations: internal/data/data.go (CREATE TABLE), internal/data/leaderboard.go (rows.Scan).

[yanmoyy]

Hi @Broderick-Westrope,

I apologize for the numerous trial commits on this branch. I was trying to set up Docker on a macOS runner in GitHub Actions. The last commit uses the best solution I found: douglascamata/setup-docker-macos-action@v1.0.0.

However, the macOS-latest runner does not seem to support Docker due to M-series processor limitations, as discussed in:

• Community Discussion 1
• Community Discussion 2

I'm unsure of the best approach, but I’ve considered two options:

1. Skip Docker-related tests (e.g., data layer tests) on macOS using an environment variable.
2. Run tests on a single OS (e.g., ubuntu-latest), as our application logic appears to be OS-independent.

I believe both options are valid. If you have feedback or prefer one approach, please let me know, and I’ll finalize the workflow after locally testing the GitHub Action with act.

[Broderick-Westrope]

@yanmoyy

No need for an apology; a pull request is a place you can experiment with the changes you'd like to propose.

I'd like to keep the tests running for each OS so that we can ensure there are no differences (as I've found once or twice during this project). With this in mind, I think the easiest way to do what we want to do would be with an environment variable that skips tests which rely on test containers.

Two things to note for these new tests:

• Let's make sure we test as much of the code as possible on each operating system. This means we should only do the environment variable skip for integration tests which must rely on Test Containers.
• That said, don't let this limit how many tests you create that use Test Containers since more tests is great :)

[yanmoyy]

Same problem happens like before (#48).
As you can see from the test result, the failure occurs in internal/tui/views, not in internal/data, where I recently implemented Testcontainers support.

I think we need to fix this issue before we merge this branch.

[yanmoyy]

Hi @Broderick-Westrope,

I've extended the SKIP_TESTCONTAINERS environment variable to include windows-latest as well, since GitHub's Windows runners don't support Linux containers for Testcontainers due to a bridge plugin error.

I experimented with the windows-2025 runner, which uses WSL2, as windows-latest (based on Windows Server 2022) relies on WSL1. However, the test still failed with the same error (result).

After further investigation, it seems Windows runners on GitHub Actions may not fully support Linux containers, possibly due to default Windows container support (as noted on the Testcontainers website and StackOverflow question).

As a result, I updated the workflow to set SKIP_TESTCONTAINERS=true for Windows too.

Additionally, I added timeouts to the tests in internal/views/ to address intermittent failures, as seen in this error log. This change is reflected in this commit as an example.

I’m unsure if this is the optimal solution, but I believe limiting Testcontainers to Linux environments (e.g., ubuntu-latest) might be the most practical approach. But I'll keep investigating this issue...😢