Merge pull request #5810 from Particular/john/at_phase4

Add acceptance tests for core ServicePulse API routes

Author: johnsimons

docs/acceptance-test-review-plan.md

@@ -109,6 +109,38 @@ Registering the double correctly is only half the job. If the assertion would al
 
 The suite runs against every persister. An assertion about how RavenDB happens to store or trim something says nothing about ServiceControl's behaviour. It also ends up on another persister's exclusion list, where it looks like a missing feature instead of a test that asks for too much. Assert what every persister has to do to be correct.
 
+### The wait that does not cover what the assertion reads
+
+Wait on the path you are about to assert on, not on a cheaper one that looks equivalent.
+
+Persisters do not make a write visible everywhere at the same moment, and search is where they differ most. PostgreSQL indexes a `to_tsvector` expression with GIN, which is maintained inside the writing transaction, so a committed message is searchable at once. SQL Server's full text index is populated by a background process (`CHANGE_TRACKING AUTO`), so a `q=` search answers nothing for a while after the plain list already returns the same message. A test that waits for ingestion by listing messages and then searches therefore passes on PostgreSQL and RavenDB, and fails on SQL Server:
+
+```csharp
+// Wrong: the list does not go through the search index, so the search below can run against
+// an index that has not caught up.
+.Do("Wait for the failures to be ingested", async _ => (await Query(string.Empty)).Count == 3)
+.Do("Search", async _ => matches = await Query($"q={SearchTerm}"))
+
+// Right: the wait runs the same query the assertion depends on.
+.Do("Wait for the search index to catch up", async _ =>
+{
+    matches = await Query($"q={SearchTerm}");
+    return matches.Count == 2;
+})
+```
+
+This cuts across [the assertion that could not have failed](#the-assertion-that-could-not-have-failed), so be deliberate about where the wait stops and the assertion starts.
+
+Wait for the loosest condition that makes the query answerable, not for the answer you expect. `Count == 2` never advances when a search matches three, so the interesting regression, matching too much, is reported as a 90 second timeout rather than as the assertion that would have named the extra row. `Count >= 2` advances as soon as there is enough to judge and lets the assertion do the judging, which turns that same regression into a failure in a few seconds reading `Extra (1): DeliveryFailed`.
+
+Whatever the wait cannot avoid holding, record on the scenario context, because the runner prints the context when a scenario does not finish while a `TimeoutException` on its own says only that 90 seconds passed:
+
+```csharp
+ctx.SearchMatched = string.Join(", ", TypesIn(matchingTerm));
+```
+
+A stalled run then reads as the step it stopped on, from `Advancing from X to Y`, plus what that step kept seeing, which separates an index that never populated from one that matched the wrong thing.
+
 ### The setup that nothing reads
 
 Headers put into a dictionary nothing reads, constants nothing compares against, fixtures left behind after an assertion was deleted. Each one is harmless by itself. Together they make a test look like it covers more than it does, which is how everything above gets through review.

docs/writing-acceptance-tests.md

@@ -0,0 +1,60 @@
+namespace ServiceControl.AcceptanceTests.Licensing
+{
+    using System.Net;
+    using System.Threading.Tasks;
+    using AcceptanceTesting;
+    using NServiceBus.AcceptanceTesting;
+    using NUnit.Framework;
+    using ServiceControl.Licensing;
+
+    class When_the_license_is_requested : AcceptanceTest
+    {
+        [Test]
+        public async Task Should_report_the_instance_and_where_to_extend_the_trial()
+        {
+            LicenseInfo license = null;
+
+            await Define<Context>()
+                .Done(async _ =>
+                {
+                    var result = await this.TryGet<LicenseInfo>($"/api/license?refresh=true&clientName={ClientName}");
+                    license = result.Item;
+                    return result.HasResult;
+                })
+                .Run();
+
+            using (Assert.EnterMultipleScope())
+            {
+                Assert.That(license.InstanceName, Is.EqualTo(Settings.InstanceName),
+                    "ServicePulse labels the license page with the instance it is talking to");
+
+                Assert.That(license.LicenseExtensionUrl, Does.StartWith("https://particular.net/extend-your-trial"),
+                    "With no MassTransit connector reporting in, the license page offers the trial extension rather than the connector link");
+
+                Assert.That(license.LicenseExtensionUrl, Does.Contain($"p={ClientName}"),
+                    "The link has to carry the caller through, or Particular cannot tell which product the request came from");
+            }
+        }
+
+        [Test]
+        public async Task Should_reject_a_request_that_names_no_client()
+        {
+            HttpStatusCode status = default;
+
+            await Define<Context>()
+                .Done(async _ =>
+                {
+                    using var response = await this.GetRaw("/api/license");
+                    status = response.StatusCode;
+                    return true;
+                })
+                .Run();
+
+            Assert.That(status, Is.EqualTo(HttpStatusCode.BadRequest));
+        }
+
+        const string ClientName = "servicepulse";
+
+        class Context : ScenarioContext;
+    }
+}

src/ServiceControl.AcceptanceTests/Licensing/When_the_license_is_requested.cs

@@ -0,0 +1,83 @@
+namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks
+{
+    using System;
+    using System.Linq;
+    using System.Threading;
+    using System.Threading.Tasks;
+    using AcceptanceTesting;
+    using AcceptanceTesting.EndpointTemplates;
+    using NServiceBus;
+    using NServiceBus.AcceptanceTesting;
+    using NServiceBus.CustomChecks;
+    using NUnit.Framework;
+    using ServiceBus.Management.Infrastructure.Settings;
+    using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheck;
+    using CheckStatus = global::ServiceControl.Persistence.Status;
+
+    class When_a_failing_custom_check_is_dismissed : AcceptanceTest
+    {
+        [Test]
+        public async Task Should_come_back_while_the_check_is_still_failing()
+        {
+            CustomCheckView dismissed = null;
+            CustomCheckView returned = null;
+
+            await Define<Context>()
+                .WithEndpoint<Checked>()
+                .Do("Wait for the check to report a failure", async ctx =>
+                {
+                    var checks = await this.TryGetMany<CustomCheckView>("/api/customchecks",
+                        check => check.CustomCheckId == CheckId && check.Status == CheckStatus.Fail);
+
+                    dismissed = checks.HasResult ? checks.Items.Single() : null;
+
+                    return dismissed != null;
+                })
+                .Do("Dismiss it from the page", async _ =>
+                    await this.Delete($"/api/customchecks/{WithoutPrefix(dismissed.Id)}"))
+                .Do("Wait until it has gone", async _ =>
+                {
+                    var checks = await this.TryGetMany<CustomCheckView>("/api/customchecks");
+
+                    return checks.Items.All(check => check.CustomCheckId != CheckId);
+                })
+                .Do("Wait for the next report from the endpoint", async _ =>
+                {
+                    var checks = await this.TryGetMany<CustomCheckView>("/api/customchecks",
+                        check => check.CustomCheckId == CheckId && check.Status == CheckStatus.Fail);
+
+                    returned = checks.HasResult ? checks.Items.Single() : null;
+
+                    return returned != null;
+                })
+                .Done(_ => true)
+                .Run();
+
+            Assert.That(returned.FailureReason, Is.EqualTo(dismissed.FailureReason),
+                "Dismissing a check that is still failing cannot silence it for good, or a real failure disappears from the page for as long as it lasts");
+        }
+
+        static string WithoutPrefix(string id) =>
+            id.StartsWith(DocumentPrefix, StringComparison.OrdinalIgnoreCase) ? id[DocumentPrefix.Length..] : id;
+
+        const string DocumentPrefix = "CustomChecks/";
+        const string CheckId = "DismissedCheck";
+
+        class Context : ScenarioContext, ISequenceContext
+        {
+            public int Step { get; set; }
+        }
+
+        public class Checked : EndpointConfigurationBuilder
+        {
+            public Checked() =>
+                EndpointSetup<DefaultServerWithoutAudit>(c => c.ReportCustomChecksTo(Settings.DEFAULT_INSTANCE_NAME, TimeSpan.FromSeconds(1)));
+
+            class FailingCheck() : CustomCheck(CheckId, "Testing", TimeSpan.FromSeconds(1))
+            {
+                public override Task<CheckResult> PerformCheck(CancellationToken cancellationToken = default) =>
+                    Task.FromResult(CheckResult.Failed("Still failing"));
+            }
+        }
+    }
+}