Skip to content

Add characterization tests for AiService turn logic - #76

Merged
dmccoystephenson merged 1 commit into
mainfrom
feature/ai-service-tests
Aug 2, 2026
Merged

Add characterization tests for AiService turn logic#76
dmccoystephenson merged 1 commit into
mainfrom
feature/ai-service-tests

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Stage B cycle (unit-test expansion). AiService is the whole AI opponent — threat evaluation, target selection, and garrison management for player 2 — and it had no test file at all, while every other service in backend/src/main/java/com/barony/backend/service/ does. This adds 17 characterization tests that lock in its current behavior.

Coverage added, by decision path:

  • Turn selection — only idle, AI-owned armies are given orders (already-moving armies keep their existing order; human armies are never touched).
  • Priority ladder — a threatened own-village (defend) outranks an adjacent neutral village (capture); the neutral-village target outranks a weak enemy village; the castle assault is the last-resort target.
  • Build-up rule — an AI army under the 5-soldier minimum holds its own village; at exactly 5 it moves out.
  • Safety check — a neutral village is skipped when an equal-or-stronger enemy is within the danger range, and taken when that enemy is weaker.
  • Force ratios — the 1.5x village attack ratio and the 2.0x castle assault ratio, each tested on both sides of the boundary.
  • Garrisoning — a departing army splits off a single soldier to hold its village; no garrison is split when another AI army already holds the tile, or once the AI is at its 5-army cap.

No production code was changed. Per the dev-loop's Stage B rule these are characterization tests only — they assert what the code does today, so they act as regression guards for future AI tuning.

Module(s) touched

backend only. No REST endpoint, request/response shape, or player-visible behavior changed, so no web-client, README, PLAYER_GUIDE, or CHANGELOG update applies.

Test plan

  • New file backend/src/test/java/com/barony/backend/service/AiServiceTest.java follows the sibling convention (JUnit 5, <ClassName>Test.java, mirrored package, plain instantiation like CombatServiceTest).
  • CI is the verification anchor for this PR. Local ./mvnw is unavailable in the environment this cycle ran in, so the local build is UNVERIFIED; the "CI Build and Test" workflow on this PR head is the real anchor. Both the Backend Build and Test and Web Client Build and Test jobs must pass, and the backend surefire output must show AiServiceTest actually ran.

Issues

No tracking issue — this is a Stage B (unit-test expansion) cycle, not an issue fix.

Deferred this cycle: #55 (configurable, per-account dashboard layout). It is a multi-part feature spanning a new server-side per-user preferences store, authenticated backend endpoints, the web-client caller, and panel reorder/show-hide/collapse UI in game.html / game-logic.js — comfortably past this loop's ~400 net non-test LOC / ~10 file scope ceiling for a single PR. It needs splitting into slices (e.g. client-side collapse/reorder persistence first, then server-side sync keyed to the authenticated user) before it is an implementable unit of work. PR #42 is a Copilot draft and was left untouched.


This PR description was drafted during a Gardener session (Stephenson-Software/gardener).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dmccoystephenson

dmccoystephenson commented Aug 2, 2026

Copy link
Copy Markdown
Member Author

Self-review rubric (Phase 4). CI on this head is the external anchor: run 30739697877 — Backend Build and Test pass (23s), Web Client Build and Test pass (27s).

  • Scope: PASSgit diff --name-only origin/main...HEAD returns exactly one path, backend/src/test/java/com/barony/backend/service/AiServiceTest.java, +277/-0. No production code, no formatting churn, no unrelated files.
  • Tests-new: PASS — this PR adds no public methods; it exists to cover AiService.executeAiTurn, which had no test file. Confirmed executed rather than merely compiled: the backend surefire log shows Running com.barony.backend.service.AiServiceTest / Tests run: 17, Failures: 0, Errors: 0, Skipped: 0, and the suite total rose to 256.
  • Tests-fix: N/A — no bug fix in this PR, so there is nothing to stash-and-revert. Per the dev-loop's Stage B rule these are characterization tests asserting current behavior; no production behavior was altered to make any of them pass.
  • Sibling structure: PASS — mirrors CombatServiceTest in the same package: JUnit 5, <ClassName>Test.java, plain new AiService() (matching how GameService itself instantiates it at GameService.java:19), private newGameState() / setTile() / addArmy() helpers, PLAYER_1/PLAYER_2 constants, static-import assertions.
  • Sibling renames: N/A — nothing renamed.
  • Docs: PASS — walked the Phase 7 table against the diff. README (Quick Start, ports, Backend API endpoints + curl examples), PLAYER_GUIDE (controls/rules), CHANGELOG (player-visible changes), MVP, DOCS: none of them describe test sources, and no endpoint, response shape, or gameplay rule changed. Nothing to update.
  • Issue resolution: N/A — no Closes #N. This is a Stage B cycle, not an issue fix. Make the dashboard layout configurable and persist it per account #55, the only open non-Copilot issue, is deferred with its reason recorded in the PR body.
  • CI: PASS — both required jobs green on this head, re-confirmed above.
  • Both-modules: PASS — checked per job, not in aggregate: backend 256 tests pass, web-client job passes independently. The web-client was untouched by this diff, so its green run is a no-regression signal rather than verification of new code.
  • API-contract: N/A — no backend REST endpoint (path, method, request body, or response shape) changed, so there is no web-client caller or README API section to keep in sync.
  • Changelog: N/A — test-only change, nothing player-visible.
  • Constructor-injection: N/A — no new Spring component; AiService is a plain package-private class the test instantiates directly.
  • Override-correct: N/A — no @Override in the diff.

Local anchor caveat, stated plainly: ./mvnw could not be run in the environment this cycle executed in, so there was no local test run — every expectation in this file was hand-derived from the source and then validated by CI on this exact head. That is why the surefire per-class line was checked explicitly rather than trusting the green check mark.

Adversarial finding (out-of-diff observation, folded in here rather than as an inline comment). Writing these tests surfaced two guards in AiService that are unreachable given the current call order, which is why no test covers them:

  • AiService.java:206 — the army.getSoldiers() <= 1 guard in shouldLeaveGarrison can never be the deciding condition. shouldLeaveGarrison is only reached after assignAiTarget, and an army with ≤1 soldiers standing on an AI-owned village always short-circuits earlier in isGarrisoningAndBuilding (1 < MIN_SOLDIERS_TO_MOVE), so it never gets a destination and fails the preceding !army.isMoving() check instead. An army that already had a destination isn't idle and is never passed in at all.
  • AiService.java:226 — the pendingGarrisons loop in hasExistingGarrison can never return true. A pending garrison only collides at coordinates where two idle AI armies share one AI village, and in that case the earlier army is always blocked by the first loop (the later army is still stationary, having not yet been assigned a target). Traced with two and three co-located armies: exactly one garrison is produced, always via the stationary-army loop.

Both are harmless defensive code, not bugs, so per the Stage B "characterization, not change" rule I did not touch production code to remove them — flagging for a future cleanup cycle's judgment rather than filing backlog noise.

Coverage limit worth naming: every test uses a square 10×10 grid, so GameState.getHeight() returning grid[0].length is never exercised against a non-square map. Also, aiArmyAtTheMinimumStrengthLeavesItsOwnVillage asserts only the destination and deliberately tolerates the garrison-split side effect (the army drops to 4 soldiers and the roster grows to 2) so the boundary assertion stays focused; the split itself is pinned separately by departingAiArmyLeavesASingleSoldierBehindToHoldItsVillage.


This comment was drafted during a Gardener session (Stephenson-Software/gardener).

@dmccoystephenson
dmccoystephenson merged commit 50494b5 into main Aug 2, 2026
2 checks passed
@dmccoystephenson
dmccoystephenson deleted the feature/ai-service-tests branch August 2, 2026 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant