Automated test suite: smoke, unit and integration tiers - #25
Merged
Sellafield merged 24 commits intoAug 15, 2026
Conversation
Fixes two Important review findings on tools/smoke-test.ps1: - OpenProcess and GetExitCodeProcess return values were unchecked; a failure would silently leave the exit-code variable at 0 and report a graceful shutdown that never happened. Both are now checked and route to a thrown error with a distinct message on failure. - No top-level trap existed, so an unhandled terminating error (missing dotnet, unreadable log, failed P/Invoke) would exit through PowerShell's own default code instead of a documented one. Added exit code 7 and a trap immediately after $ErrorActionPreference = 'Stop', per the patched plan.
Perpetuum.Tests references Perpetuum.csproj, which is marked
SupportedOSPlatform("windows"). Every other consumer of Perpetuum.csproj
declares the same attribute itself; Perpetuum.Tests did not, which is
why the analyzer fired CA1416 on every ValueTypeExtensions call.
Adds DbQueryTests, exercising ExecuteHelper's command type inference, parameter mapping, null handling, timeout propagation, and result reading against FakeDb. Fixes FakeDataReader.Current to no longer index Rows[-1]: DbEnumerator (behind DataReaderExtensions.ToEnumerable, which DbQuery.Execute() and ExecuteSingleRow() rely on) calls GetFieldType for every column once, before the first Read(), to build its schema info, while the reader is still unpositioned. All 6 tests that go through Execute() reproduced this before the fix; the 2 that go through ExecuteScalar()/ ExecuteNonQuery() passed unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 6 review found two of the four behaviours the task set out to pin
were missing: command type inference and ExecuteSingleRow's row
reduction. RecordedCommand did not even carry CommandType, so no test
could reach DbQuery.ExecuteHelper's `_commandText.Contains(' ')`
heuristic (DbQuery.cs:64) that decides Text vs StoredProcedure.
Adds CommandType to RecordedCommand and FakeDb.Record, and adds
The_command_type_is_inferred_from_whether_the_text_contains_a_space
(both branches of the heuristic) and
ExecuteSingleRow_returns_the_first_row_and_null_when_there_are_none.
Both passed on first run; the heuristic behaves as documented.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iour Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e procedure check Review fixes for Task 9: - Cross-reference comment above InsurancePricesQuery (InsuranceQueryTests.cs) and above the matching stub in DbQueryTests.cs, stating the three-copy literal is only kept in sync by hand. - The_insurance_price_recalculation_procedure_exists_and_is_callable renamed to The_insurance_price_recalculation_procedure_exists, with both reasons it is not redundant with Task 8 stated in comments (it anchors the unit tier's WhenNonQuery stub; Task 8's check is driven from docs/db_structure/ and would not catch the procedure and its doc file being deleted together). - The sys.objects lookup is now joined to sys.schemas and filtered on s.name = 'dbo', matching the schema-qualification already used by Task 8's StoredProcedureConformanceTests, with a comment explaining why a bare name match is not safe on this database.
…side the transaction
Reverting the fix fails every test in the class, not just the one named for the reload. The getter of Transaction.Current throws inside a completed-but-undisposed TransactionScope rather than returning a value, so FakeDbConnection.Open() throws and Refresh() aborts before any assertion runs. The named test is the diagnostic one; the other two fail as collateral. This is also a sharper statement of ISSUE-039 itself: reading the ambient transaction inside a completed scope is the error, which is why production failed at SqlConnection.Open() with the same message.
Six items from the whole-branch review, landed before the PR opens: 1. EnvironmentDiscoveryTests: Writes_are_disabled_unless_explicitly_allowed now drives PERPETUUM_TESTDB_ALLOW_WRITE itself (unset, "0", "1") instead of reading whatever the shell ambiently carries. The old version ran zero assertions whenever the variable was already set - exactly the case for a developer on a later write-enabled stage - while guarding the only opt-in that protects the operator's real database. 2. DatabaseFixture: renamed the Environment property to LocalEnvironment so it no longer shadows System.Environment; only internal uses existed. 3. StoredProcedureConformanceTests: closed the XML doc comment on ProcedureNameFromDocumentedFileName around all of its prose instead of leaving trailing unwrapped /// lines after </summary>. 4. RecordingLogger: restored the comment explaining why Exceptions checks both LogType.Error and ThrownException != null. 5. FakeDb: documented that _results/_nonQueries are intentionally not lock-protected (test-setup-only, unlike _commands) and that When() resolves by first-match-wins. 6. smoke-test.ps1: exit code 2 is also returned when the server binary is not found after a successful build; the docstring now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a `test` job to .github/workflows/dotnet.yml running
dotnet test src/Perpetuum.Tests/Perpetuum.Tests.csproj \
--no-restore --configuration Release -p:Platform=x64
The job is blocking, with no continue-on-error. Without it the test projects
would be merged and never run.
It does not reference Perpetuum.Tests.Integration, which needs a configured
GameRoot and a live database. Two independent barriers keep that tier out of
CI: the job never names the project, and [RequiresGameRoot] would skip its
tests even if something did.
`build`, `build-admintool-installer` and `publish-wiki` are untouched,
including publish-wiki's `needs: build`. Nothing that exists today changes
behaviour: `dotnet restore` already resolved the whole solution, and the
uploaded artifact still comes from bin/x64/Release/net8.0, which the test
projects do not write to.
Five documents stated that this repository has no automated tests. That was true when they were written and is no longer true, so a merge without this commit would leave the documentation set contradicting the code. docs/codebase/TESTING.md is rewritten. Current State, Test Infrastructure, CI Pipeline and Adding Tests are replaced; Manual Testing is kept, because no tier covers gameplay behaviour and running the server by hand is still the only way to validate it. Gaps is narrowed to what is still uncovered rather than deleted. The Analysis Date convention is kept with a new date. The document now records how to run each tier, the two environment variables, the four static service locators used as seams, and the seven documented exit codes of the smoke script. CLAUDE.md changes in two places: the Build & Run section gains the unit-tier command, and Testing & Validation stops instructing Claude to propose manual validation as the only option. It now asks for tests first and manual steps for what tests cannot reach, and adds two prohibitions: do not restructure production code to make a test possible without saying so, and do not add a regression test without observing it fail against the unfixed code. ARCHITECTURE.md, CONCERNS.md and STACK.md each carried a one-line claim that no tests exist; each is corrected to say coverage is partial and to point at TESTING.md. The ARCHITECTURE.md edit is a judgement call. CLAUDE.md requires that file to be updated for major architectural changes, and whether a test suite qualifies is arguable. The change made here is minimal — it corrects the existing Architectural Constraints bullet rather than adding a section. If the maintainers would rather see a full section, or nothing at all, say so and it will be changed. docs/backlog/improvements.md gains IMPROVEMENT-045, status IN_PROGRESS, with Last ID used raised from 044 to 045. It records the three tiers, what stages 0-4 delivered, and the six stages that are not started. Two categories of stale claim are deliberately left alone: docs/backlog/ completed.md and docs/superpowers/plans/. Both are dated records of work already done, correct at the time of writing. Editing them would rewrite history rather than document the present.
The figure was 761, which counted the whole solution including Perpetuum.AdminTool. Measured across Perpetuum and Perpetuum.RequestHandlers, the projects the fake actually intercepts, it is 755. The scope is now stated alongside the number.
Sellafield
self-requested a review
August 15, 2026 04:12
Sellafield
approved these changes
Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an automated test suite to this repository, in three tiers, plus the CI job that runs it and the
documentation that records it.
This is an architectural addition to your repository that nobody asked for, so the short version
first: it changes no production code, it is additive in every file it touches except the solution
file and four documents, and if you do not want it, nothing here is load-bearing for anything else.
IMPROVEMENT-045is included indocs/backlog/improvements.mdso the entry exists whichever way thisgoes.
The argument
Two bugs found in this repository over the last week are now regression tests:
Issue033EmptyFlockTestsFreeRoamingPathFinderthrowing on a presence with no flocks (ISSUE-033, fixed in #19)Issue039InsuranceTransactionTestsLoadInsurancePrices()running inside an already-completedTransactionScope(ISSUE-039, fixed in #23)Both were observed failing with their fixes reverted before being accepted into the branch. That is
the only claim in this pull request that proves the suite detects anything; "58 tests pass" on its own
is a statement about nothing.
ISSUE-039 is the one worth dwelling on. It produced no error in the log, no crash, and no visible
symptom — the server simply kept quoting insurance prices from a cache that was never reloaded. It was
found by accident, while chasing a different bug. That is the class of defect an automated suite exists
to catch.
There is a second reason, and it is the stronger one. The share of AI-authored code in this repository
is going up — including in these pull requests. The value of a test suite is that it answers "did this
break something" without a human re-deriving the answer for every contribution, from any author.
ISSUE-038 already asks in writing for "an automated test client" looping session connect/disconnect
while sampling
dotnet-gcdump. That is a soak harness rather than a test suite, but the demand forautomation is already on your record.
What is here
tools/smoke-test.ps1GameRootand a live databasesrc/Perpetuum.Testssrc/Perpetuum.Tests.IntegrationGameRootand a live databaseThree tiers rather than one because no single tier catches what this codebase actually breaks.
ISSUE-039 only manifests when a real
SqlConnection.Open()readsTransaction.Currentand finds acompleted scope — a faked connection passes it happily. ISSUE-036 was a SQL Server dependency
resolution defect. Both needed a real database or a real server run to surface.
Tier 1 builds the solution, starts the real server, waits for
State : [Online], waits for the logto quiesce, sends Ctrl+C through
GenerateConsoleCtrlEvent, and asserts the process reachesState : [Off]and exits 0. Force-killing a server that will not stop is reported as a failure, not apass. Seven documented exit codes.
Assertions are in three categories: required (absence fails), forbidden (presence fails), and
reported — printed, never asserted. The spawned member count across recorded runs was 6406, 6423
and 6425; it changes with every content patch, so asserting on it would build a test that fails when
the game works.
Tier 2 uses the four static service locators you already have as its seams. Each is a settable
public staticassigned in exactly one place inPerpetuumBootstrapper, so a fixture assigns a doublebefore exercising code.
Db.DbQueryFactoryis the important one:Db.Query()funnels 755 call sitesin
PerpetuumandPerpetuum.RequestHandlersthrough it, andDbQueryalready takes aDbConnectionFactorydelegate, so the whole data layer is interceptable without touching productioncode.
Fakes/Data/implements the ADO.NET interfaces as a recording fake: register a result setagainst a command pattern, then assert on the SQL and parameters the code under test actually produced.
docs/codebase/TESTING.mdcalled those locators "the main obstacle". They turned out to be thesolution.
Tier 3 runs against the real
perpetuumsa. No synthetic schema, no scratch database, no restore.Every developer who touches this code already has the standard environment, and a second copy of the
DDL would drift from production. It checks that all 231 documented stored procedures and 111 documented
functions exist in the live database with the documented signature, and it executes the same queries
tier 2 stubs — which is what keeps the fake an assertion about how the database behaves rather than
about how it was imagined to behave.
What this does not do
git diff develop -- src/Perpetuum src/Perpetuum.RequestHandlersis empty. YourCLAUDE.mdforbidsspeculative refactors, so nothing was restructured to make a test possible.
Perpetuum.RequestHandlersisexplicitly a non-goal. Still untested: the entity system, module state machines, the season service,
request handlers, the mission engine, and concurrency. Those are stages 5-10 in
IMPROVEMENT-045andare not in this pull request.
two test projects.
build,build-admintool-installerandpublish-wikiare untouched, includingpublish-wiki'sneeds: build.dotnet restorealreadyresolved the whole solution. The uploaded artifact still comes from
bin/x64/Release/net8.0, whichthe test projects do not write to — they have no
BaseOutputPathoverride.CI
.github/workflows/dotnet.ymlgains onetestjob running the unit tier, blocking, nocontinue-on-error. Without it the test projects would be merged and never run.It does not reference the integration project. Two independent barriers keep that tier out of CI: the
job never names it, and
[RequiresGameRoot]would skip its tests even if something did.Documentation
Five documents stated that this repository has no automated tests. True when written, no longer true,
so a merge without these edits would leave the documentation set contradicting the code.
docs/codebase/TESTING.mdCLAUDE.mddocs/codebase/ARCHITECTURE.mddocs/codebase/CONCERNS.mddocs/codebase/STACK.mddocs/backlog/completed.mdanddocs/superpowers/plans/also contain the claim, and are deliberatelyleft alone — they are dated records of work already done, correct at the time of writing. Editing them
would rewrite history rather than document the present.
Questions
ARCHITECTURE.mdis a judgement call. YourCLAUDE.mdrequires that file to be updated formajor architectural changes, and whether a test suite qualifies is arguable. What is here is
minimal — one corrected bullet in Architectural Constraints, not a new section. If you would rather
have a full section, or nothing at all, say so and it changes.
IMPROVEMENT-045isIN_PROGRESSand priorityMEDIUM. Set it to whatever reflects how youactually want to treat this.
pull request is no. If the answer is yes, the order in
IMPROVEMENT-045is a proposal, not acommitment — if the subsystems you would most want covered are in a different order, that is useful
to know now.
developer here already has it. If that assumption is wrong for your contributors, tier 3 is the part
to reconsider.
Verifying locally
That needs no setup and is what CI runs. For the other two tiers, with
PERPETUUM_GAMEROOTpointing atthe directory holding
perpetuum.ini:Tier 3 without
PERPETUUM_GAMEROOTskips rather than fails, so it stays quiet on a machine that is notset up for it.
The branch is 23 commits and reads as a sequence; reviewing commit by commit will be easier than
reviewing the diff in one piece.