neon-sprawl/docs/plans/NS-21-implementation-plan.md

# NS-21 — Implementation plan

## Story reference

| Field | Value |
|--------|--------|
| **Key** | NS-21 |
| **Title** | Set up GDScript unit testing and cover existing client scripts |
| **Jira** | [NS-21](https://neon-sprawl.atlassian.net/browse/NS-21) |
| **Parent** | [NS-20 — Tech Debt](https://neon-sprawl.atlassian.net/browse/NS-20) |

## Goal, scope, and out-of-scope

**Goal:** Introduce **automated GDScript tests** for the Neon Sprawl Godot client (**Godot 4.6** per `client/project.godot`) and add **meaningful coverage** for scripts that exist today, alongside **contributor docs** and **CI** that runs the suite when client code or tests change.

**In scope**

- **Harness:** Add a maintained test stack (**GDUnit4** preferred for Godot 4.x headless + CI; **GUT** acceptable if GDUnit4 blocks on 4.6—decide during implementation and record in README). Vendor the addon under `client/addons/` (pinned release) so CI and clones match.
- **Documentation:** `client/README.md` (or a short dedicated doc under `client/` linked from README) with **install** (if any beyond opening the project), **local run commands**, and how tests are laid out.
- **CI:** At least one GitHub Actions job that runs the suite on **push/PR** to `main`, with **path filters** on `client/` (including `client/addons/` test plugin paths, `client/**/*.gd` tests, workflow file). Align **Godot binary version** with **4.6-stable** (same pattern as existing headless smoke notes in `client/README.md`).
- **Tests:** Cover current behavior where feasible for:
  - `client/scripts/player.gd` — public API: `snap_to_server`, `set_authoritative_nav_goal`, `clear_nav_goal`, and **physics-process outcomes** that can be asserted with a **minimal scene tree** (CharacterBody3D + child NavigationAgent3D matching `$NavigationAgent3D`) without the full `main.tscn`.
  - `client/scripts/position_authority_client.gd` — **HTTP flow** via a **test double** (subclass of `HTTPRequest` or injected node) that invokes `request_completed` with canned bodies/status codes: boot `GET` 200 → `authoritative_position_received` with `apply_as_snap == true`; `POST` 400 → `move_rejected`; happy `POST` 200 → `VERIFY_GET` → `GET` 200 → second arg `false`. Cover `_base_root` / URL trimming indirectly through successful request URLs if practical.
  - `client/scripts/ground_pick.gd` — **`_collider_is_walkable`** (walkable group ancestry) using a **minimal node hierarchy** under test; **steep-segment / wall cutoff** behavior only if we can do it without brittle full viewport ray setup—otherwise **document** deferral to manual / future scene test.
- **Lint alignment:** Keep **gdlint** / **gdformat** green; extend lint paths if new roots are added (e.g. `client/test/`).

**Out of scope**

- **Full integration** tests of `main.gd` / `main.tscn` (navigation bake timing, label timers, full pick pipeline)—**intentionally deferred** here: thin composition + `await` chains are brittle without a dedicated scene test harness; README should state that explicitly (per Jira).
- Replacing **server** tests or adding **E2E** browser/gameplay automation.
- Changing **production** gameplay contracts (NS-16/19/23) except where a test exposes a real bug (unlikely in this story).

## Acceptance criteria checklist

- [ ] README or short doc under `client/` explains **install** (if needed) and **run** commands for the test suite.
- [ ] At least **one CI workflow** runs tests on **PR/push** with **path filters** appropriate to `client/` and the new test assets.
- [ ] New tests cover **`player.gd`**, **`position_authority_client.gd`**, and **`ground_pick.gd`** where feasible; any **intentionally untested** surface (e.g. full `_input` ray pick, `main.gd`) is **briefly noted** in the plan or README.

## Technical approach

1. **Pick and pin GDUnit4** (or GUT): download/addon version verified against **Godot 4.6**; commit under `client/addons/…`; enable the plugin in `client/project.godot`.
2. **Configure test discovery** per the addon (e.g. GDUnit4 test directory / naming); keep **no `class_name`** on production scripts that the README already calls out for headless—tests attach scripts by path or scene instance.
3. **`player.gd`:** Build a small **SceneTree**-hosted rig in setup: `CharacterBody3D` + `NavigationAgent3D` named to match `player.gd`’s `$NavigationAgent3D`; call `snap_to_server` / goals; advance **physics frames** (`await` or GDUnit helpers) and assert position, `velocity`, and `_has_walk_goal` behavior where observable via public effects (arrival epsilon, clear goal).
4. **`position_authority_client.gd`:** Replace or wrap **`HTTPRequest`** with a **double** that records `request()` args and simulates **`request_completed`**; drive `sync_from_server` / `submit_move_target` and assert signals and `_busy` / phase behavior.
5. **`ground_pick.gd`:** Instantiate script on `Node3D`; build `StaticBody3D` (or plain `Node`) in/out of **`walkable`** group; use `call("_collider_is_walkable", collider)` **only if** needed for white-box tests, or test via a thin extracted helper if the team prefers no underscore calls (optional refactor—keep minimal).
6. **CI:** Reuse the **official Linux 4.6-stable** Godot binary pattern from `client/README.md`; run the addon’s **headless test runner** CLI; fail the job on test failure. Optionally **merge** into `.github/workflows/gdscript.yml` as a second job or add `client-tests.yml`—whichever keeps caches and Godot download maintainable.
7. **Documentation:** Add a **“Automated tests (GDScript)”** section to `client/README.md` with exact commands.

## Files to add

| Path | Purpose |
|------|---------|
| `client/addons/gdUnit4/` (or chosen plugin root) | Vendored **GDUnit4** (or **GUT**) addon, **pinned** to a release compatible with **Godot 4.6**. |
| `client/test/player_test.gd` | GDUnit tests for **`player.gd`** (snap, nav goal, clear, arrival / idle paths where assertable). |
| `client/test/position_authority_client_test.gd` | Tests for **`position_authority_client.gd`** using **HTTPRequest double** + signal assertions. |
| `client/test/ground_pick_test.gd` | Tests for **`ground_pick.gd`** walkable collider logic (and optional ray/segment cases if implemented). |

## Files to modify

| Path | Rationale |
|------|-----------|
| `client/project.godot` | Enable **test plugin**; set **GDUnit4** (or GUT) project options / test root if required. |
| `client/README.md` | Document **how to run tests locally** and CI pointer; note **`main.gd` / full pick** deferred. |
| `.github/workflows/gdscript.yml` and/or **new** `client`-focused workflow under `.github/workflows/` | Add job(s) to **download Godot 4.6** and **run the suite** with **path filters** covering `client/` (including addons + tests). |

## Tests

| Test file | What it covers |
|-----------|----------------|
| `client/test/player_test.gd` | `snap_to_server` resets transform/velocity and nav target; `set_authoritative_nav_goal` / `clear_nav_goal`; movement toward goal / arrival within **ARIVE_EPS** / **VERT_ARRIVE_EPS** (using minimal tree + physics stepping). |
| `client/test/position_authority_client_test.gd` | Boot `GET` success → `authoritative_position_received(..., true)`; `POST` `400` + JSON → `move_rejected`; `POST` `200` + verify `GET` `200` → `authoritative_position_received(..., false)`; malformed JSON does not emit position. |
| `client/test/ground_pick_test.gd` | `_collider_is_walkable` true when ancestor in **`walkable`** group, false otherwise; document in-file if ray stepping remains manual-only. |

**Manual verification (supplemental):** Run **F5** main scene after plugin install to ensure editor still loads; run documented **headless** Godot command from README; confirm CI green on a PR touching `client/scripts/player.gd`.

## Open questions / risks

- **Addon choice lock-in:** If **GDUnit4** lags a **4.6** patch, switch to **GUT** for this story and document—low risk if decided early.
- **CI time:** Downloading **Godot** each run vs **cache** action—balance simplicity (download URL) vs speed; may iterate after first green pipeline.
- **White-box calls** to `_*` methods in tests vs **small extractions**—prefer smallest change; refactor only if reviews object to underscore `call()`.