diff --git a/docs/wiki/Testing-And-Test-Data.md b/docs/wiki/Testing-And-Test-Data.md index 4e11526..b891d43 100644 --- a/docs/wiki/Testing-And-Test-Data.md +++ b/docs/wiki/Testing-And-Test-Data.md @@ -7,11 +7,14 @@ | Framework | TUnit. Not xUnit, NUnit or MSTest. | | Substitutes | TUnit.Mocks. Not NSubstitute or Moq. | | Test data | Bogus, reached through `TestBuilder`. | -| Structure | Explicit `// Arrange`, `// Act`, `// Assert`. | +| Acceptance tests | BDTest over TUnit, through `BDTest.TUnit`. Not Gherkin, SpecFlow or Reqnroll. | +| A real dependency | `Purview.Containers`. Not Testcontainers directly, and never a shared instance. | +| Structure | Explicit `// Arrange`, `// Act`, `// Assert` — Given/When/Then in an acceptance test. | | Naming | `{SubjectUnderTest}_{Scenario}_{Expectation}`, and `async Task`. | | Placement | One `{Class}Tests` class per class under test, **in the namespace of the class under test**. | | Construction | Dependencies and the system under test come from `CreateXXX` helpers with optional parameters. | -| Cancellation | Pass a `CancellationToken` wherever a downstream method accepts one. | +| Async | The async overload wherever the API offers one — in the test **and in every helper it calls**. A sync helper that calls such an API is converted, or carries a stated reason it cannot be. | +| Cancellation | If any method the test calls accepts a `CancellationToken`, the test's **last parameter** is one, and it is forwarded to every call that accepts one. | | Domain content | Built through `TestBuilder`. Never inline. | ## Why @@ -20,6 +23,11 @@ is the reason a test project file is empty. A second framework in the same repository means every project file grows a conditional, and the pipeline grows a second discovery path. +That is a rule about the **runner**, not about the style of a test. BDTest is allowed for acceptance tests +precisely because `BDTest.TUnit` is an adapter *over* TUnit rather than a runner of its own: the discovery +path, the filter and the emitted category are unchanged. A BDD framework that brought its own runner would +be the thing this rule exists to refuse. + **Same namespace.** A test in the namespace of the class it tests reads as part of the thing it describes, needs no `using` to reach the type, and cannot silently test a different type with the same name. It also makes the pairing obvious in a file list. @@ -117,6 +125,122 @@ public async Task Create_GivenSlugThatIsNotALowercaseSlug_Throws(string value) } ``` +## Async all the way down + +**Where an API offers an async overload, a test uses it.** `File.ReadAllTextAsync`, not +`File.ReadAllText`. That much the build catches. The part that needs stating is the second half: + +**The rule reaches through test helpers.** A helper that calls an API with an async overload is itself +converted to async — it is not exempt for being a helper, and "it is only a test" is not a reason. + +The build enforces the first half and cannot enforce the second, and the gap between them is the whole +point: + +- `VSTHRD103` is an `error` in the SDK's `.editorconfig`, and it flags a synchronous call with an + `Async`-suffixed equivalent **when the call sits inside a `Task`-returning method**. In an `async Task` + test, `File.ReadAllText` fails the build. +- A **synchronous helper is not a `Task`-returning method**, so the same call inside it is invisible to + the analyzer. Moving the sync call one level down turns a build error into nothing at all, and the + suite still looks clean. + +That hole is not hypothetical — this repository had it. `PolicyDocumentationTests` read its page through +`static string Documentation => File.ReadAllText(DocumentationPath);`, and five `async Task` tests used it +with a clean build, because none of them contained the offending call. + +It compounds with the [cancellation rule](#cancellation), which keys off whether any method in the chain +accepts a `CancellationToken`. A sync helper accepts none, so it **silently removes the trigger**: the +tests above correctly declared no token, and were correctly uncancellable, and nothing said so. The sync +helper is where a chain quietly stops being cancellable. + +There is a plainer reason too. TUnit runs tests in parallel, and a blocking read holds a thread-pool +thread that an async read would have returned. + +### Converting a helper + +Three things change together, and the first two are not optional: + +```csharp +// Before — invisible to VSTHRD103, and the chain accepts no token +static string Documentation => File.ReadAllText(DocumentationPath); + +// After +static async Task ReadDocumentationAsync(CancellationToken cancellationToken) => + await File.ReadAllTextAsync(DocumentationPath, cancellationToken); +``` + +- **It returns a `Task`**, which is what brings every call inside it back under `VSTHRD103`. +- **It gains the `Async` suffix.** `VSTHRD200` is an `error`, so the rename is enforced rather than + stylistic. +- **It takes a `CancellationToken` last**, because the chain now accepts one — see + [Cancellation](#cancellation). Its callers then declare and forward one. + +**A property cannot be `async`, so it becomes a method.** That is the conversion, not an exemption from +it; a read-only property hiding IO is the most common form this violation takes. + +### When it genuinely cannot be converted + +State the reason where the call is, and keep it a fact about the API rather than a preference: + +- **No async overload exists.** `File.Exists`, `File.Delete` and `Directory.EnumerateFiles` have none, so + they stay as they are. In this repository `ArchitecturePolicy.Load()` and `PolicySchemaValidator.Validate()` + are the same case: `Purview.Architecture.Testing` ships synchronous methods only, which is checkable + rather than arguable. +- **A base type or interface fixes the signature.** Override what you are given. + +What is never a conversion: `Task.Run` around a blocking call, `Task.FromResult` around a sync result, or +`.Result` and `.GetAwaiter().GetResult()` to flatten a task. The last two are `VSTHRD002`, an `error` +here, and the first two move the block rather than remove it. + +## Cancellation + +The rule has three parts, and the trigger is the call chain rather than the test: + +**If any method the test calls accepts a `CancellationToken`, the test declares one.** Not only the method +under test — anything in the chain, including the `File.ReadAllTextAsync` a repository test reaches for and +the delegate handed to `Assert.That`. The moment one such call exists, the test gains the parameter. + +**It is the last parameter.** TUnit supplies the value; nothing in the test constructs it: + +```csharp +[Test] +public async Task RegisterAsync_GivenAnUnregisteredConvention_StoresIt(CancellationToken cancellationToken) +``` + +With `[Arguments]`, the data binds to the leading parameters and the token is supplied on top of them, so +the token goes last and the attribute's positional order still matches the signature: + +```csharp +[Test] +[Arguments("build-enforced", true)] +[Arguments("review-only", false)] +public async Task ListEnforcedByBuildAsync_GivenAConvention_FiltersIt( + string id, + bool enforcedByBuild, + CancellationToken cancellationToken +) +``` + +**It is forwarded to every call that accepts one.** This is the part that is actually worth the rule: + +```csharp +// Act +var result = await catalogue.ListEnforcedByBuildAsync(cancellationToken); +``` + +Never `CancellationToken.None`, never `default`, and never a token the test creates for itself. Two +failures follow from doing otherwise: + +- **A hung call hangs the run instead of failing one test.** TUnit's token is the *test's* token — it is + cancelled when the test times out or the run is aborted — so a call given `None` cannot be stopped. One + method that drops the token is enough: the chain is only as cancellable as its least cooperative hop, + which is why *partial* forwarding is the trap rather than a lesser version of the rule. +- **Nothing proves the production code threads the token through.** A test that passes `None` exercises + the signature without exercising the behaviour, and the test is the only place a swallowed token would + have been noticed. + +A test whose chain accepts no token takes no parameter — the parameterised value-object test above is the +shape, and adding an unused token there is noise, not compliance. + ## Substitutes TUnit.Mocks is globally imported by the SDK, so a test file needs no `using`. The idiom is to set up the @@ -215,6 +339,138 @@ var id = TestBuilder.ConventionId("build-enforced"); // method call -> var string title = new('a', Convention.MaxTitleLength); // new() -> explicit type ``` +## Acceptance tests + +An acceptance test states a behaviour somebody outside the team asked for, in their words. It is a +different artefact from a unit test, and it gets a different shape: **[BDTest](https://bdtest.tomlonghurst.dev/) +over TUnit, through the `BDTest.TUnit` adapter, in a `.AcceptanceTests` project.** + +Two things make BDTest the choice rather than Gherkin, SpecFlow or Reqnroll: + +- **It is code-only.** There is no `.feature` file and no generated test body, so there is no second + artefact that can drift from the step that implements it. The drift between a feature file and its + bindings is the failure that makes a BDD suite stop being trusted, and it is a failure this shape cannot + have. +- **It keeps one runner.** `BDTest.TUnit` is an adapter over TUnit, so the SDK's wiring, the category and + the `--treenode-filter` all still apply. Nothing in the pipeline learns a second way to find a test. + +A scenario is a story, a context, a steps class and a chain: + +```csharp +[Story(AsA = "account holder", IWant = "to withdraw money", SoThat = "I can use my savings")] +public class WithdrawalTests : TUnitBDTestBase +{ + AccountSteps Account => new(Context); + + [Test] + [ScenarioText("Withdrawing money reduces the balance")] + public async Task WithdrawMoney() => + await Given(() => Account.CreateAnAccount(100m)) + .When(() => Account.Withdraw(25m)) + .Then(() => Account.TheBalanceIs(75m)) + .BDTestAsync(); +} + +public class AccountSteps(AccountContext context) +{ + [StepText("I have an account with a balance of {0}")] + public void CreateAnAccount(decimal balance) => context.Balance = balance; + + [StepText("I withdraw {0}")] + public void Withdraw(decimal amount) => context.Balance -= amount; + + [StepText("the account balance is {0}")] + public async Task TheBalanceIs(decimal expected) => await Assert.That(context.Balance).IsEqualTo(expected); +} +``` + +Five rules govern it, and the first is the one that bites: + +- **`.BDTestAsync()` is what runs the scenario.** A chain that stops at `.Then(...)` compiles, passes, and + executes none of its steps — the same failure mode as an un-awaited `Assert.That`, and the reason both + are called out rather than assumed. +- **Given/When/Then replaces Arrange/Act/Assert, and only here.** The chain already names the three phases, + so the comments would be noise. Everywhere else the AAA comments stay. +- **`Context` is the only state.** Steps receive it through the constructor and never hold fields of their + own; BDTest gives each test its own context, which is what keeps a parallel or repeated run honest. +- **`[StepText]` is written for the reader of the report, not the author of the test.** `{0}` is replaced + with the argument's actual value at run time, so the report states what the scenario did rather than what + it was called. +- **Domain content still comes from `TestBuilder`.** The step is where the vocabulary changes; the test-data + rule does not. + +Assertions inside a step are ordinary TUnit assertions, awaited as everywhere else. `BDTest.Reporting` +produces the single-file HTML report; publish it as a build artifact from the job that ran the suite, and +treat sharing it with the people who asked for the behaviour as the point of writing the suite this way. + +**`src/` does not demonstrate this.** The reference implementation is a catalogue of conventions behind one +service — it has no user-visible behaviour to accept — so adding a BDTest suite would add a package and a +second job to prove nothing this page does not already state. The rule stands on +[the BDTest documentation](https://bdtest.tomlonghurst.dev/) and the example above. + +## A test that needs a real dependency + +A database, a queue, a blob store or an emulator comes from +[`Purview.Containers`](https://purview.dev/docs/containers/) — a throwaway Linux container per suite, behind +one API over either Microsoft WSL Containers or Docker, with typed modules for SQL Server, PostgreSQL, +Redis, MySQL, RabbitMQ, Azurite and NATS. + +The two alternatives are what the rule exists to refuse: + +- **A shared instance** — a developer's local SQL Server, a team database — makes tests order-dependent and + a failure unreproducible, because the state a test found was left by whichever test ran before it, on + whichever machine. +- **Testcontainers directly** ties the suite to a Docker installation. On Windows that is an install and a + licensing question for every developer and every runner. The WSL Containers backend removes it, and the + same test code runs on either backend, so the choice is made per machine rather than per repository. + +```csharp +await using var postgres = new PostgreSqlBuilder() + .WithDatabase("tests") + .WithUsername("postgres") + .WithPassword("postgres") + .Build(); + +await postgres.StartAsync(); + +await using var connection = new NpgsqlConnection(postgres.GetConnectionString()); +await connection.OpenAsync(); +``` + +`Build()` validates the configuration, `StartAsync()` returns only once every wait strategy is satisfied — +so there is no sleep to tune — and `await using` disposes the container at the end of the scope. + +Three rules make such a suite safe to own: + +- **It lives in its own `.IntegrationTests` project.** The tier comes from the name, which keeps it + out of the pull-request filter without anybody remembering an attribute. See + [Verification Model](Verification-Model.md). +- **It skips locally when no backend is usable, and fails in CI.** Those are two different situations and + must not share an outcome. Locally, probe and skip; in CI, pin the backend with + `PURVIEW_CONTAINERS_BACKEND`, because a named backend never falls back, so an unavailable runtime fails + loudly rather than letting a green job mean nothing ran: + + ```csharp + [Before(Class)] + public static async Task EnsureABackendIsAvailable() + { + var backends = await ContainerBackends.ProbeAllAsync(); + + Skip.Unless(backends.Any(backend => backend.IsUsable), "No container backend is available."); + } + ``` + + Left unpinned, the automatic selection probes in priority order and prefers WSL Containers over Docker. +- **The version is pinned exactly.** `Purview.Containers` is **experimental and prerelease** — its API, + defaults and packaging rules can change between releases — so treat it as the direction of travel rather + than a stability guarantee. The `Purview.Containers` packages are a family, so their version is a single + property in `Directory.Packages.props` and a bump cannot be applied to half of them. + +**`src/` does not demonstrate this either**, and for the stated reason: this repository ships no persistence +and its pipeline runs the `[Category=Unit]` tier only. +[containers](https://github.com/purview-dev/containers) is the repository to read instead — its +`integration-docker` job is the second-job pattern in use. + ## How it is enforced | Rule | Enforced by | @@ -223,23 +479,30 @@ string title = new('a', Convention.MaxTitleLength); // new() -> explic | Test projects are discovered and categorised | The build (name-suffix detection) | | A shared testing project is referenced, not executed | The build (`IsSharedTestingProject` handling) | | Style, including the `var` carve-out | The build (analyzers, at `warning` or above) | +| An acceptance suite is categorised `Acceptance` and kept out of the fast tier | The build (name-suffix detection) and the pipeline's `TestFilter` | +| A container-backed suite fails rather than passes when its backend is missing in CI | The pipeline, when the backend is pinned with `PURVIEW_CONTAINERS_BACKEND` | | AAA structure, naming, `CreateXXX` helpers, `TestBuilder` usage | Review | -| A `CancellationToken` is passed through | Review | +| Given/When/Then chains end in `.BDTestAsync()` | Review | +| A real dependency comes from `Purview.Containers` rather than a shared instance | Review | +| An async overload is used inside an `async Task` test | The build — `VSTHRD103` is `error` in the SDK's `.editorconfig` | +| A converted helper carries the `Async` suffix | The build — `VSTHRD200` is `error` | +| A **sync helper** that calls an API with an async overload is converted | Review — a non-`Task`-returning method is outside `VSTHRD103`'s scope, so nothing else can see it | +| A declared `CancellationToken` is the last parameter | The build — `CA1068` is `warning` in the SDK's `.editorconfig`, and a clean build has no warnings | +| A test whose chain accepts a token declares one, and forwards it to every call that accepts one | Review | ## At your scale - **One test project.** TUnit, AAA, the naming convention, and `TestBuilder`. That is the whole of it. - **A shared fixture.** Move it into `SharedTestingInfra` and make the class public — a shared testing project is a library, so its fixtures are legitimately public API and `CA1515` does not apply. -- **A database or a container.** [`Purview.Containers`](https://purview.dev/docs/containers/) is the - organisation's answer: throwaway Linux containers behind one API, over either Microsoft WSL Containers - (no Docker installation on Windows) or Docker, with typed modules for SQL Server, PostgreSQL, Redis, - MySQL, RabbitMQ, Azurite and NATS. Give the suite its own test project so the tier is expressed by the - name, and make it skip itself when no backend is reachable. - - It is **experimental and prerelease** — the API, defaults and packaging rules can change between - releases — so treat it as the direction of travel rather than a stability guarantee, and pin the - version you adopt. +- **A database or a container.** Add one `.IntegrationTests` project and reach for + `Purview.Containers` as described in [A test that needs a real + dependency](#a-test-that-needs-a-real-dependency). One module and one container is the whole of it at + this size; do not build a fixture framework around it until a second suite needs the same container. +- **A behaviour somebody outside the team asked for.** That is when BDTest earns its place, and not + before — a `.AcceptanceTests` project with one story in it is a reasonable start, and a + repository with no such audience should keep writing unit tests. Reach for the HTML report only once + somebody is actually going to read it. - **A package with a documented contract.** Add the consumer-verification script described in [Engineering Principles](Engineering-Principles.md). @@ -248,5 +511,7 @@ string title = new('a', Convention.MaxTitleLength); // new() -> explic - [Verification Model](Verification-Model.md) - [Architecture as Tests](Architecture-As-Tests.md) - [Worked Examples](Worked-Examples.md) +- [BDTest](https://bdtest.tomlonghurst.dev/) +- [Purview.Containers](https://purview.dev/docs/containers/) - [build-sdk: Testing Wiring](https://github.com/purview-dev/build-sdk/blob/main/docs/wiki/Testing-Wiring.md) diff --git a/docs/wiki/Verification-Model.md b/docs/wiki/Verification-Model.md index 9a167c0..4a0f8ea 100644 --- a/docs/wiki/Verification-Model.md +++ b/docs/wiki/Verification-Model.md @@ -14,9 +14,14 @@ add: the SDK reads the test project's name suffix and emits an assembly-level ca | `.UnitTests` | `Unit` | Yes | | `Architecture.UnitTests` | `Unit` | Yes | | `.ContractTests` | `Contract` | Usually | +| `.AcceptanceTests` | `Acceptance` | No — its own job | | `.IntegrationTests` | `Integration` | Only when the resource is available | | `.E2ETests` | `E2E` | No | +The category is a pattern match rather than a lookup — the SDK takes the word before `Tests` — so +`Acceptance` needs nothing added to the pipeline to be recognised. `Acceptance` is also in the SDK's +`TestType` list, which is what generates the `InternalsVisibleTo` back to the target project. + `Architecture.UnitTests` is named that way **deliberately**, and the obvious alternative is a trap. The SDK takes the category from the word before `Tests`, so `Architecture.ArchitectureTests` would get `Category=Architecture` — and a pull-request filter of `[Category=Unit]` would quietly stop running the @@ -107,6 +112,13 @@ end to end. Either way the rule holds: **the fast tier is always on, and the slow tier is explicit.** +A BDTest acceptance suite is a slow tier of this kind, with one addition: the job that runs it publishes the +`BDTest.Reporting` HTML report as a build artifact, because the report is the reason the suite is written +in that shape at all. A container-backed suite adds a different one: pin the backend with +`PURVIEW_CONTAINERS_BACKEND` in the job, so a runner without that runtime fails the job rather than +skipping every test and reporting green. Both are covered in [Testing and Test +Data](Testing-And-Test-Data.md). + ## The configuration file `purview-build.json` is probed for in a documented order, first match wins, and files are never merged: diff --git a/package.json b/package.json index c7d0a86..af0e4ff 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "engineering-playbook", - "version": "0.1.0-prerelease.2", + "version": "0.1.0-prerelease.3", "license": "MIT", "author": { "name": "Kieron Lanning", diff --git a/skills/purview-testing-conventions/SKILL.md b/skills/purview-testing-conventions/SKILL.md index 078f85f..1fe3232 100644 --- a/skills/purview-testing-conventions/SKILL.md +++ b/skills/purview-testing-conventions/SKILL.md @@ -4,7 +4,8 @@ description: >- Use when writing or reviewing tests in a .NET repository following the Purview conventions - TUnit and TUnit.Mocks, the AAA structure and naming, placing a test in the namespace of the class under test, CreateXXX construction helpers, building domain content through TestBuilder with Bogus, the - assertion surface, parameterised tests, and which tier a test project belongs to. + assertion surface, parameterised tests, which tier a test project belongs to, BDTest for + acceptance tests, and Purview.Containers for a suite that needs a real database or queue. metadata: product: Purview engineering playbook repository: purview-dev/engineering-playbook @@ -17,18 +18,23 @@ metadata: | Framework | **TUnit**. Not xUnit, NUnit or MSTest. | | Substitutes | **TUnit.Mocks**. Not NSubstitute or Moq. | | Test data | **Bogus**, reached through `TestBuilder`. | -| Structure | Explicit `// Arrange`, `// Act`, `// Assert`. | +| Acceptance tests | **BDTest** over TUnit, via `BDTest.TUnit`. Not Gherkin, SpecFlow or Reqnroll. | +| A real dependency | **`Purview.Containers`**. Not Testcontainers directly, never a shared instance. | +| Structure | Explicit `// Arrange`, `// Act`, `// Assert` — Given/When/Then in an acceptance test. | | Naming | `{SubjectUnderTest}_{Scenario}_{Expectation}`, and `async Task`. | | Placement | One `{Class}Tests` class per class under test, **in the namespace of the class under test**. | | Construction | Dependencies and the system under test come from `CreateXXX` helpers with optional parameters. | -| Cancellation | Pass a `CancellationToken` wherever a downstream method accepts one. | +| Async | The async overload wherever the API offers one — in the test **and in every helper it calls**. Convert a sync helper, or state why it cannot be. | +| Cancellation | If any method the test calls accepts a `CancellationToken`, the test's **last parameter** is one, forwarded to every call that accepts one. | | Domain content | Built through `TestBuilder`. Never inline. | ## Why these, specifically **One framework.** The SDK wires TUnit into any project whose name ends in a test suffix — that wiring is why a test project file is empty. A second framework means every project file grows a -conditional and the pipeline grows a second discovery path. +conditional and the pipeline grows a second discovery path. That is a rule about the **runner**, not +the style of a test — `BDTest.TUnit` is an adapter *over* TUnit, so an acceptance suite does not break +it. **Same namespace as the subject.** The test reads as part of the thing it describes, needs no `using` to reach the type, and **cannot silently test a different type with the same name**. @@ -122,6 +128,80 @@ public async Task Create_GivenSlugThatIsNotALowercaseSlug_Throws(string value) } ``` +## Async all the way down + +**Where an API offers an async overload, use it** — `File.ReadAllTextAsync`, not `File.ReadAllText`. +**And the rule reaches through test helpers:** a helper calling such an API is itself converted. Being +a helper is not an exemption. + +The build enforces the first half and cannot enforce the second, which is why the second needs saying: + +- `VSTHRD103` is an **`error`** and flags a sync call with an `Async`-suffixed equivalent **when the + call sits inside a `Task`-returning method**. In an `async Task` test, `File.ReadAllText` fails the + build. +- A **sync helper is not `Task`-returning**, so the identical call inside it is invisible to the + analyzer. Pushing the call one level down turns a build error into nothing. + +It also compounds with the cancellation rule, which keys off whether anything in the chain accepts a +`CancellationToken`: a sync helper accepts none, so it **silently removes the trigger** and the test is +uncancellable while looking compliant. (TUnit also runs tests in parallel, so a blocking read holds a +thread-pool thread an async read would release.) + +Converting — three things change together, and the first two are not optional: + +```csharp +// Before — invisible to VSTHRD103, and the chain accepts no token +static string Documentation => File.ReadAllText(DocumentationPath); + +// After +static async Task ReadDocumentationAsync(CancellationToken cancellationToken) => + await File.ReadAllTextAsync(DocumentationPath, cancellationToken); +``` + +- It returns a `Task`, bringing every call inside it back under `VSTHRD103`. +- It gains the **`Async` suffix** — `VSTHRD200` is an `error`, so the rename is enforced. +- It takes a trailing `CancellationToken`, because the chain now accepts one; its callers declare and + forward one. + +**A property cannot be `async`, so it becomes a method** — that is the conversion, not an exemption. +A read-only property hiding IO is the most common form of this violation. + +It genuinely cannot be converted when **no async overload exists** (`File.Exists`, `File.Delete`, +`Directory.EnumerateFiles`; likewise `ArchitecturePolicy.Load()` and `PolicySchemaValidator.Validate()`, +which `Purview.Architecture.Testing` ships synchronous-only) or when **a base type or interface fixes +the signature**. State the reason at the call. Never fake it: `Task.Run` around a blocking call, +`Task.FromResult` around a sync result, or `.Result` / `.GetAwaiter().GetResult()` to flatten a task — +the last two are `VSTHRD002`, an `error`. + +## Cancellation + +Three parts, triggered by the **call chain** rather than by the test: + +1. **If any method the test calls accepts a `CancellationToken`, the test declares one.** Not only the + method under test — anything in the chain, including a `File.ReadAllTextAsync` or the delegate handed + to `Assert.That`. +2. **It is the last parameter**, and TUnit supplies the value. With `[Arguments]`, the data binds to the + leading parameters and the token is supplied on top, so the attribute's positional order still matches + the signature: + + ```csharp + [Test] + [Arguments("build-enforced", true)] + public async Task ListEnforcedByBuildAsync_GivenAConvention_FiltersIt( + string id, + bool enforcedByBuild, + CancellationToken cancellationToken + ) + ``` + +3. **It is forwarded to every call that accepts one.** Never `CancellationToken.None`, never `default`, + never a token the test creates. TUnit's token is the *test's* token — cancelled on timeout or on an + aborted run — so a call given `None` cannot be stopped, and **one hop that drops it makes the whole + chain uncancellable**, which is why partial forwarding is a failure and not a lesser version of the + rule. It is also the only place that proves the production code threads the token through at all. + +A test whose chain accepts no token takes no parameter; an unused token is noise, not compliance. + ## Substitutes TUnit.Mocks is globally imported by the SDK, so no `using` is needed: @@ -224,14 +304,35 @@ string title = new('a', Convention.MaxTitleLength); // new() -> explici | Test projects are discovered and categorised | the build (name-suffix detection) | | A shared testing project is referenced, not executed | the build (`IsSharedTestingProject`) | | Style, including the `var` carve-out | the build (analyzers, `warning` or above) | +| An acceptance suite is categorised `Acceptance` and kept out of the fast tier | the build (name-suffix detection) and the pipeline's `TestFilter` | +| A container-backed suite fails rather than passes when its backend is missing in CI | the pipeline, when the backend is pinned with `PURVIEW_CONTAINERS_BACKEND` | | AAA structure, naming, `CreateXXX` helpers, `TestBuilder` usage | **review** | -| A `CancellationToken` is passed through | **review** | +| Given/When/Then chains end in `.BDTestAsync()` | **review** | +| A real dependency comes from `Purview.Containers`, not a shared instance | **review** | +| An async overload is used inside an `async Task` test | the build — `VSTHRD103` is `error` | +| A converted helper carries the `Async` suffix | the build — `VSTHRD200` is `error` | +| A **sync helper** calling an API with an async overload is converted | **review** — a non-`Task`-returning method is outside `VSTHRD103`'s scope | +| A declared `CancellationToken` is the last parameter | the build — `CA1068` is `warning`, and a clean build has no warnings | +| A test whose chain accepts a token declares one and forwards it everywhere | **review** | + +## Acceptance tests and container-backed tests + +An acceptance test — a behaviour somebody outside the team asked for, in their words — is written with +**BDTest over TUnit** (`BDTest.TUnit`) in a `.AcceptanceTests` project, where Given/When/Then +replaces the AAA comments. A suite that needs a real database, queue or emulator gets a throwaway +container from **`Purview.Containers`** in its own `.IntegrationTests` project, skipping +locally when no backend is usable and failing in CI. + +Read `references/acceptance-and-container-tests.md` before writing either: it carries the scenario and +container shapes, and the two traps — a chain that never calls `.BDTestAsync()` runs no steps and still +passes, and a suite that skips in CI turns a missing runtime into a green job. ## Tiers -A test project's **name** carries its tier — the SDK categorises `*UnitTests` as `Unit` — so the -pipeline's `TestFilter` selects a tier without a hand-written attribute. Split a slow suite into its -own project rather than tagging individual tests. +A test project's **name** carries its tier — the SDK categorises `*UnitTests` as `Unit`, and takes the +word before `Tests` as a pattern, so `*AcceptanceTests` gives `Acceptance` with nothing added to the +pipeline. The `TestFilter` then selects a tier without a hand-written attribute. Split a slow suite +into its own project rather than tagging individual tests. Filtering uses TUnit's `--treenode-filter`, not `--filter`: these repositories run on Microsoft.Testing.Platform, where `dotnet test --filter` reports "Zero tests ran". @@ -242,10 +343,11 @@ Microsoft.Testing.Platform, where `dotnet test --filter` reports "Zero tests ran - **A shared fixture.** Move it into `SharedTestingInfra` and make the class public — a shared testing project is a library, so its fixtures are legitimately public API and `CA1515` does not apply. -- **A database or a container.** `Purview.Containers` gives throwaway Linux containers behind one - API, over WSL Containers or Docker. Give the suite its own project so the tier is in the name, and - make it skip itself when no backend is reachable. It is **experimental and prerelease** — pin the - version you adopt. +- **A database or a container.** One `.IntegrationTests` project and one `Purview.Containers` + module. Do not build a fixture framework around it until a second suite needs the same container. +- **A behaviour somebody outside the team asked for.** That, and not before, is when BDTest earns its + place: one `.AcceptanceTests` project with one story. Reach for the HTML report only once + somebody will read it. - **A package with a documented contract.** Add a consumer-verification script; see `purview-engineering-principles`. diff --git a/skills/purview-testing-conventions/references/acceptance-and-container-tests.md b/skills/purview-testing-conventions/references/acceptance-and-container-tests.md new file mode 100644 index 0000000..e8b9110 --- /dev/null +++ b/skills/purview-testing-conventions/references/acceptance-and-container-tests.md @@ -0,0 +1,117 @@ +# Acceptance tests and container-backed tests + +Two shapes that sit outside the unit tier. Both keep TUnit as the runner, and both live in their own +test project so the tier comes from the project name rather than an attribute. + +## Acceptance tests: BDTest + +An acceptance test states a behaviour somebody outside the team asked for, in their words. Use +**BDTest over TUnit through the `BDTest.TUnit` adapter**, in a `.AcceptanceTests` project +(the SDK emits `Category=Acceptance` for it). + +Not Gherkin, SpecFlow or Reqnroll, for two reasons: + +- **BDTest is code-only.** No `.feature` file and no generated test body, so there is no second + artefact that can drift from the step implementing it. That drift is the failure that makes a BDD + suite stop being trusted. +- **One runner stays one runner.** `BDTest.TUnit` is an adapter over TUnit, so the SDK's wiring, the + emitted category and `--treenode-filter` are all unchanged. A BDD framework with its own runner is + exactly what the one-framework rule refuses. + +```csharp +[Story(AsA = "account holder", IWant = "to withdraw money", SoThat = "I can use my savings")] +public class WithdrawalTests : TUnitBDTestBase +{ + AccountSteps Account => new(Context); + + [Test] + [ScenarioText("Withdrawing money reduces the balance")] + public async Task WithdrawMoney() => + await Given(() => Account.CreateAnAccount(100m)) + .When(() => Account.Withdraw(25m)) + .Then(() => Account.TheBalanceIs(75m)) + .BDTestAsync(); +} + +public class AccountSteps(AccountContext context) +{ + [StepText("I have an account with a balance of {0}")] + public void CreateAnAccount(decimal balance) => context.Balance = balance; + + [StepText("I withdraw {0}")] + public void Withdraw(decimal amount) => context.Balance -= amount; + + [StepText("the account balance is {0}")] + public async Task TheBalanceIs(decimal expected) => await Assert.That(context.Balance).IsEqualTo(expected); +} +``` + +Five rules, and the first is the one that bites: + +- **`.BDTestAsync()` runs the scenario.** A chain stopping at `.Then(...)` compiles, passes, and + executes none of its steps — the same failure mode as an un-awaited `Assert.That`. +- **Given/When/Then replaces Arrange/Act/Assert, and only here.** The chain already names the three + phases. Everywhere else the AAA comments stay. +- **`Context` is the only state.** Steps take it through the constructor and hold no fields of their + own; BDTest gives each test its own context, which is what keeps parallel and repeated runs honest. +- **`[StepText]` is written for the report's reader.** `{0}` is replaced with the argument's actual + value at run time, so the report states what the scenario did, not what it was called. +- **Domain content still comes from `TestBuilder`.** The vocabulary changes at the step; the test-data + rule does not. + +Assertions inside a step are ordinary awaited TUnit assertions. `BDTest.Reporting` produces the +single-file HTML report — publish it as a build artifact from the job that ran the suite. + +## A test that needs a real dependency: `Purview.Containers` + +A database, queue, blob store or emulator comes from `Purview.Containers`: a throwaway Linux container +behind one API over Microsoft WSL Containers or Docker, with typed modules for SQL Server, PostgreSQL, +Redis, MySQL, RabbitMQ, Azurite and NATS. + +The two refused alternatives: + +- **A shared instance** (a local SQL Server, a team database) makes tests order-dependent and failures + unreproducible — the state a test found was left by whatever ran before it, on whatever machine. +- **Testcontainers directly** ties the suite to a Docker installation, which on Windows is an install + and a licensing question per developer and per runner. The WSL backend removes it, and the same test + code runs on either. + +```csharp +await using var postgres = new PostgreSqlBuilder() + .WithDatabase("tests") + .WithUsername("postgres") + .WithPassword("postgres") + .Build(); + +await postgres.StartAsync(); + +await using var connection = new NpgsqlConnection(postgres.GetConnectionString()); +await connection.OpenAsync(); +``` + +`Build()` validates the configuration, `StartAsync()` returns only once every wait strategy is +satisfied — there is no sleep to tune — and `await using` disposes the container at end of scope. + +Three rules: + +- **Its own `.IntegrationTests` project**, so the tier is in the name and the suite stays out + of the pull-request filter without anybody remembering an attribute. +- **Skip locally when no backend is usable; fail in CI.** Two different situations, so never one + outcome. Locally, probe and skip. In CI, pin the backend with `PURVIEW_CONTAINERS_BACKEND` — a named + backend never falls back, so a missing runtime fails loudly instead of letting a green job mean + nothing ran: + + ```csharp + [Before(Class)] + public static async Task EnsureABackendIsAvailable() + { + var backends = await ContainerBackends.ProbeAllAsync(); + + Skip.Unless(backends.Any(backend => backend.IsUsable), "No container backend is available."); + } + ``` + + Left unpinned, automatic selection probes in priority order and prefers WSL Containers over Docker. +- **Pin the version exactly.** `Purview.Containers` is **experimental and prerelease** — API, defaults + and packaging rules can change between releases. The packages are a family, so one property in + `Directory.Packages.props` governs all of them and a bump cannot be applied to half. diff --git a/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs b/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs index 9de3e23..c462e9d 100644 --- a/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs +++ b/src/tests/Architecture.UnitTests/PolicyDocumentationTests.cs @@ -19,7 +19,16 @@ public sealed partial class PolicyDocumentationTests static string DocumentationPath => Path.Combine(ArchitecturePolicy.RepositoryRoot(), "docs", "wiki", "Project-Responsibilities.md"); - static string Documentation => File.ReadAllText(DocumentationPath); + /// Reads the responsibilities page. + /// Cancels the read. + /// The page's text. + /// + /// A method rather than a property, because a property cannot be async and a synchronous one + /// would hide File.ReadAllText from VSTHRD103 - the analyzer only sees a synchronous + /// call when it sits inside a Task-returning method. See docs/wiki/Testing-And-Test-Data.md. + /// + static async Task ReadDocumentationAsync(CancellationToken cancellationToken) => + await File.ReadAllTextAsync(DocumentationPath, cancellationToken); [GeneratedRegex( @"^\| `(?[A-Za-z]+)` \| (?[a-z]+) \| (?[^|]+) \| (?[^|]+) \|$", @@ -28,11 +37,11 @@ public sealed partial class PolicyDocumentationTests private static partial Regex CatalogueRowPattern(); [Test] - public async Task Documentation_GivenTheCatalogueTable_ListsEveryResponsibility() + public async Task Documentation_GivenTheCatalogueTable_ListsEveryResponsibility(CancellationToken cancellationToken) { // Arrange var documented = CatalogueRowPattern() - .Matches(Documentation) + .Matches(await ReadDocumentationAsync(cancellationToken)) .Select(match => match.Groups["name"].Value) .ToArray(); @@ -49,16 +58,19 @@ .. Policy } [Test] - public async Task Documentation_GivenTheCatalogueTable_DoesNotInventResponsibilities() + public async Task Documentation_GivenTheCatalogueTable_DoesNotInventResponsibilities( + CancellationToken cancellationToken + ) { // Arrange var known = Policy.Responsibilities.Select(responsibility => responsibility.Name).ToArray(); + var text = await ReadDocumentationAsync(cancellationToken); // Act IReadOnlyList invented = [ .. CatalogueRowPattern() - .Matches(Documentation) + .Matches(text) .Select(match => match.Groups["name"].Value) .Where(name => !known.Contains(name, StringComparer.Ordinal)) .Distinct(StringComparer.Ordinal), @@ -74,11 +86,16 @@ await Assert } [Test] - public async Task Documentation_GivenTheCatalogueTable_MatchesThePolicyDependencies() + public async Task Documentation_GivenTheCatalogueTable_MatchesThePolicyDependencies( + CancellationToken cancellationToken + ) { + // Arrange + var text = await ReadDocumentationAsync(cancellationToken); + // Act List mismatches = []; - foreach (Match row in CatalogueRowPattern().Matches(Documentation)) + foreach (Match row in CatalogueRowPattern().Matches(text)) { var name = row.Groups["name"].Value; var responsibility = Policy.Responsibilities.FirstOrDefault(candidate => @@ -123,10 +140,10 @@ await Assert } [Test] - public async Task Documentation_GivenTheDiscouragedNames_ListsEveryOne() + public async Task Documentation_GivenTheDiscouragedNames_ListsEveryOne(CancellationToken cancellationToken) { // Arrange - var text = Documentation; + var text = await ReadDocumentationAsync(cancellationToken); // Act IReadOnlyList missing = @@ -144,10 +161,10 @@ await Assert } [Test] - public async Task Documentation_GivenEveryWaiver_ExplainsIt() + public async Task Documentation_GivenEveryWaiver_ExplainsIt(CancellationToken cancellationToken) { // Arrange - var text = Documentation; + var text = await ReadDocumentationAsync(cancellationToken); // Act // A waiver that nobody wrote down is indistinguishable from a rule that was never enforced. diff --git a/src/tests/Architecture.UnitTests/PolicySchemaTests.cs b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs index 76755ea..ba94304 100644 --- a/src/tests/Architecture.UnitTests/PolicySchemaTests.cs +++ b/src/tests/Architecture.UnitTests/PolicySchemaTests.cs @@ -46,14 +46,14 @@ public async Task Policy_GivenTheSchema_Validates() } [Test] - public async Task Schema_GivenADocumentMissingARequiredField_RejectsIt() + public async Task Schema_GivenADocumentMissingARequiredField_RejectsIt(CancellationToken cancellationToken) { // The schema must be able to fail, or "the policy validates" says nothing. - await AssertRejectedAsync("""{ "version": 1 }"""); + await AssertRejectedAsync("""{ "version": 1 }""", cancellationToken); } [Test] - public async Task Schema_GivenAWaiverWithATokenReason_RejectsIt() + public async Task Schema_GivenAWaiverWithATokenReason_RejectsIt(CancellationToken cancellationToken) { // The minLength on 'reason' is the rule that stops a waiver becoming a silent suppression. await AssertRejectedAsync( @@ -71,12 +71,13 @@ await AssertRejectedAsync( ], "waivers": [ { "project": "Infrastructure", "rules": ["naming"], "reason": "because" } ] } - """ + """, + cancellationToken ); } [Test] - public async Task Schema_GivenAnUnknownResponsibilityField_RejectsIt() + public async Task Schema_GivenAnUnknownResponsibilityField_RejectsIt(CancellationToken cancellationToken) { // additionalProperties:false is what catches a typo'd key, which would otherwise be read as an // absent rule - the most dangerous kind of mistake in a policy file. @@ -95,12 +96,13 @@ await AssertRejectedAsync( } ] } - """ + """, + cancellationToken ); } [Test] - public async Task Schema_GivenTheRemovedRulesObject_RejectsIt() + public async Task Schema_GivenTheRemovedRulesObject_RejectsIt(CancellationToken cancellationToken) { // Version 2 removed 'rules': its flags were deserialised but never consulted, so accepting the // block again would restore a switch that looks enforced and is not. @@ -119,19 +121,21 @@ await AssertRejectedAsync( ], "rules": { "noCycles": true } } - """ + """, + cancellationToken ); } /// Writes a candidate policy, validates it, and asserts the schema rejected it. /// The malformed policy. + /// Cancels the write. /// A task that completes when the assertion has run. - static async Task AssertRejectedAsync(string json) + static async Task AssertRejectedAsync(string json, CancellationToken cancellationToken) { var path = Path.Combine(Path.GetTempPath(), $"architecture-policy-{Guid.NewGuid():N}.json"); try { - await File.WriteAllTextAsync(path, json); + await File.WriteAllTextAsync(path, json, cancellationToken); var failures = PolicySchemaValidator.Validate(path); diff --git a/templates/AGENTS.md b/templates/AGENTS.md index 93594cd..4fc3215 100644 --- a/templates/AGENTS.md +++ b/templates/AGENTS.md @@ -54,17 +54,35 @@ This repository contains `Acme.`, .AcceptanceTests`, where Given/When/Then replaces the AAA comments, and the + chain must end in `.BDTestAsync()` — a chain that stops at `.Then(...)` passes without running a step. - **Containers come from `Purview.Containers`, not `Testcontainers` directly.** One API over Microsoft WSL Containers (no Docker installation on Windows) or Docker, with typed modules for SQL Server, PostgreSQL, Redis, MySQL, RabbitMQ, Azurite and NATS. It is experimental and prerelease, so pin the version you adopt. - Put the suite in its own `*.IntegrationTests` project so the tier comes from the name, and make it skip - itself when no backend is reachable — a missing runtime must be a skip, never a pass. + Put the suite in its own `*.IntegrationTests` project so the tier comes from the name. Locally, probe + with `ContainerBackends.ProbeAllAsync()` and skip when nothing is usable — a missing runtime must be a + skip, never a pass. In CI, pin the backend with `PURVIEW_CONTAINERS_BACKEND` so an unavailable runtime + fails the job instead of skipping every test and reporting green. - **Package versions are central, and a family's version is a property.** All versions live in `Directory.Packages.props`. A version shared by a family — `TUnit`/`TUnit.Core`/`TUnit.Mocks`, an EF Core provider against EF Core, any `Purview.*` product released as a set — is declared once in a labelled `PropertyGroup` and referenced as `Version="$(TUnitVersion)"`, so a bump cannot be applied to half the family. A package with no family keeps its literal version inline. -- **Pass a `CancellationToken`** wherever a downstream method accepts one. +- **In a test, prefer the async overload wherever the API offers one** — `File.ReadAllTextAsync`, not + `File.ReadAllText` — **and that reaches through test helpers.** A helper calling such an API is converted + to `async Task`, gains the `Async` suffix and a trailing `CancellationToken`; a property that hides IO + becomes a method, because a property cannot be `async`. `VSTHRD103` fails the build for a sync call + inside a `Task`-returning method, but a sync helper is not `Task`-returning, so the analyzer cannot see + it — that is the case a reviewer has to catch. Exempt only when no async overload exists + (`File.Exists`, `File.Delete`, `Directory.EnumerateFiles`) or a base type fixes the signature; say which. + `Task.Run`, `Task.FromResult` and `.GetAwaiter().GetResult()` are not conversions. +- **Pass a `CancellationToken`** wherever a downstream method accepts one. In a test that means: if any + method in the call chain accepts one, the test method's **last parameter** is a `CancellationToken` — + TUnit supplies it, and with `[Arguments]` the data binds to the leading parameters — and it is forwarded + to every call that accepts one. Never `CancellationToken.None`, never `default`, never a token the test + creates. One hop that drops it makes the whole chain uncancellable. - **The build is the authority.** A convention the build checks is checked by the build; a convention only a reviewer can check is stated as a review obligation and never described as enforced. - **A clean build has no warnings.** Fix it, or suppress it where it occurs with a stated justification. diff --git a/templates/Directory.Packages.props b/templates/Directory.Packages.props index fbb2cf8..2929160 100644 --- a/templates/Directory.Packages.props +++ b/templates/Directory.Packages.props @@ -24,8 +24,9 @@ block. Delete the groups you do not need: the ones below are illustrative, not a required set. Check these versions against the SDK's current release before you commit. They are the versions - this playbook builds with today, plus one illustrative package family (Purview.Containers) that - you delete if no suite needs a container. + this playbook builds with today, plus two illustrative package families — Purview.Containers, which + you delete if no suite needs a container, and BDTest, which you delete if no suite is + acceptance-based. --> true @@ -51,6 +52,12 @@ --> 1.0.0-prerelease.6 1.75.0 + + 3.0.0 @@ -87,5 +94,11 @@ + + +