Refactor Imperium.UnitTests spec framework for clarity and consistency#139
Merged
Conversation
Split the flat test project into a source-mirror layout:
- Support/Spec → Imperium.Testing.Spec namespace (Specification, Runner,
Filter, CollectionAssert, Markdown) ready for future package extraction
- Support/Spec.Tests → split SpecTests.fs into per-area files
- Imperium/{Accounting,Rondel} → Context/Assertions/Specs (+ Rondel
StateFormatting); BC specs use module abbreviations in Main.fs to keep
Accounting/Rondel short names
- Imperium/Contract → contract transformation tests
- Imperium/Gameplay → placeholder test module
- Imperium.Terminal → mirrors src/Imperium.Terminal subfolders
Compile order in the fsproj is explicit and topological. All 130 tests
pass; dotnet test, native runner, and --render-spec-markdown filters
verified.
Update the test project layout section to reflect the new source-mirror structure introduced by #137 (Support/Spec, Imperium/{BC}/{Context, Assertions,Specs}, Imperium.Terminal mirrors). Document the Imperium.Testing.Spec namespace and the opens pattern consumer files use. Refresh all test-coverage snapshot paths to the new file locations. Drop CLAUDE.md so AGENTS.md remains the single agent guide.
Mark runActions and prepareContext as private in Runner.fs; they are implementation details, not part of the spec framework's public surface (SpecRunner, runExpectation, toExpecto). Rewrite the on/specOn factory tests in SpecificationTests.fs to assert on the context inside an expect block and drive via runExpectation, removing their dependency on prepareContext.
Replace the single-field record `type T = { MatchExpectation: ... }`
with `type Predicate = string list -> bool`. The wrapping record only
served to label one function; the alias keeps the documented signature
and lets call sites apply the predicate directly (`filter path` instead
of `filter.MatchExpectation path`).
Also renames `T` to `Predicate` to match the codebase's descriptive
type-naming convention (no other module uses `type T`).
Convert Runner.fs from a wrapper module (module Imperium.Testing.Spec.Runner containing type SpecRunner + nested module SpecRunner) to the canonical F# shape: namespace Imperium.Testing.Spec with the SpecRunner type and a companion SpecRunner module side-by-side. The private helpers runActions and prepareContext move inside the SpecRunner module so they live next to the public functions that use them. Consumers drop the now-redundant `open Imperium.Testing.Spec.Runner` line. Also renames toExpecto to toExpectoTestList to better convey its return type, and removes an unused Specification open in Rondel/Context.fs.
Convert Specification.fs from a wrapper module (module Imperium.Testing.Spec.Specification containing types + a same-named nested Specification module) to the canonical F# shape: namespace Imperium.Testing.Spec with types and a companion Specification module side-by-side. The companion module now holds only the spec/specOn CE factories. The previously-nested withGivenState/withActions/preserve helpers were unused across the codebase and have been removed; they can be reintroduced if a real consumer needs to compose specs outside the CE syntax. This mirrors the SpecRunner refactor: same shape applied to the framework's other primary type, eliminating the Specification.Specification.X redundancy.
SpecificationTests.fs previously held tests for three different modules
under a generic 'Spec' testList. Split into focused files that mirror the
framework module layout:
- SpecificationTests.fs ('Specification') — 5 tests for the CE factories
(spec/specOn) and Expecto-assertion compatibility.
- SpecRunnerTests.fs ('SpecRunner') — 5 tests for runExpectation outcomes
(assertion/action failures, state snapshots, AssertException) and the
preserve flag.
- CollectionAssertTests.fs ('CollectionAssert') — 2 tests for HasAny/HasNone
failure message contents.
Also fixes 4 testCase descriptions that were inadvertently prefixed with
'SpecRunner.' during an earlier mass rename, and capitalizes the two
collection assertion descriptions.
After the split, each Spec.Tests file maps 1:1 to a Spec framework module
(SpecificationTests ↔ Specification, SpecRunnerTests ↔ SpecRunner, etc.).
The module SpecMarkdown lives in the Imperium.Testing.Spec namespace, so the Spec prefix repeats the namespace. Rename to Markdown to match the codebase convention (Rondel, Accounting, Gameplay, Primitives — namespaces convey scope, modules don't repeat it). Also renames MarkdownRenderOptions to RenderOptions: the Markdown prefix existed only to disambiguate from the module's previous SpecMarkdown name, and now produces the redundant Markdown.RenderOptions read. testList in MarkdownTests.fs updated to match the new module name. The function renderSpecMarkdown is left intact — it's a domain action name tied to the --render-spec-markdown CLI flag, not a module reference.
For Accounting and Rondel test contexts, switch from a file-module wrapper
to the canonical F# namespace + type + companion module shape (matching
SpecRunner and Specification):
- File module Imperium.UnitTests.{BC}.Context becomes namespace
Imperium.UnitTests.{BC} with type Context + module Context containing
the create factory.
- AccountingContext / RondelContext drop the redundant BC prefix and
become just Context (namespace conveys the BC).
- createContext drops the redundant verb and becomes Context.create.
The runner SpecRunner value, previously top-level in each Context.fs, now
lives in the only file that uses it (Specs.fs) as a private value. The
Context type carries no responsibility for test execution wiring.
Also drops three now-redundant open statements (Assertions and Specs files
implicitly see their parent namespace via their module declaration path).
Two redundant-prefix renames in the BC test files: - accountingSpecs / rondelSpecs → specifications. The BC prefix repeats the file's namespace and the plural reads honestly as a Specification list. - renderSpecMarkdown → renderMarkdown. The function name no longer needs the Spec prefix now that the SpecMarkdown module is just Markdown; the namespace and the spec arguments convey "render specs as markdown." The CLI flag --render-spec-markdown stays as-is (used by CI and docs). docs/architecture.md updated to mirror the function name and pick up an earlier-missed SpecMarkdown.render → Markdown.render reference.
Updates the test-framework sections of AGENTS.md to match the recent
restructure on this branch:
- Spec framework file listing: Runner.fs → SpecRunner.fs; reflects the
type + companion module shape now used by SpecRunner and Specification;
renames SpecMarkdown → Markdown.
- Spec.Tests file listing now lists all five files (SpecRunnerTests and
CollectionAssertTests added after the split).
- BC test layout: Context.fs now declares type Context + Context.create
in namespace Imperium.UnitTests.{BC}; runner moved from Context.fs to
Specs.fs as a private value.
- Consumer-open guidance: opening Imperium.Testing.Spec now suffices for
the bulk of the framework; Imperium.Testing.Spec.Specification only
needed to unqualify the spec/specOn CE factories. The defunct
Imperium.Testing.Spec.Runner open removed.
- Usage-pattern code example updated: specOn createContext →
specOn Context.create, toExpecto runner → SpecRunner.toExpectoTestList
runner, accountingSpecs → specifications.
- Key design decision references promoted to the qualified
SpecRunner.runExpectation / SpecRunner.toExpectoTestList names.
- Test coverage snapshot for Spec.Tests reorganized into per-file bullets
(one per module being tested); replaces the stale 31-tests-in-3-files
description.
Last verified date bumped to 2026-05-25.
The module produces an ASCII rondel board diagram with nation tokens placed on spaces — that's a noun (Board), not a process (StateFormatting). Renames to match the framework's noun-module + verb-function pattern (Markdown.render, SpecFilter.apply, CollectionAssert.forAccessor): - File: StateFormatting.fs → Board.fs - Module: module Imperium.UnitTests.Rondel.StateFormatting → [<RequireQualifiedAccess>] module Imperium.UnitTests.Rondel.Board (adds RequireQualifiedAccess for consistency with the other flat utility modules in the spec framework) - Function: format → render (parallels Markdown.render) - Call site: Some StateFormatting.format → Some Board.render AGENTS.md and the fsproj compile order updated accordingly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR reorganizes the test specification framework: it replaces the monolithic Spec module with a modular Imperium.Testing.Spec (Specification DSL, SpecRunner, SpecFilter, Markdown, CollectionAssert), migrates Accounting and Rondel test suites to the new framework (contexts, assertions, renderers), updates the test project compile ordering and test entrypoint, and refreshes docs and examples. ChangesSpec Framework Restructuring and Test Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Imperium.UnitTests/Imperium/Accounting/Assertions.fs (1)
10-10: ⚡ Quick winMake collection accessor module-private.
Use
let private events = ...so only assertion helpers are exposed from this module.As per coding guidelines:
tests/Imperium.UnitTests/**/{Context,Assertions,Specs}.fs: “for repeated collection checks in assertions, define accessors likelet private events = CollectionAssert.forAccessor (...)”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Imperium.UnitTests/Imperium/Accounting/Assertions.fs` at line 10, The binding events in Assertions.fs is public but should be module-private; change the declaration of events (the result of forAccessor taking Context -> ctx.Events) to use a private binding (e.g., let private events = forAccessor (fun (ctx: Context) -> ctx.Events :> seq<_>)) so only the assertion helpers are exposed; update the events symbol accordingly and leave forAccessor and Context usage as-is.tests/Imperium.UnitTests/Imperium/Rondel/Assertions.fs (1)
12-14: ⚡ Quick winKeep accessor helpers private.
Please mark both accessors private (
let private events,let private commands) to avoid widening module surface unnecessarily.As per coding guidelines:
tests/Imperium.UnitTests/**/{Context,Assertions,Specs}.fs: “for repeated collection checks in assertions, define accessors likelet private events = CollectionAssert.forAccessor (...)”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Imperium.UnitTests/Imperium/Rondel/Assertions.fs` around lines 12 - 14, The two accessor bindings are public but should be private; change the declarations for events and commands to be private (e.g. make the bindings named events and commands use "let private") so the accessors created with forAccessor (used with Context -> ctx.Events and Context -> ctx.Commands) are not exposed from the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.md`:
- Line 112: Documentation refers to the wrong symbol: replace the incorrect
`Markdown.toMarkdownDocument` reference with the actual function name
`Markdown.render` (as implemented in `Markdown.fs` and referenced elsewhere) so
the description matches the code and the AI summary; ensure the sentence still
describes that `Markdown.render` calls `runExpectation` for every expectation
and renders all results without aborting on failures.
---
Nitpick comments:
In `@tests/Imperium.UnitTests/Imperium/Accounting/Assertions.fs`:
- Line 10: The binding events in Assertions.fs is public but should be
module-private; change the declaration of events (the result of forAccessor
taking Context -> ctx.Events) to use a private binding (e.g., let private events
= forAccessor (fun (ctx: Context) -> ctx.Events :> seq<_>)) so only the
assertion helpers are exposed; update the events symbol accordingly and leave
forAccessor and Context usage as-is.
In `@tests/Imperium.UnitTests/Imperium/Rondel/Assertions.fs`:
- Around line 12-14: The two accessor bindings are public but should be private;
change the declarations for events and commands to be private (e.g. make the
bindings named events and commands use "let private") so the accessors created
with forAccessor (used with Context -> ctx.Events and Context -> ctx.Commands)
are not exposed from the module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14dd2fce-16ee-4223-b9be-c2ee48093203
📒 Files selected for processing (31)
AGENTS.mddocs/architecture.mdtests/Imperium.UnitTests/Imperium.Terminal/Accounting/HostTests.fstests/Imperium.UnitTests/Imperium.Terminal/BusTests.fstests/Imperium.UnitTests/Imperium.Terminal/Rondel/DirectCommitTests.fstests/Imperium.UnitTests/Imperium.Terminal/Rondel/HostTests.fstests/Imperium.UnitTests/Imperium.Terminal/Rondel/StoreTests.fstests/Imperium.UnitTests/Imperium.UnitTests.fsprojtests/Imperium.UnitTests/Imperium/Accounting/Assertions.fstests/Imperium.UnitTests/Imperium/Accounting/Context.fstests/Imperium.UnitTests/Imperium/Accounting/Specs.fstests/Imperium.UnitTests/Imperium/Contract/AccountingContractTests.fstests/Imperium.UnitTests/Imperium/Contract/RondelContractTests.fstests/Imperium.UnitTests/Imperium/Gameplay/GameplayTests.fstests/Imperium.UnitTests/Imperium/Rondel/Assertions.fstests/Imperium.UnitTests/Imperium/Rondel/Board.fstests/Imperium.UnitTests/Imperium/Rondel/Context.fstests/Imperium.UnitTests/Imperium/Rondel/Specs.fstests/Imperium.UnitTests/Main.fstests/Imperium.UnitTests/Spec.fstests/Imperium.UnitTests/SpecTests.fstests/Imperium.UnitTests/Support/Spec.Tests/CollectionAssertTests.fstests/Imperium.UnitTests/Support/Spec.Tests/FilterTests.fstests/Imperium.UnitTests/Support/Spec.Tests/MarkdownTests.fstests/Imperium.UnitTests/Support/Spec.Tests/SpecRunnerTests.fstests/Imperium.UnitTests/Support/Spec.Tests/SpecificationTests.fstests/Imperium.UnitTests/Support/Spec/CollectionAssert.fstests/Imperium.UnitTests/Support/Spec/Filter.fstests/Imperium.UnitTests/Support/Spec/Markdown.fstests/Imperium.UnitTests/Support/Spec/SpecRunner.fstests/Imperium.UnitTests/Support/Spec/Specification.fs
💤 Files with no reviewable changes (3)
- tests/Imperium.UnitTests/SpecTests.fs
- tests/Imperium.UnitTests/Imperium/Gameplay/GameplayTests.fs
- tests/Imperium.UnitTests/Spec.fs
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
1 participant
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.
Closes #137.
Summary
Continues the structural cleanup tracked by issue #137 with a sweep across the
Imperium.UnitTestsspec framework and per-BC test infrastructure. All renames are mechanical; no behavior changes. 130 tests pass throughout.SpecRunnerandSpecificationnow use the canonical F#namespace + type + companion moduleshape (matchesOption,Result,Map).SpecMarkdown→Markdown(drops redundant namespace-repeating prefix),MarkdownRenderOptions→Markdown.RenderOptions.SpecFilter.Trecord collapsed to aPredicate = string list -> booltype alias.runActions,prepareContext, andtoMarkdownDocumentprivatized.SpecificationTests.fspreviously mixed three modules under a generic"Spec"testList. Split intoSpecificationTests,SpecRunnerTests,CollectionAssertTests— each file maps 1:1 to a framework module.AccountingContext/RondelContexttypes renamed toContext,createContextfactory renamed toContext.create, both BCs promoted tonamespace + type + companion moduleshape.accountingSpecs/rondelSpecs→specifications. TheSpecRunnervalue moved from eachContext.fsinto the only file that uses it (Specs.fs) as a private value.StateFormatting: Renamed toBoard.render— module name describes what it produces (a board diagram) and matches the framework's noun-module + verb-function pattern.renderSpecMarkdownfunction →renderMarkdown(CLI flag preserved for CI). Dropped 5 unusedopenstatements.AGENTS.mdsynced to reflect all renames.Summary by CodeRabbit