diff --git a/AGENTS.md b/AGENTS.md index e7b1f86a..ead52bb9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,7 +112,16 @@ dotnet build BlocksBeyondTheStars.sln # build everything (Linux) `run-tests.ps1` defaults to the fast .NET suites (`Dotnet` + `ClientCore`); the Unity Editor suites (`UnityEdit` EditMode, `UnityPlay` PlayMode-vs-real-server-exe) are opt-in via `-Suites`. How the client is tested against the real server is documented in -[docs/developer/CLIENT_TESTING.md](docs/developer/CLIENT_TESTING.md). +[docs/developer/CLIENT_TESTING.md](docs/developer/CLIENT_TESTING.md); how to write tests for the +server/shared suite (fixtures, exemplars, conventions) is in +[docs/developer/SERVER_TESTING.md](docs/developer/SERVER_TESTING.md). + +**Internal spec citations (`anf_*.md`):** some doc comments cite files like +`anf_admin_einstellungen.md` §… as "technical requirements". These are the project's internal +German design specs from before open-sourcing and are **not in the public repository**. The +English summary in the doc comment is the authoritative public statement of the behaviour; a +comment that leans on such a citation without summarising the rule is a documentation bug — +fix or report it rather than hunting for the file. To confirm a client rebuild actually happened, check the `BlocksBeyondTheStars.Client.dll` timestamp in the build output (the `.exe` timestamp is not reliable). The full build guide — pipeline details, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 09cba8c0..2e534d70 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,7 +47,9 @@ If you are a developer, we welcome pull requests. dotnet test # run all xUnit tests (keep them green) ``` The playable Windows client is built with `scripts/build-client.ps1` (requires the Unity - Editor). See [docs/developer/DEVELOPER.md](docs/developer/DEVELOPER.md). + Editor). See [docs/developer/DEVELOPER.md](docs/developer/DEVELOPER.md). If you want to + *write* tests (a great first contribution), start with + [docs/developer/SERVER_TESTING.md](docs/developer/SERVER_TESTING.md). 3. **Open a pull request** against `main` with a short description of the change and why. Small, focused PRs are easier to review and merge. diff --git a/TODO.md b/TODO.md index 1634ea9e..d75b14de 100644 --- a/TODO.md +++ b/TODO.md @@ -7099,6 +7099,25 @@ is **pre-approved** (keys in `tools/ai-assets/.env`, run via `uv`). --- +## ✅ Done (2026-08-04): server test-writing guide + de-ghosted spec citations (#571 follow-up) + +A contributor stepped down from #571 ("not enough business-logic context to write meaningful +tests"); root-cause analysis found the gaps and this closes them: + +- **`docs/developer/SERVER_TESTING.md`** (new) — writing tests for the server/shared suite: + three exemplar tests to copy from (pure → content-loading → full server), the invisible + fixtures (`TestPaths.DataDir()` + `ContentLoader`, `TestLocales`), an "invariants, not + mirrored constants" table per target kind (the tautology trap), determinism rules (incl. the + Win/Linux libm trig gotcha), and the CI analyzer traps. Linked from CONTRIBUTING.md, + AGENTS.md and the docs index. +- **Internal spec citations de-ghosted** — 26 source files cite `anf_*.md` German design specs + that are gitignored (`/media/`), so fork contributors chased references into the void. + AGENTS.md now states the convention (doc-comment summary = public authority; a citation + without a summary is a doc bug); the three #571 target files leaning hardest on citations got + self-contained behaviour summaries (`MissionValidator` rule list, `ServerPresets` lookup + contract, `FrequencyExtensions` consumers + invariants incl. the `OreFactor` Off≠0 exception). +- Comment/docs-only change — no behaviour difference; issue #571 body refreshed to match. + ## ✅ Done (2026-08-04): NPCs grounded, brighter and individual (#711) Three related NPC problems fixed in one branch (issue #711): diff --git a/docs/developer/README.md b/docs/developer/README.md index ed297d04..9c6516cc 100644 --- a/docs/developer/README.md +++ b/docs/developer/README.md @@ -20,6 +20,9 @@ belongs in TODO.md). Each doc states its own status near the top. Last reorganis the CI job pair, the artifact execute-bit fix, and running an unsigned/un-notarized `.app`. - [CLIENT_TESTING.md](CLIENT_TESTING.md) — how the Unity client is tested against the **real** game server (the `Client.Core` split, the three test tiers, the selectable `run-tests.ps1` runner). +- [SERVER_TESTING.md](SERVER_TESTING.md) — **writing tests** for the server/shared .NET suite: exemplar + tests to copy from, the content/locale fixtures, what "meaningful" assertions look like, and the CI + analyzer traps. Start here for a first test contribution (e.g. issue #571). - [SELF_HOSTING.md](SELF_HOSTING.md) — run and host a dedicated server, config keys, the web portal & updates. ## World & worldgen diff --git a/docs/developer/SERVER_TESTING.md b/docs/developer/SERVER_TESTING.md new file mode 100644 index 00000000..f197fd7d --- /dev/null +++ b/docs/developer/SERVER_TESTING.md @@ -0,0 +1,102 @@ +# Server testing — writing your first test for the .NET suite + +> **Status: contributor guide.** Written 2026-08-04 after contributor feedback on issue #571 +> ("I don't have enough context to write meaningful tests"). [CLIENT_TESTING.md](CLIENT_TESTING.md) +> covers how the Unity client is tested; **this doc covers the server/shared suite** +> (`tests/BlocksBeyondTheStars.Tests`) — where things are, which existing tests to copy from, +> and what "meaningful" means for the kinds of code you'll be testing. + +## The one-minute version + +```powershell +dotnet test -c Release --filter Category!=Slow # fast daily loop (~3 min) +dotnet test # everything, incl. ~31 Slow soak tests +./scripts/test-coverage.ps1 -Open # full coverage report (slow, ~30 min) +``` + +Add a file to `tests/BlocksBeyondTheStars.Tests/`, name it `Tests.cs`, use plain xUnit +`[Fact]`/`[Theory]`. No registration needed anywhere — CI shards test classes across runners +automatically ([`scripts/partition-tests.py`](../../scripts/partition-tests.py)), so a new class +just gets picked up. + +## Three exemplars to copy from + +Most of the 220+ test files are integration-style tests around a full `GameServer` — **don't +start there**. These three show the ladder, simplest first: + +1. **Pure logic, no fixtures** — [`NameGeneratorTests.cs`](../../tests/BlocksBeyondTheStars.Tests/NameGeneratorTests.cs). + Asserts determinism (same seed → same output) and *shape* properties ("two capitalised + words") over a seed range. This is the pattern for `Vector3i`, `ChunkCoord`, noise, + `Localizer`, `FrequencyExtensions`, `ServerPresets`, `MissionValidator`. +2. **Needs game content** — [`BlockShapeTests.cs`](../../tests/BlocksBeyondTheStars.Tests/BlockShapeTests.cs). + Loads the real data-driven content once in the constructor and uses a Guid-named temp dir + with `IDisposable` cleanup. +3. **Full server** — [`GameServerIntegrationTests.cs`](../../tests/BlocksBeyondTheStars.Tests/GameServerIntegrationTests.cs). + Constructs a real `GameServer` with a loopback transport and a SQLite repo. Only needed + when the behaviour under test *is* the server loop (ticks, intents, persistence). + +## Fixtures & helpers you would otherwise not find + +- **`TestPaths.DataDir()`** ([TestPaths.cs](../../tests/BlocksBeyondTheStars.Tests/TestPaths.cs)) + walks up from the test output directory to the repo's `data/` folder — so tests run against + the *real* shipped content from any build output location. The standard opener is: + + ```csharp + private static GameContent Load() => ContentLoader.LoadFromDirectory(TestPaths.DataDir()); + ``` + + `GameContent` is the loaded form of `data/*.json` (blocks, items, recipes, planets …); most + validators and generators take it as a parameter, and loading the real thing is both easier + and more honest than mocking it. +- **`TestLocales.Load("en" | "de")`** ([TestLocales.cs](../../tests/BlocksBeyondTheStars.Tests/TestLocales.cs)) + reads the real locale tables — use it to assert a feature's keys exist in **both** languages + (a missing key renders as literal `[some.key]` in game instead of failing loudly). +- **Temp state**: `Path.Combine(Path.GetTempPath(), "bbts__" + Guid.NewGuid().ToString("N"))` + plus `IDisposable` cleanup — never share paths between tests; the suite runs 4-way parallel. + +## What "meaningful" means here (the tautology trap) + +A lot of the untested surface is small pure functions whose *constants are the implementation* — +e.g. `FrequencyExtensions.Probability(Rare) == 0.15`. A test that just restates the switch arm +mirrors the code and verifies nothing. **Assert the invariants instead** — the properties that +must survive any future retuning of the numbers: + +| Target kind | Meaningful assertions | +|---|---| +| Factor tables (`FrequencyExtensions`) | `Off` is exactly `0` where "off must mean off"; values are **monotone** in enum order; the enum's default level maps to `1.0` for factors documented as "existing worlds unchanged" | +| Primitive types (`Vector3i`, `ChunkCoord`) | equality ↔ hash-code consistency, roundtrips (pack/unpack, parse/format), arithmetic identities | +| Lookup tables (`ServerPresets`, locale keys) | case-insensitivity, unknown name → `null` (not throw), every advertised `Names` entry resolves | +| Validators (`MissionValidator`) | each documented error case produces a problem; a known-good definition produces **zero** problems | +| `Localizer` | active-locale hit, English fallback, unknown key → `[key]` wrap, empty key → empty string | +| Noise / RNG / generators | same seed → same output; output range; seam continuity at the world wrap (see [WORLD_WRAP.md](WORLD_WRAP.md)); **never compare trig-derived floats or golden-hash them** — `sin`/`cos` differ between Windows and Linux libm and the hash will pass locally and fail in CI. Hash integers, or assert structural properties | + +If you can't tell which invariants are intended: the XML doc comment on the type is the public +authority on behaviour (see the note on internal specs below), and asking on the issue is +always fine — pointing at an undocumented spot is itself a useful contribution. + +## Where behaviour is specified + +- The **XML doc comments** on the types themselves — these are kept as the public source of + truth for intended behaviour. +- The [docs/developer/ index](README.md) has a deep-dive per area (worldgen → + [WORLD_GENERATION.md](WORLD_GENERATION.md), topology → [WORLD_WRAP.md](WORLD_WRAP.md), …). +- Some comments cite `anf_*.md` files ("technical requirements"). Those are the project's + **internal German design specs from before open-sourcing; they are not in the public repo.** + Treat the English summary in the doc comment as the authoritative statement of intent — and + if a comment leans on such a citation without summarising the rule, that's a doc bug worth + reporting (or fixing in your PR). + +## CI rules that will bite you + +CI treats **warnings as errors** (Roslyn + Meziantou + VS.Threading analyzers). The recurring +first-PR traps: + +- Async test methods need an `Async` suffix (**VSTHRD200**). +- Don't `await` a `TaskCompletionSource.Task` in tests (**VSTHRD003**) — use a `SemaphoreSlim`. +- Any test not marked `[Trait("Category", "Slow")]` must finish **well under 120 s** — the PR + gate ([`scripts/check-test-durations.py`](../../scripts/check-test-durations.py)) fails on it. + Use the `Slow` trait for soak/heavy-worldgen tests; they still run on every push to `main`. +- Don't rely on `dotnet test -v minimal` output to check for warnings — it hides them. Do a + clean `dotnet build -warnaserror` (see AGENTS.md "Local verification after changes"). +- The suite pins `maxParallelThreads: 4` ([xunit.runner.json](../../tests/BlocksBeyondTheStars.Tests/xunit.runner.json)) + — heavy worldgen tests funnel through shared caches; don't "fix" a slow local run by raising it. diff --git a/src/BlocksBeyondTheStars.Shared/Configuration/ServerPresets.cs b/src/BlocksBeyondTheStars.Shared/Configuration/ServerPresets.cs index 3c1cb6c7..a8beb2f3 100644 --- a/src/BlocksBeyondTheStars.Shared/Configuration/ServerPresets.cs +++ b/src/BlocksBeyondTheStars.Shared/Configuration/ServerPresets.cs @@ -4,8 +4,11 @@ namespace BlocksBeyondTheStars.Shared.Configuration; /// -/// Predefined rule profiles (technical requirements / `anf_admin_einstellungen.md` §4 and -/// `anf_space_flight.md` §13.2) so admins don't have to set every rule individually. +/// Predefined rule profiles so admins don't have to set every rule individually. Contract: +/// lists every available preset; resolves trimmed and +/// case-insensitively and returns null for unknown (or null) names — callers fall back to +/// default rules. (Originally the internal design specs `anf_admin_einstellungen.md` §4 / +/// `anf_space_flight.md` §13.2 — this summary is the public authority.) /// public static class ServerPresets { diff --git a/src/BlocksBeyondTheStars.Shared/Missions/MissionValidator.cs b/src/BlocksBeyondTheStars.Shared/Missions/MissionValidator.cs index 84dc52f4..750f7141 100644 --- a/src/BlocksBeyondTheStars.Shared/Missions/MissionValidator.cs +++ b/src/BlocksBeyondTheStars.Shared/Missions/MissionValidator.cs @@ -6,9 +6,13 @@ namespace BlocksBeyondTheStars.Shared.Missions; /// -/// Validates a mission definition against the loaded content (technical requirements / -/// `anf_admin_blueprinf.md` §10). Shared by the in-game player editor and the admin -/// extension editor so the same rules apply everywhere. +/// Validates a mission definition against the loaded content. Shared by the in-game player +/// editor and the admin extension editor so the same rules apply everywhere. The rules: a +/// mission needs a non-empty id and at least one objective; every objective needs a supported +/// type (Collect/Mine/Deliver), a positive required count and a target that exists in the +/// loaded content (a block for Mine, an item otherwise); every reward must reference a known +/// item with a positive count. (Originally the internal design spec `anf_admin_blueprinf.md` +/// §10 — this summary is the public authority.) /// public static class MissionValidator { diff --git a/src/BlocksBeyondTheStars.Shared/World/WorldDescription.cs b/src/BlocksBeyondTheStars.Shared/World/WorldDescription.cs index 04b5c563..75a4aa8d 100644 --- a/src/BlocksBeyondTheStars.Shared/World/WorldDescription.cs +++ b/src/BlocksBeyondTheStars.Shared/World/WorldDescription.cs @@ -13,9 +13,18 @@ public enum Frequency Frequent, } +/// +/// Maps a level to the numeric knobs the generators consume. The exact +/// constants are tuning values and may be retuned; the intended invariants are: every mapping is +/// monotone non-decreasing in enum order, is exactly 0 wherever +/// "off must mean off" ( is the documented exception), and factor-style +/// mappings return 1.0 at the owning property's default level so existing worlds are unchanged. +/// public static class FrequencyExtensions { - /// Selection weight; 0 means the feature never spawns. + /// Relative weight for weighted selection — consumed by the universe generator's + /// planet-type roll (); 0 means the + /// feature never spawns. public static int Weight(this Frequency f) => f switch { Frequency.Off => 0, @@ -26,7 +35,9 @@ public static class FrequencyExtensions _ => 0, }; - /// Probability in [0,1] used for per-body chance rolls. + /// Probability in [0,1] for independent chance rolls — e.g. the universe + /// generator's space-station and wreck gates (, + /// ) and the settlement/station template-use rolls. public static double Probability(this Frequency f) => f switch { Frequency.Off => 0.0, @@ -37,7 +48,9 @@ public static class FrequencyExtensions _ => 0.0, }; - /// Flora/tree density factor (world options; Normal = unchanged, Off = barren). + /// Flora/tree density multiplier applied to per-planet flora generation + /// (world options / ; Normal = 1.0 = unchanged, + /// Off = barren). public static double FloraFactor(this Frequency f) => f switch { Frequency.Off => 0.0,