diff --git a/docs/acceptance-test-review-plan.md b/docs/acceptance-test-review-plan.md index 22ec0ab117..fae3d0dcb0 100644 --- a/docs/acceptance-test-review-plan.md +++ b/docs/acceptance-test-review-plan.md @@ -1,146 +1,111 @@ # Primary instance acceptance test review -A review of the primary instance's acceptance tests for assertion correctness, prioritised by what -ServicePulse actually calls. +A review of the primary instance's acceptance tests for assertion correctness, prioritised by what ServicePulse actually calls. ## Why this is worth doing -Two tests in this suite were recently found to be testing nothing at all. Both registered a test -double with `AddSingleton()` while the code under test injects -`IEnumerable`, so the double was never resolved and never ran. Both -compiled, both passed, and both had passed for years. +Two tests in this suite were recently found to be testing nothing at all. Both registered a test double with `AddSingleton()` while the code under test injects `IEnumerable`, so the double was never resolved and never ran. Both compiled, both passed, and both had passed for years. -That is the shape of the problem: these failures are silent. A test that asserts nothing and a test -that asserts something unfalsifiable both look identical to CI. The purpose of this review is to find -the rest of them before the EF persistence work starts leaning on this suite as its safety net. +That is the shape of the problem: these failures are silent. A test that asserts nothing and a test that asserts something unfalsifiable both look identical to CI. The purpose of this review is to find the rest of them before the EF persistence work starts leaning on this suite as its safety net. + +## Where this has got to + +Phases 1 and 2 are done. Phase 3 has swept the first of its four areas, `Recoverability/MessageFailures`. Phase 4 has closed the licensing block, 8 routes of the 40, on both of the branches those routes take. + +What is left: three areas to sweep, of which the two `ExternalIntegration` ones are parked until the EF work lands, and 32 routes to cover. ## Scope -In scope: the functional areas of `ServiceControl.AcceptanceTests`, being `Recoverability`, -`Monitoring`, `EventLogs` and `WebApi`. That is 71 `When_*.cs` files. +In scope: the functional areas of `ServiceControl.AcceptanceTests`, being `Recoverability`, `Monitoring`, `EventLogs` and `WebApi`. That is 71 `When_*.cs` files. -Out of scope: `Security/*` (25 files across ForwardedHeaders, OpenIdConnect, Cors and Https), which -carries a different risk model and deserves its own pass. Also out of scope: the audit instance, -monitoring instance, and multi-instance suites, except where they already cover a route the primary -suite misses. +Out of scope: `Security/*` (25 files across ForwardedHeaders, OpenIdConnect, Cors and Https), which carries a different risk model and deserves its own pass. Also out of scope: the audit instance, monitoring instance, and multi-instance suites, except where they already cover a route the primary suite misses. Three artefacts anchor the work: -- `src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories 77 - routes. It is **not** the whole primary-instance surface: the approval test behind it scans only - `typeof(Program).Assembly` and `typeof(MyRoutesController).Assembly`. -- `src/Particular.LicensingComponent.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` - inventories the other 8, served under the `api/licensing` prefix, via a separate approval test - scanning `typeof(ThroughputCollector).Assembly`. Anything reasoning about "the API surface" has to - read both files or it will silently miss the throughput and licensing endpoints. -- `src/composables/apiRoutes.ts` in ServicePulse maps each gated UI capability to the route behind - it and names the ServiceControl controller for each. It describes itself as the only place - coupling ServicePulse to ServiceControl's route surface. +- `src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories 77 routes. It is **not** the whole primary-instance surface: the approval test behind it scans only `typeof(Program).Assembly` and `typeof(MyRoutesController).Assembly`. +- `src/Particular.LicensingComponent.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt` inventories the other 8, served under the `api/licensing` prefix, via a separate approval test scanning `typeof(ThroughputCollector).Assembly`. Anything reasoning about "the API surface" has to read both files or it will silently miss the throughput and licensing endpoints. +- `src/composables/apiRoutes.ts` in ServicePulse maps each gated UI capability to the route behind it and names the ServiceControl controller for each. It describes itself as the only place coupling ServicePulse to ServiceControl's route surface. That makes 85 routes in total. ## Defect patterns -Each of these was found in the current suite. The value of naming them is that each becomes a -repeatable check to run over all 71 files, rather than a one-off fix. +Each of these was found in the current suite. The value of naming them is that each becomes a repeatable check to run over all 71 files, rather than a one-off fix. ### 1. Test double registered against the wrong service type (silent) -The double is registered as its concrete type, so the collection the production code injects never -contains it. The test still passes, having exercised none of the behaviour it names. Found twice: -`CounterEnricher` in `When_errors_with_same_uniqueid_are_imported` and `FailOnceEnricher` in -`When_single_message_fails_in_batch`. Both are fixed; the pattern is not. +The double is registered as its concrete type, so the collection the production code injects never contains it. The test still passes, having exercised none of the behaviour it names. Found three times: `CounterEnricher` in `When_errors_with_same_uniqueid_are_imported`, `FailOnceEnricher` in `When_single_message_fails_in_batch`, and `CriticalErrorCustomCheck` in `When_a_critical_error_is_triggered`, which phase 2 turned up. All three are fixed. -**Detect:** for every `AddSingleton()` in a test, assert T is resolved by the component under -test. A convention test over the test assembly can do this: resolve the host and assert each -registered double appears in the collection its interface feeds. +**Detect:** read how the collaborator takes its dependency before registering anything, since a service resolved as a collection is added to rather than replaced. Phase 2 audited all seven registrations in the suite and settled on writing the practice down in [Writing acceptance tests](writing-acceptance-tests.md) rather than building a check, because the fault is a test that passes without its own setup taking effect and a convention test would catch one shape of that. ### 2. No assertion, failure surfaces only as a timeout (diagnostics) -Seven of the 71 files contain no `Assert` at all. Five use the `Do("step", …)` sequence helper, which -logs `Advancing from X to Y` on each transition, so a regression there is diagnosable from the -console output. That is a deliberate and acceptable pattern. The remaining two gate on a single -`Done` predicate and report a regression as a bare 90-second timeout with nothing to read. +Seven files contain no `Assert` at all. Five use the `Do("step", …)` sequence helper, which logs `Advancing from X to Y` on each transition, so a regression there is diagnosable from the console output. That is a deliberate and acceptable pattern. The other two gated on a single `Done` predicate and reported a regression as a bare timeout with nothing to read. -**Detect:** files matching `When_*.cs` with zero `Assert.` occurrences, minus those using the -`Sequence` helper. Currently 2. +**Detect:** files with a `[Test]` and zero `Assert.` occurrences, minus those using the `Sequence` helper. Seven have no assertion, five of them legitimately. `ErrorImportPerformanceTests` is fixed: it records the count on the context, which the runner prints when a scenario does not finish. That leaves `When_single_message_fails_in_batch`, in the area phase 3 sweeps next. ### 3. Assertion restates the condition the scenario already waited on (unfalsifiable) -`When_a_invalid_id_is_sent_to_retry` ends with `Assert.That(context.Done, Is.True)` after -`.Done(ctx => ctx.Done)`. The assertion cannot fail: if the flag were false the scenario would have -timed out first. The real subject of that test, that posting a retry for a non-existent id does not -break subsequent batches, is never asserted, and the response of the invalid POST is never -inspected. +`When_a_invalid_id_is_sent_to_retry` ended with `Assert.That(context.Done, Is.True)` after `.Done(ctx => ctx.Done)`. That assertion could not fail: if the flag were false the scenario would have timed out first. The real subject of the test, that posting a retry for a non-existent id does not break subsequent batches, was never asserted, and the response of the invalid POST was never inspected. -**Detect:** a mechanical grep found 8 candidates of the shape `Assert.That(context.Flag, Is.True)`. -One is confirmed tautological; the rest need reading individually, because a flag set by a message -handler and gated on something else is legitimate. +**Detect:** a mechanical grep finds candidates of the shape `Assert.That(context.Flag, Is.True)`, currently 9. The confirmed one is fixed and now asserts the status the unknown id answers with. The rest need reading individually, because a flag set by a message handler and gated on something else is legitimate: `When_failed_message_searched_by_body_content` was read and is sound, since its `Done` returns true whether or not the flag is set. Four of the nine are in `ExternalIntegration`, which is parked. ### 4. Assertions coupled to one persister's internals (portability) -Tests that assert RavenDB implementation details rather than the contract both persisters offer. The -multi-attempt tests asserted Raven's ten-attempt trimming and its full attempt history, neither of -which EF provides by design. These read as EF gaps when they are really over-specified tests, and -they are the main reason the EF exclusion list looks longer than the real feature gap. +Tests that assert RavenDB implementation details rather than the contract both persisters offer. The multi-attempt tests asserted Raven's ten-attempt trimming and its full attempt history, neither of which EF provides by design. These read as EF gaps when they are really over-specified tests, and they are the main reason the EF exclusion list looks longer than the real feature gap. -**Detect:** the `` blocks in the two EF acceptance csprojs are the existing -inventory. Each entry is either a real gap, a settled design difference, or an over-specified test, -and the comments do not currently distinguish them. +**Detect:** the `` blocks in the two EF acceptance csprojs are the existing inventory. Each entry is either a real gap, a settled design difference, or an over-specified test, and the comments do not currently distinguish them. ### 5. Setup computed but never asserted (rot) -Fixtures, headers and constants that exist to support an assertion that was removed or never -written: a `Counter` header recorded into a dictionary nothing reads, a -`MaximalNumberOfStoredFailedAttempts` constant, a list of failure times trimmed to a window that is -then discarded. Harmless on its own, but it makes tests read as though they cover more than they do, -which is how the first two patterns survive review. +Fixtures, headers, constants and context properties that exist to support an assertion that was removed or never written. `Recoverability/MessageFailures` held eight: a `Retried` flag written by a handler and read by nothing in four tests whose subject is that a retry happened, two `FromAddress` and two `LocalAddress` captures, each with a constructor parameter that existed only to feed them. Thirteen `Console.WriteLine` calls in eleven files were the same thing in another form, printing either nothing identifying or what the neighbouring assertion message already says. + +Harmless on its own, but it makes tests read as though they cover more than they do, which is how the first two patterns survive review. -**Detect:** per-file reading. Context properties with a setter and no read outside the scenario are -the strongest signal. +**Detect:** per-file reading. Context properties with a setter and no read outside the scenario are the strongest signal. ## The routes that need tests -40 of the 85 routes are called by no acceptance test in any suite. This is the whole list, confirmed -route by route against the two approved route lists and the test sources, and it is the work rather -than a sample of it. Ticking every box here is what closes phase 4. +40 of the 85 routes were called by no acceptance test in any suite when this list was confirmed, route by route, against the two approved route lists and the test sources. The licensing block has since been covered, leaving 32. This is the whole list rather than a sample of it, and ticking every box here is what closes phase 4. -The groups are sized to be one PR each and ordered by what breaks in ServicePulse if the route -regresses. Each entry names the ServicePulse consumer where there is one, because that is what the -test should assert: the contract the UI relies on, not a 200. +The groups are sized to be one PR each and ordered by what breaks in ServicePulse if the route regresses. Each entry names the ServicePulse consumer where there is one, because that is what the test should assert: the contract the UI relies on, not a 200. ### Nav-gating routes -If one of these regresses, ServicePulse loses a whole section of its navigation and nothing in the -suite notices. +If one of these regresses, ServicePulse loses a whole section of its navigation and nothing in the suite notices. - [ ] `GET /api/heartbeats/stats`: gates the Heartbeats nav item (`viewHeartbeats`) - [ ] `GET /api/messages2`: gates the audit messages nav item (`viewAuditMessages`) - [ ] `GET /api/license`: gates the Licence nav item (`viewLicense`) -`GET /api/licensing/report/available` gates the Throughput nav item and belongs to this tier too, -but it is written with the rest of the licensing block below. `GET /api/connection` gates the -Connections nav item and is covered in MultiInstance only, so it is a home decision rather than a -new test. - -### Licensing and throughput - -All eight `api/licensing` routes, one PR. The largest single win: the throughput *domain logic* -beneath them is the best-unit-tested area in the codebase, with a dedicated -`Particular.LicensingComponent.UnitTests` project carrying report generation, masking, date handling -and summary indicator tests plus approval files. None of it touches `LicensingController`, so the -HTTP surface, its serialisation and its authorisation are untested end to end. Check first whether -the acceptance test host even starts the licensing component, as that decides whether this is a new -test file or a new test project. - -- [ ] `GET /api/licensing/report/available`: gates the Throughput nav item (`viewThroughput`) -- [ ] `POST /api/licensing/settings/masks/update`: `manageThroughput` -- [ ] `GET /api/licensing/settings/masks`: throughput masking settings -- [ ] `GET /api/licensing/settings/test`: connection test on the throughput settings page -- [ ] `GET /api/licensing/settings/info`: throughput settings summary -- [ ] `GET /api/licensing/report/file`: downloads the throughput report -- [ ] `GET /api/licensing/endpoints`: endpoint throughput list -- [ ] `POST /api/licensing/endpoints/update`: saves per-endpoint user indicators +`GET /api/licensing/report/available` gates the Throughput nav item and belongs to this tier too, but it is written with the rest of the licensing block below. `GET /api/connection` gates the Connections nav item and is covered in MultiInstance only, so it is a home decision rather than a new test. + +### Licensing and throughput (done) + +All eight `api/licensing` routes, covered by one scenario: `ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport`. The open question about whether this needed its own test project is answered: `LicensingComponent` is in `ServiceControlMainInstance.Components`, so the acceptance host already starts it, and both the Raven and EF persisters register an `ILicensingDataStore`, so the test needs no exclusions. + +The scenario arranges throughput the way it really arrives, by dispatching the message a monitoring instance sends to the throughput queue, then walks the ServicePulse throughput page: check a report is possible, review where the numbers come from, correct the queue that is not an NServiceBus endpoint, redact the customer name, download the report, and assert the redaction and the correction both survive into the file that gets sent to Particular. + +- [x] `GET /api/licensing/report/available`: gates the Throughput nav item (`viewThroughput`) +- [x] `POST /api/licensing/settings/masks/update`: `manageThroughput` +- [x] `GET /api/licensing/settings/masks`: throughput masking settings +- [x] `GET /api/licensing/settings/test`: connection test on the throughput settings page +- [x] `GET /api/licensing/settings/info`: throughput settings summary +- [x] `GET /api/licensing/report/file`: downloads the throughput report +- [x] `GET /api/licensing/endpoints`: endpoint throughput list +- [x] `POST /api/licensing/endpoints/update`: saves per-endpoint user indicators + +Writing it surfaced a domain rule no route-level test would have reached: a report counts only complete days, so throughput recorded for today does not make one available. The first version of the scenario reported today's numbers and sat on a 90-second timeout. That rule is now stated in the test's arrangement. + +Each route is covered on both of its branches, by one scenario each. The suite runs on LearningTransport, which registers no `IBrokerThroughputQuery`, so `ThroughputCollector` receives null for it and takes the branch only Learning and MSMQ take. RabbitMQ, Azure Service Bus, SQS, SQL Server and PostgreSQL all register one, and on that branch `report/available` requires broker-sourced throughput rather than any throughput, `settings/test` runs a broker connection test, `settings/info` returns the broker's settings, and the report carries a `ReportMethod` of `Broker` rather than `ServiceControl`. The test names say which branch each one covers. + +- [x] Cover the broker path for the eight `api/licensing` routes + +`When_creating_a_usage_report_on_a_broker_transport` covers the branch every production install except MSMQ takes. It registers a fake `IBrokerThroughputQuery` through `CustomizeHostBuilderBeforeServiceControl`, because `AddLicensingComponent` decides whether to start `BrokerThroughputCollectorHostedService` by checking whether a query is registered, and it decides that while `AddServiceControl` runs. It also replaces the collector registration with one whose `DelayStart` is zero, since the production value of 40 seconds is longer than a scenario should take. + +Beyond the branch itself it asserts the grouping: a broker queue and a monitored endpoint whose names differ only by the sanitized character have to end up as one endpoint in the report, or a customer's usage is counted twice. Making the fake's `SanitizeEndpointName` an identity function fails that assertion, so it is doing real work. + +Getting the two scenarios to pass together turned up a fault in the suite that had nothing to do with them. `RavenPersisterSettings.ThroughputDatabaseName` defaults to a fixed name, and the Raven acceptance storage configuration set only `DatabaseName`, so throughput and licensing data from every acceptance test shared one database while everything else was isolated per test. The persistence tests already set it per test; the acceptance ones now do the same, and clean it up. Two things made this hard to see: the symptom looked like queue interference, because the two tests' endpoints differ only by the character the broker sanitizes and so merged into one grouped endpoint, and the `MonitoringService` bug fixed separately was losing endpoints at the same time. The EF suites were never affected, since throughput lives in their per-test database. ### Notifications @@ -168,9 +133,7 @@ test file or a new test project. - [ ] `POST /api/recoverability/groups/{id}/comment`: group comments - [ ] `DELETE /api/recoverability/groups/{id}/comment`: group comments - [ ] `GET /api/recoverability/groups/id/{groupId}`: single group. The archive twin, `GET /api/archive/groups/id/{groupId}`, is covered -- [ ] `HEAD /api/recoverability/groups/{id}/errors`: the count behind group paging - [ ] `GET /api/endpoints/{name}/errors`: failed messages filtered to one endpoint -- [ ] `HEAD /api/errors`: the count behind failed-message paging ### Retry and resolve routes @@ -180,8 +143,7 @@ test file or a new test project. ### Audit-backed message queries -These need an audit instance, so decide whether MultiInstance is the right home before adding them -to the primary suite. +These need an audit instance, so decide whether MultiInstance is the right home before adding them to the primary suite. - [ ] `GET /api/messages/search`: the `?q=` form. The `/search/{keyword}` form is covered - [ ] `GET /api/endpoints/{endpoint}/messages/search`: the same, scoped to an endpoint @@ -195,10 +157,18 @@ to the primary suite. - [ ] `GET /api/license/details` - [ ] `POST /api/license/detailsUpload` +### Deliberately untested + +No ServicePulse journey reaches these, and ServicePulse issues no HEAD request at all, so no scenario in this plan will cover them. Recorded here so that they stay a decision rather than an oversight: if a consumer for them turns up, they need tests. + +- `HEAD /api/errors`: the count behind failed-message paging +- `HEAD /api/recoverability/groups/{id}/errors`: the count behind group paging + +`HEAD /api/redirect` is the exception and is already covered, by `When_a_request_is_repeated_with_its_etag`. + ### Covered only in MultiInstance -Not gaps, but not in the primary suite either. Decide deliberately whether MultiInstance is the -right home before duplicating any of them. +Not gaps, but not in the primary suite either. Decide deliberately whether MultiInstance is the right home before duplicating any of them. - [ ] `GET /api/connection`: gates the Connections nav item (`viewConnections`) - [ ] `GET /api/endpoints/known`: known endpoints list, covered by `When_endpoint_known_to_audit_instance` @@ -207,93 +177,63 @@ right home before duplicating any of them. - [ ] `GET /api/endpoints/{endpoint}/messages` - [ ] `GET /api/endpoints/{endpoint}/messages/search/{keyword}` -Worth being precise about the heartbeats entry, because several others are the same shape. Heartbeat -*ingestion* is well covered: six tests drive endpoints starting up, going quiet, and being marked -monitored. What no test calls is `/api/heartbeats/stats`, the endpoint ServicePulse's heartbeats page -actually reads. The plumbing is tested; the contract on top of it is not. +Worth being precise about the heartbeats entry, because several others are the same shape. Heartbeat *ingestion* is well covered: six tests drive endpoints starting up, going quiet, and being marked monitored. What no test calls is `/api/heartbeats/stats`, the endpoint ServicePulse's heartbeats page actually reads. The plumbing is tested; the contract on top of it is not. ## The work ### Phase 1: confirm the gap list properly (done) -The original table was grep-based, and a route reached through a helper, a constant or an -interpolated base path would have been missed. The list above replaces it, built from both approved -route lists rather than from greps and checked route by route against the primary and MultiInstance -sources. Reading only the ServiceControl route list is what hid the licensing routes in the first -place, so both were read. +The original table was grep-based, and a route reached through a helper, a constant or an interpolated base path would have been missed. The list above replaces it, built from both approved route lists rather than from greps and checked route by route against the primary and MultiInstance sources. Reading only the ServiceControl route list is what hid the licensing routes in the first place, so both were read. -Two details mattered while confirming it. The approved lists carry the action-level template only, -so they hold two rows both reading `GET /configuration`, one on `RootController` under `api` and one -on `AuthenticationController` under `api/authentication`; the controller-level `[Route]` has to be -folded in or the two merge. And the verb has to be tracked separately from the path, because -ServicePulse gates `GET` and `POST` on `/api/notifications/email` as two different capabilities. +Two details mattered while confirming it. The approved lists carry the action-level template only, so they hold two rows both reading `GET /configuration`, one on `RootController` under `api` and one on `AuthenticationController` under `api/authentication`; the controller-level `[Route]` has to be folded in or the two merge. And the verb has to be tracked separately from the path, because ServicePulse gates `GET` and `POST` on `/api/notifications/email` as two different capabilities. -Six entries changed as a result. `GET /api/endpoints/known` is not a gap: MultiInstance covers it. -The other five were gaps the grep missed, and four of them gate a ServicePulse capability: -`GET /api/license` and `GET /api/connection` gate nav items, and `DELETE /api/customchecks/{id}`, -`PATCH/POST /api/errors/archive` and `POST /api/recoverability/groups/{id}/errors/unarchive` are -actions the UI offers. +Six entries changed as a result. `GET /api/endpoints/known` is not a gap: MultiInstance covers it. The other five were gaps the grep missed, and four of them gate a ServicePulse capability: `GET /api/license` and `GET /api/connection` gate nav items, and `DELETE /api/customchecks/{id}`, `PATCH/POST /api/errors/archive` and `POST /api/recoverability/groups/{id}/errors/unarchive` are actions the UI offers. -No tooling came out of this phase, deliberately. A test that scans the suite's source to police its -own coverage is a second thing to maintain and gets stale in its own way; the list above is the -artefact, and new routes are a review-time concern. +No tooling came out of this phase, deliberately. A test that scans the suite's source to police its own coverage is a second thing to maintain and gets stale in its own way; the list above is the artefact, and new routes are a review-time concern. ### Phase 2: the silent-registration class (done) -The whole suite registers services from a test in seven places, so the audit was exhaustive rather -than a sample. Five were correct. Two were the known `IEnrichImportedErrorMessages` cases, already -fixed. One was new: - -`When_a_critical_error_is_triggered` registered `CriticalErrorCustomCheck` as its own concrete type, -with a comment saying it overrode the production registration to shorten the check interval. It did -not. The check is registered with `TryAddEnumerable` against `ICustomCheck` and consumed through -`GetServices()`, so the test's registration was never resolved and the check ran on -its 60-second production interval. The test passed either way, about a minute slower than intended. -It now removes the production registration explicitly and re-adds the check against `ICustomCheck`, -and runs in six seconds. - -One registration is worth knowing about even though it is correct. `When_a_retry_fails_to_be_sent` -substitutes a `FakeReturnToSender` by re-registering `ReturnToSender`, which works only because -`CustomizeHostBuilder` runs after all production registration and `ReturnToSenderDequeuer` resolves -a single instance rather than a collection. That is a real distinction, not a detail: the same move -against a collection adds a second implementation and leaves the production one running. - -No harness check came out of this phase. The underlying fault is a test written so that it could -pass without its own setup taking effect, and a convention test policing registrations would catch -one shape of that while leaving the rest. The practice is written down instead, in -[Writing acceptance tests](writing-acceptance-tests.md), which covers registering against the -injected abstraction, replacing a production registration so that it fails loudly if production -moves, and asserting on evidence the double actually ran. +The whole suite registers services from a test in seven places, so the audit was exhaustive rather than a sample. Five were correct. Two were the known `IEnrichImportedErrorMessages` cases, already fixed. One was new: + +`When_a_critical_error_is_triggered` registered `CriticalErrorCustomCheck` as its own concrete type, with a comment saying it overrode the production registration to shorten the check interval. It did not. The check is registered with `TryAddEnumerable` against `ICustomCheck` and consumed through `GetServices()`, so the test's registration was never resolved and the check ran on its 60-second production interval. The test passed either way, about a minute slower than intended. It now removes the production registration explicitly and re-adds the check against `ICustomCheck`, and runs in six seconds. + +One registration is worth knowing about even though it is correct. `When_a_retry_fails_to_be_sent` substitutes a `FakeReturnToSender` by re-registering `ReturnToSender`, which works only because `CustomizeHostBuilder` runs after all production registration and `ReturnToSenderDequeuer` resolves a single instance rather than a collection. That is a real distinction, not a detail: the same move against a collection adds a second implementation and leaves the production one running. + +No harness check came out of this phase. The underlying fault is a test written so that it could pass without its own setup taking effect, and a convention test policing registrations would catch one shape of that while leaving the rest. The practice is written down instead, in [Writing acceptance tests](writing-acceptance-tests.md), which covers registering against the injected abstraction, replacing a production registration so that it fails loudly if production moves, and asserting on evidence the double actually ran. ### Phase 3: sweep the 71 files, one area per PR -Read for the five patterns above, area by area, so each PR stays reviewable and the diff maps to one -owner's mental model. Fix what is cheap to fix in the same PR; raise anything that changes what a -test means as its own change with the reasoning written down. +Read for the five patterns above, area by area, so each PR stays reviewable and the diff maps to one owner's mental model. Fix what is cheap to fix in the same PR; raise anything that changes what a test means as its own change with the reasoning written down. + +- [x] `Recoverability/MessageFailures`, 22 files, the densest area and the one the EF work touches most. + + One unfalsifiable assertion, `When_a_invalid_id_is_sent_to_retry`, which asserted the flag its own `Done` predicate had already waited for. It now asserts what the test is named for: retrying an id that does not exist answers `202 Accepted` rather than rejecting, and the scenario still completes, so the batch behind it kept moving. The retry loop it used to sit behind is gone. It was there to wait for an API that is never not ready: the instance is started while the component runner is created, before any endpoint starts, and ServiceControl has no code path that answers 503. The loop also caught every non-success alike, so a genuine rejection would have spun rather than failed. + + One test reporting failure as a bare timeout, `ErrorImportPerformanceTests`. The count now goes on the scenario context, which the runner prints when a scenario does not finish, so a failure says how many of the 100 messages arrived. + + Dead setup in six files: `Retried` in four, `FromAddress` in two, `LocalAddress` in two, each written by a handler and read by nothing, along with the `ReceiveAddresses` parameter that only existed to feed them. `Retried` was the misleading one, sitting in tests whose subject is that a retry happened while `RetryCount` did the actual work. + + Nothing found for the persister-coupling pattern: no file in this area is excluded from the EF suites. The two registrations here were already settled in phase 2. + + Three tests moved to the `Do` sequence helper, which is where the area's readability was worst. `When_a_retry_for_a_failed_message_is_successful` held a four-step sequence inside a single `Done` predicate five times over, re-entered on every poll, with a `RetryIssued` guard to stop the retry firing repeatedly. As steps that guard is unnecessary, though the flag itself stays because the handler reads it to decide whether to throw. `When_a_failed_message_is_pending_retry` and `When_a_invalid_id_is_sent_to_retry` had the same shape spread across chained endpoint `When` clauses. A stalled run now names the step it stopped on rather than only the elapsed time. + + Thirteen `Console.WriteLine` calls went, across eleven files. Six were a bare "Message Handled" in a handler, which carries no identity and fires on every delivery, so in tests turning on how many times a message was handled it cannot tell the first attempt from the retry. Worse, they sat next to the counter that does answer that, and the runner already prints the context on failure. The rest either narrated a step that throws with detail when it fails, or dumped state next to an assertion whose message says the same thing. They read as debugging left in place rather than diagnostics anyone chose. -- `Recoverability/MessageFailures`, 22 files, the densest area and the one the EF work touches most. -- `Recoverability/*` root, `Groups`, `MessageRedirects`: the retry, group and redirect flows. -- `Monitoring/*` and `EventLogs`: heartbeats, custom checks, endpoint monitoring. -- `Recoverability/ExternalIntegration` and `Monitoring/ExternalIntegration`: hold until the EF - external-integration work lands, then review against both persisters at once. + Not every test wants this. A test that sends a message, waits for one thing and asserts reads worse as a sequence, which is most of `When_a_message_has_failed`. A step also has no `bus`, so anything that sends has to stay an endpoint `When`. +- [ ] `Recoverability/*` root, `Groups`, `MessageRedirects`: the retry, group and redirect flows. +- [ ] `Monitoring/*` and `EventLogs`: heartbeats, custom checks, endpoint monitoring. +- [ ] `Recoverability/ExternalIntegration` and `Monitoring/ExternalIntegration`: hold until the EF external-integration work lands, then review against both persisters at once. ### Phase 4: work through the route list -Straight down the list in "The routes that need tests", one PR per group, in the order the groups -are given. The phase is done when every box is ticked. +Straight down the list in "The routes that need tests", one PR per group, in the order the groups are given. The phase is done when every box is ticked. -Each test asserts the contract its consumer relies on rather than just a 200: the shape ServicePulse -reads, the status it branches on, the effect the action has on the next request. A test that only -proves the route is routable would leave the same gap in a different form. +Each test asserts the contract its consumer relies on rather than just a 200: the shape ServicePulse reads, the status it branches on, the effect the action has on the next request. A test that only proves the route is routable would leave the same gap in a different form. ## What this plan does not do -It does not keep the route list current by machine. The list is a snapshot taken during phase 1, and -a route added after that will not appear in it on its own. Catching those is a review-time concern: -a new controller action arrives with the PR that adds it, which is where the test for it belongs. +It does not keep the route list current by machine. The list is a snapshot taken during phase 1, and a route added after that will not appear in it on its own. Catching those is a review-time concern: a new controller action arrives with the PR that adds it, which is where the test for it belongs. It does not review the `Security/*` tests, the audit instance, or the monitoring instance. -It does not treat persister parity as a workstream in its own right. It appears only as one defect -pattern, on the grounds that most of the current EF exclusions are over-specified tests rather than -missing features, which is itself a claim worth confirming during phase 3. +It does not treat persister parity as a workstream in its own right. It appears only as one defect pattern, on the grounds that most of the current EF exclusions are over-specified tests rather than missing features, which is itself a claim worth confirming during phase 3. diff --git a/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_broker_transport.cs b/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_broker_transport.cs index 00e4fd37af..931828710b 100644 --- a/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_broker_transport.cs +++ b/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_broker_transport.cs @@ -24,8 +24,6 @@ namespace ServiceControl.AcceptanceTests.Licensing using Particular.LicensingComponent.Shared; using ServiceControl.Transports.BrokerThroughput; - // The same journey as When_creating_a_usage_report_on_a_non_broker_transport, on the branch every - // production install except MSMQ takes. class When_creating_a_usage_report_on_a_broker_transport : AcceptanceTest { [Test] @@ -63,8 +61,6 @@ await Define() }) .Do("List the endpoints reporting throughput", async _ => { - // Monitoring reports the same endpoint the broker measured, so wait for both - // before reading the list, otherwise the grouping below proves nothing. var summary = await this.TryGet>( "/api/licensing/endpoints", items => items.Any(item => item.MaxDailyThroughput == BrokerThroughput)); @@ -99,9 +95,6 @@ await Define() Assert.That(reportData.GetProperty("ReportMethod").GetString(), Is.EqualTo("Broker"), "Particular reads the report method to know how the numbers were measured"); - // The broker calls the queue Contoso/Sales and monitoring calls the endpoint the same - // thing, but the two only line up once the broker's sanitized name is applied to both. - // Without that they are two endpoints, and the customer's report counts them twice. Assert.That(endpoints, Has.Exactly(1).Items, "The broker queue and the monitored endpoint are one endpoint, not two"); @@ -124,8 +117,7 @@ await Define() } } - // The collector waits 40 seconds before its first pass, which is longer than this scenario - // should take, and the delay is only reachable through the registration. + // The collector waits 40 seconds before its first pass, reachable only through the registration. static void CollectFromTheBrokerImmediately(IHostApplicationBuilder builder) { var scheduled = builder.Services.Single(registration => @@ -161,7 +153,6 @@ class Context : ScenarioContext, ISequenceContext public int Step { get; set; } } - // Reports the same endpoint the broker measured, under the name monitoring knows it by. class MonitoringInstance : EndpointConfigurationBuilder { public MonitoringInstance() => diff --git a/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport.cs b/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport.cs index 6a6ae15a55..ec919c799a 100644 --- a/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport.cs +++ b/src/ServiceControl.AcceptanceTests/Licensing/When_creating_a_usage_report_on_a_non_broker_transport.cs @@ -18,9 +18,6 @@ namespace ServiceControl.AcceptanceTests.Licensing using Particular.LicensingComponent.MonitoringThroughput; using Particular.LicensingComponent.Shared; - // The journey ServicePulse's throughput page walks a user through: see whether a report is - // possible, review where the numbers come from, correct what counts as an endpoint, redact the - // names that cannot leave the building, and download the report to send to Particular. class When_creating_a_usage_report_on_a_non_broker_transport : AcceptanceTest { [Test] @@ -135,8 +132,6 @@ class Context : ScenarioContext, ISequenceContext public int Step { get; set; } } - // Stands in for the monitoring instance, which reports what it saw to the primary instance's - // throughput queue. class MonitoringInstance : EndpointConfigurationBuilder { public MonitoringInstance() => @@ -146,8 +141,7 @@ class ReportThroughput : DispatchRawMessages { protected override TransportOperations CreateMessage(Context context) { - // Yesterday: a report only counts complete days, so throughput recorded for today - // is deliberately not enough to generate one. + // Yesterday, because a usage report only counts complete days. var recorded = new RecordEndpointThroughputData { StartDateTime = DateTime.UtcNow.AddDays(-1).AddHours(-1), diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/When_an_unmonitored_endpoint_is_marked_as_monitored.cs b/src/ServiceControl.AcceptanceTests/Monitoring/When_an_unmonitored_endpoint_is_marked_as_monitored.cs index 73f8e53e51..acaa985d80 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/When_an_unmonitored_endpoint_is_marked_as_monitored.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/When_an_unmonitored_endpoint_is_marked_as_monitored.cs @@ -60,7 +60,6 @@ await Define() MonitorHeartbeat = true }); state = State.WaitingForHeartbeatFailure; - Console.WriteLine("Patch successful"); } return false; diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_message_fails_twice_with_different_exceptions.cs b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_message_fails_twice_with_different_exceptions.cs index ca914f59b2..2b7b5e0389 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_message_fails_twice_with_different_exceptions.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_a_message_fails_twice_with_different_exceptions.cs @@ -78,7 +78,6 @@ await Define() foreach (var failureId in originalExceptionAndStackTraceFailureGroupIds) { - Console.WriteLine($"failureId: {failureId}"); Assert.That(retriedExceptionAndStackTraceFailureGroupIds, Does.Not.Contain(failureId), $"Failure Group {failureId} is still set on retried message"); } } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_messages_have_failed.cs b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_messages_have_failed.cs index c523d8111d..1dd436f7fc 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_messages_have_failed.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/Groups/When_messages_have_failed.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; using System.Linq; - using System.Text.Json; using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.EndpointTemplates; @@ -74,7 +73,6 @@ public async Task Should_be_grouped() Assert.That(messageTypeGroups.Count, Is.EqualTo(2), "There should be 2 Message Type Groups"); } - defaultGroups.ForEach(g => Console.WriteLine(JsonSerializer.Serialize(g))); Assert.That(exceptionTypeAndStackTraceGroups.Select(g => g.Id).Except(defaultGroups.Select(g => g.Id)), Is.Empty, "/api/recoverability/groups did not retrieve Exception Type and Stack Trace Group"); diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/ErrorImportPerformanceTests.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/ErrorImportPerformanceTests.cs index a0a56c6336..f91755eb5e 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/ErrorImportPerformanceTests.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/ErrorImportPerformanceTests.cs @@ -19,22 +19,13 @@ class ErrorImportPerformanceTests : AcceptanceTest public async Task Should_import_all_messages(CancellationToken cancellationToken = default) { await Define() - .WithEndpoint(b => b.When(bus => Task.WhenAll(Enumerable.Repeat(0, 100).Select(i => bus.SendLocal(new MyMessage())))).DoNotFailOnErrorMessages()) + .WithEndpoint(b => b.When(bus => Task.WhenAll(Enumerable.Repeat(0, ExpectedMessages).Select(i => bus.SendLocal(new MyMessage())))).DoNotFailOnErrorMessages()) .Done(async c => { var result = await this.TryGetMany("/api/messages?per_page=150"); - if (!result) - { - return false; - } - - List messages = result; - if (messages.Count < 100) - { - Console.Out.WriteLine("Messages found: " + messages.Count); - } - - return messages.Count >= 100; + c.MessagesImported = result ? ((List)result).Count : 0; + + return c.MessagesImported >= ExpectedMessages; }) .Run(cancellationToken); } @@ -53,6 +44,11 @@ public class MyMessageHandler : IHandleMessages public class MyMessage : ICommand; - public class MyContext : ScenarioContext; + const int ExpectedMessages = 100; + + public class MyContext : ScenarioContext + { + public int MessagesImported { get; set; } + } } } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_failed_message_is_pending_retry.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_failed_message_is_pending_retry.cs index 9d1e7ff00f..2bbb0b2f86 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_failed_message_is_pending_retry.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_failed_message_is_pending_retry.cs @@ -23,31 +23,20 @@ public async Task Should_status_retryissued_after_retry_is_sent() FailedMessage failedMessage = null; await Define() - .WithEndpoint(b => b.When(async ctx => - { - if (ctx.UniqueMessageId == null) - { - return false; - } - - var result = await this.TryGet($"/api/errors/{ctx.UniqueMessageId}"); - failedMessage = result; - return result; - }, async (bus, ctx) => + .WithEndpoint(b => b.DoNotFailOnErrorMessages()) + .Do("Wait for the message to fail", async ctx => + ctx.UniqueMessageId != null && await this.TryGet($"/api/errors/{ctx.UniqueMessageId}")) + .Do("Issue a retry", async ctx => { ctx.AboutToSendRetry = true; await this.Post($"/api/errors/{ctx.UniqueMessageId}/retry"); - }).DoNotFailOnErrorMessages()) - .Done(async ctx => + }) + .Do("Wait for the retry to be handled", ctx => Task.FromResult(ctx.Retried)) + .Do("Read the failed message back", async ctx => { - if (ctx.Retried) - { - failedMessage = await this.TryGet($"/api/errors/{ctx.UniqueMessageId}"); - return true; - } - - return false; + failedMessage = await this.TryGet($"/api/errors/{ctx.UniqueMessageId}"); }) + .Done() .Run(); Assert.That(failedMessage.Status, Is.EqualTo(FailedMessageStatus.RetryIssued), "Status was not set to RetryIssued"); @@ -81,7 +70,6 @@ public class MyMessageHandler(Context scenarioContext, IReadOnlySettings setting { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Message Handled"); if (scenarioContext.AboutToSendRetry) { scenarioContext.Retried = true; @@ -97,11 +85,12 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) } } - public class Context : ScenarioContext + public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } public bool Retried { get; set; } public bool AboutToSendRetry { get; set; } + public int Step { get; set; } } public class MyMessage : ICommand; diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_invalid_id_is_sent_to_retry.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_invalid_id_is_sent_to_retry.cs index 630e7b938c..429b9302be 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_invalid_id_is_sent_to_retry.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_invalid_id_is_sent_to_retry.cs @@ -1,6 +1,7 @@ namespace ServiceControl.AcceptanceTests.Recoverability.MessageFailures { using System; + using System.Net; using System.Threading; using System.Threading.Tasks; using AcceptanceTesting; @@ -17,30 +18,27 @@ class When_a_invalid_id_is_sent_to_retry : AcceptanceTest [CancelAfter(180_000)] public async Task SubsequentBatchesShouldBeProcessed(CancellationToken cancellationToken = default) { - var context = await Define() + HttpStatusCode retryOfUnknownId = default; + + await Define() .WithEndpoint(cfg => cfg .When(async bus => { - while (true) - { - try - { - await this.Post("/api/errors/1785201b-5ccd-4705-b14e-f9dd7ef1386e/retry"); - break; - } - catch (InvalidOperationException) - { - // api not up yet - } - } + using var response = await HttpClient.PostAsync($"/api/errors/{UnknownFailedMessageId}/retry", null, cancellationToken); + + retryOfUnknownId = response.StatusCode; await bus.SendLocal(new MessageThatWillFail()); - }).DoNotFailOnErrorMessages() - .When(async ctx => ctx.IssueRetry && await this.TryGet("/api/errors/" + ctx.UniqueMessageId), (bus, ctx) => this.Post($"/api/errors/{ctx.UniqueMessageId}/retry")).DoNotFailOnErrorMessages()) - .Done(ctx => ctx.Done) + }).DoNotFailOnErrorMessages()) + .Do("Wait for the message to fail", async ctx => + ctx.IssueRetry && await this.TryGet($"/api/errors/{ctx.UniqueMessageId}")) + .Do("Retry the failed message", async ctx => + await this.Post($"/api/errors/{ctx.UniqueMessageId}/retry")) + .Do("Wait for the retry to be handled", ctx => Task.FromResult(ctx.Done)) + .Done() .Run(cancellationToken); - Assert.That(context.Done, Is.True); + Assert.That(retryOfUnknownId, Is.EqualTo(HttpStatusCode.Accepted)); } public class FailureEndpoint : EndpointConfigurationBuilder @@ -70,8 +68,9 @@ public Task Handle(MessageThatWillFail message, IMessageHandlerContext context) } } - public class MyContext : ScenarioContext + public class MyContext : ScenarioContext, ISequenceContext { + public int Step { get; set; } public bool Done { get; set; } public bool ExceptionThrown { get; set; } public bool IssueRetry { get; set; } @@ -79,6 +78,8 @@ public class MyContext : ScenarioContext } + const string UnknownFailedMessageId = "1785201b-5ccd-4705-b14e-f9dd7ef1386e"; + public class MessageThatWillFail : ICommand; } } \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_message_has_failed.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_message_has_failed.cs index 1c6bd75b5b..fc8bae05e9 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_message_has_failed.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_message_has_failed.cs @@ -232,14 +232,12 @@ public Receiver() => [Handler] public class MyMessageHandler( MyContext testContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { testContext.EndpointNameOfReceivingEndpoint = settings.EndpointName(); - testContext.LocalAddress = receiveAddresses.MainReceiveAddress; testContext.MessageId = context.MessageId.Replace(@"\", "-"); throw new Exception("Simulated exception"); } @@ -258,14 +256,12 @@ public ReceiverWithCustomSerializer() => [Handler] public class MyMessageHandler( MyContext testContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { testContext.EndpointNameOfReceivingEndpoint = settings.EndpointName(); - testContext.LocalAddress = receiveAddresses.MainReceiveAddress; testContext.MessageId = context.MessageId.Replace(@"\", "-"); throw new Exception("Simulated exception"); } @@ -328,7 +324,6 @@ public class MyContext : ScenarioContext public string MessageId { get; set; } public string EndpointNameOfReceivingEndpoint { get; set; } public string UniqueMessageId => DeterministicGuid.MakeId(MessageId, EndpointNameOfReceivingEndpoint).ToString(); - public string LocalAddress { get; set; } } public class QueueSearchContext : ScenarioContext diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs index 1b435a9f1b..aedd16f543 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_queue_and_timeframe.cs @@ -76,7 +76,6 @@ public class MyMessageHandler( { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Message Handled"); if (scenarioContext.Step == 0) { scenarioContext.FromAddress = receiveAddresses.MainReceiveAddress; @@ -85,7 +84,6 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) } scenarioContext.RetryCount++; - scenarioContext.Retried = true; return Task.CompletedTask; } @@ -95,7 +93,6 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } - public bool Retried { get; set; } public int RetryCount { get; set; } public string FromAddress { get; set; } public int Step { get; set; } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_selection.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_selection.cs index ec46f77dad..fdf39ca8bf 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_selection.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_resolved_by_selection.cs @@ -62,22 +62,18 @@ public FailingEndpoint() => [Handler] public class MyMessageHandler( Context scenarioContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Message Handled"); if (scenarioContext.Step == 0) { - scenarioContext.FromAddress = receiveAddresses.MainReceiveAddress; scenarioContext.UniqueMessageId = DeterministicGuid.MakeId(context.MessageId, settings.EndpointName()).ToString(); throw new Exception("Simulated Exception"); } scenarioContext.RetryCount++; - scenarioContext.Retried = true; return Task.CompletedTask; } @@ -87,9 +83,7 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } - public bool Retried { get; set; } public int RetryCount { get; set; } - public string FromAddress { get; set; } public int Step { get; set; } } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_again.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_again.cs index 9232262b09..3a16100884 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_again.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_again.cs @@ -58,22 +58,18 @@ public FailingEndpoint() => [Handler] public class MyMessageHandler( Context scenarioContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Message Handled"); if (scenarioContext.Step == 0) { - scenarioContext.FromAddress = receiveAddresses.MainReceiveAddress; scenarioContext.UniqueMessageId = DeterministicGuid.MakeId(context.MessageId, settings.EndpointName()).ToString(); throw new Exception("Simulated Exception"); } scenarioContext.RetryCount++; - scenarioContext.Retried = true; return Task.CompletedTask; } } @@ -82,9 +78,7 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } - public bool Retried { get; set; } public int RetryCount { get; set; } - public string FromAddress { get; set; } public int Step { get; set; } } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_by_queue_and_timeframe.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_by_queue_and_timeframe.cs index 5e5a38396f..e3049bbd86 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_by_queue_and_timeframe.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_pending_retry_is_retried_by_queue_and_timeframe.cs @@ -67,7 +67,6 @@ public class MyMessageHandler( { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Message Handled"); if (scenarioContext.Step == 0) { scenarioContext.FromAddress = receiveAddresses.MainReceiveAddress; @@ -76,7 +75,6 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) } scenarioContext.RetryCount++; - scenarioContext.Retried = true; return Task.CompletedTask; } } @@ -85,7 +83,6 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) public class Context : ScenarioContext, ISequenceContext { public string UniqueMessageId { get; set; } - public bool Retried { get; set; } public int RetryCount { get; set; } public string FromAddress { get; set; } public int Step { get; set; } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_fails.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_fails.cs index fc3bfbcdb7..17c136af0a 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_fails.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_fails.cs @@ -112,18 +112,15 @@ public class MyMessageHandler(MyContext scenarioContext, IReadOnlySettings setti { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.WriteLine("Attempting to process message"); scenarioContext.EndpointNameOfReceivingEndpoint = settings.EndpointName(); scenarioContext.MessageId = context.MessageId.Replace(@"\", "-"); if (!scenarioContext.Succeed) //simulate that the exception will be resolved with the retry { - Console.WriteLine("Message processing failure"); throw new Exception("Simulated exception"); } - Console.WriteLine("Message processing success"); return Task.CompletedTask; } } diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_is_successful.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_is_successful.cs index dd232ed5a0..4565b14cd5 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_is_successful.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_for_a_failed_message_is_successful.cs @@ -1,4 +1,4 @@ -namespace ServiceControl.AcceptanceTests.Recoverability.MessageFailures +namespace ServiceControl.AcceptanceTests.Recoverability.MessageFailures { using System; using System.Collections.Generic; @@ -27,26 +27,16 @@ public async Task Should_show_up_as_resolved_in_the_eventlog(CancellationToken c await Define() .WithEndpoint(b => b.When(bus => bus.SendLocal(new MyMessage())).DoNotFailOnErrorMessages()) - .Done(async c => + .Do("Wait for the message to fail", async ctx => (failure = await GetFailedMessage(ctx)) != null) + .Do("Retry the message", ctx => IssueRetry(ctx, () => this.Post($"/api/errors/{ctx.UniqueMessageId}/retry"))) + .Do("Wait for it to be resolved", async ctx => await IsResolved(ctx, result => failure = result)) + .Do("Read the event log", async _ => { - var failedMessageResult = await GetFailedMessage(c); - failure = failedMessageResult; - if (!failedMessageResult) - { - return false; - } - - if (failure.Status == FailedMessageStatus.Resolved) - { - var eventLogItemsResult = await this.TryGetMany("/api/eventlogitems", item => item.Description.StartsWith("Failed message resolved by retry")); - eventLogItems = eventLogItemsResult; - return eventLogItemsResult; - } - - await IssueRetry(c, () => this.Post($"/api/errors/{c.UniqueMessageId}/retry")); - - return false; + var result = await this.TryGetMany("/api/eventlogitems", item => item.Description.StartsWith("Failed message resolved by retry")); + eventLogItems = result; + return result; }) + .Done() .Run(cancellationToken); using (Assert.EnterMultipleScope()) @@ -64,24 +54,10 @@ public async Task Should_show_up_as_resolved_when_doing_a_multi_retry(Cancellati await Define() .WithEndpoint(b => b.When(bus => bus.SendLocal(new MyMessage())).DoNotFailOnErrorMessages()) - .Done(async c => - { - var failedMessageResult = await GetFailedMessage(c); - failure = failedMessageResult; - if (!failedMessageResult) - { - return false; - } - - if (failure.Status == FailedMessageStatus.Resolved) - { - return true; - } - - await IssueRetry(c, () => this.Post("/api/errors/retry", new List { c.UniqueMessageId })); - - return false; - }) + .Do("Wait for the message to fail", async ctx => (failure = await GetFailedMessage(ctx)) != null) + .Do("Retry the message by id", ctx => IssueRetry(ctx, () => this.Post("/api/errors/retry", new List { ctx.UniqueMessageId }))) + .Do("Wait for it to be resolved", async ctx => await IsResolved(ctx, result => failure = result)) + .Done() .Run(cancellationToken); Assert.That(failure.Status, Is.EqualTo(FailedMessageStatus.Resolved)); @@ -95,24 +71,10 @@ public async Task Should_show_up_as_resolved_when_doing_a_retry_all(Cancellation await Define() .WithEndpoint(b => b.When(bus => bus.SendLocal(new MyMessage())).DoNotFailOnErrorMessages()) - .Done(async c => - { - var failedMessageResult = await GetFailedMessage(c); - failure = failedMessageResult; - if (!failedMessageResult) - { - return false; - } - - if (failure.Status == FailedMessageStatus.Resolved) - { - return true; - } - - await IssueRetry(c, () => this.Post("/api/errors/retry/all")); - - return false; - }) + .Do("Wait for the message to fail", async ctx => (failure = await GetFailedMessage(ctx)) != null) + .Do("Retry everything", ctx => IssueRetry(ctx, () => this.Post("/api/errors/retry/all"))) + .Do("Wait for it to be resolved", async ctx => await IsResolved(ctx, result => failure = result)) + .Done() .Run(cancellationToken); Assert.That(failure.Status, Is.EqualTo(FailedMessageStatus.Resolved)); @@ -122,28 +84,14 @@ await Define() [CancelAfter(120_000)] public async Task Acknowledging_the_retry_should_be_successful(CancellationToken cancellationToken = default) { - FailedMessage failure; + FailedMessage failure = null; await Define() .WithEndpoint(b => b.When(bus => bus.SendLocal(new MyMessage())).DoNotFailOnErrorMessages()) - .Done(async c => - { - var failedMessageResult = await GetFailedMessage(c); - failure = failedMessageResult; - if (!failedMessageResult) - { - return false; - } - - if (failure.Status == FailedMessageStatus.Resolved) - { - return true; - } - - await IssueRetry(c, () => this.Post($"/api/recoverability/groups/{failure.FailureGroups.First().Id}/errors/retry")); - - return false; - }) + .Do("Wait for the message to fail", async ctx => (failure = await GetFailedMessage(ctx)) != null) + .Do("Retry the group it belongs to", ctx => IssueRetry(ctx, () => this.Post($"/api/recoverability/groups/{failure.FailureGroups.First().Id}/errors/retry"))) + .Do("Wait for it to be resolved", async ctx => await IsResolved(ctx, result => failure = result)) + .Done() .Run(cancellationToken); } @@ -155,24 +103,10 @@ public async Task Should_show_up_as_resolved_when_doing_a_retry_all_for_the_give await Define() .WithEndpoint(b => b.When(bus => bus.SendLocal(new MyMessage())).DoNotFailOnErrorMessages()) - .Done(async c => - { - var failedMessageResult = await GetFailedMessage(c); - failure = failedMessageResult; - if (!failedMessageResult) - { - return false; - } - - if (failure.Status == FailedMessageStatus.Resolved) - { - return true; - } - - await IssueRetry(c, () => this.Post($"/api/errors/{c.EndpointNameOfReceivingEndpoint}/retry/all")); - - return false; - }) + .Do("Wait for the message to fail", async ctx => (failure = await GetFailedMessage(ctx)) != null) + .Do("Retry everything for the endpoint", ctx => IssueRetry(ctx, () => this.Post($"/api/errors/{ctx.EndpointNameOfReceivingEndpoint}/retry/all"))) + .Do("Wait for it to be resolved", async ctx => await IsResolved(ctx, result => failure = result)) + .Done() .Run(cancellationToken); Assert.That(failure.Status, Is.EqualTo(FailedMessageStatus.Resolved)); @@ -188,16 +122,28 @@ Task> GetFailedMessage(MyContext c) return this.TryGet("/api/errors/" + c.UniqueMessageId); } - async Task IssueRetry(MyContext c, Func retryAction) + async Task IsResolved(MyContext c, Action capture) { - if (!c.RetryIssued) - { - c.RetryIssued = true; + var result = await GetFailedMessage(c); - await retryAction(); + if (!result) + { + return false; } + + capture(result); + + return result.Item.Status == FailedMessageStatus.Resolved; } + // The handler reads this to decide whether to throw, so it has to be set before the retry + // reaches the endpoint. + Task IssueRetry(MyContext c, Func retryAction) + { + c.RetryIssued = true; + + return retryAction(); + } public class FailureEndpoint : EndpointConfigurationBuilder { @@ -210,15 +156,12 @@ public FailureEndpoint() => [Handler] public class MyMessageHandler( MyContext scenarioContext, - IReadOnlySettings settings, - ReceiveAddresses receiveAddresses) + IReadOnlySettings settings) : IHandleMessages { public Task Handle(MyMessage message, IMessageHandlerContext context) { - Console.Out.WriteLine("Handling message"); scenarioContext.EndpointNameOfReceivingEndpoint = settings.EndpointName(); - scenarioContext.LocalAddress = receiveAddresses.MainReceiveAddress; scenarioContext.MessageId = context.MessageId.Replace(@"\", "-"); if (!scenarioContext.RetryIssued) //simulate that the exception will be resolved with the retry @@ -234,7 +177,7 @@ public Task Handle(MyMessage message, IMessageHandlerContext context) public class MyMessage : ICommand; - public class MyContext : ScenarioContext + public class MyContext : ScenarioContext, ISequenceContext { public string MessageId { get; set; } @@ -243,7 +186,8 @@ public class MyContext : ScenarioContext public bool RetryIssued { get; set; } public string UniqueMessageId => DeterministicGuid.MakeId(MessageId, EndpointNameOfReceivingEndpoint).ToString(); - public string LocalAddress { get; set; } + + public int Step { get; set; } } } -} \ No newline at end of file +} diff --git a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs index af9b323433..40c1a08aef 100644 --- a/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs +++ b/src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_errors_with_same_uniqueid_are_imported.cs @@ -71,10 +71,6 @@ public void Enrich(ErrorEnricherContext context) { testContext.OnMessage(counter); } - else - { - Console.WriteLine("No Counter header found"); - } } }