From e0feeca3f034c08621b0822cb7a8937e885f675e Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 15:10:22 +0100 Subject: [PATCH 01/39] chore(changelog): add placeholder entry for #423 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 32697a06..ca86670d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ Please ADD ALL Changes to the UNRELEASED SECTION and not a specific release - CloudFormation template restructured into the shared format, with parameters only where the deploy workflow passes them and all other settings in a Settings mapping (#427) - ECS application logs in /ecs// are kept for 30 days instead of never expiring, using the same LogRetentionInDays setting as the API Gateway access logs (#421) - The ECS service is now named after the application, so it can be found by name in the DiscordBots cluster (#421) +- TBD - to be finalized after review ### Deprecated ### Removed - Removed NuGet audit suppressions for Scriban 6.2.0 vulnerabilities now that the transitive dependency has been dropped From 2513672768b99ce23fa2dd6e4485bba99a0df119 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 15:45:12 +0100 Subject: [PATCH 02/39] refactor(discord): replace Discord.Net bot login with a Discord webhook - Discord.Net removed - HttpClient webhook sender with 429 retry_after handling - own immutable embed model - release channel removed - config now Discord:WebhookUrl (DISCORD__WEBHOOKURL from Secrets Manager key DISCORD_WEBHOOK) - startup fails on missing or invalid URL - health reflects configuration only Refs #423 --- Dockerfile | 2 +- README.md | 6 + cloudformation.json | 55 +- ...MessageReceivedNotificationHandlerTests.cs | 94 ++++ ...ationMessageReceivedNotificationHandler.cs | 47 +- .../DependencyInjectionTests.cs | 42 +- .../DiscordSetupTests.cs | 139 +++++ .../Models/BotMessageTests.cs | 28 +- .../Models/BotReleaseMessageTests.cs | 37 -- .../Models/DiscordColourTests.cs | 20 + .../Models/EmbedUrlTests.cs | 26 + ...scordBotMessageNotificationHandlerTests.cs | 10 +- ...tReleaseMessageNotificationHandlerTests.cs | 30 -- .../Services/BotServiceTests.cs | 126 +++-- .../Services/DiscordBotConfigurationTests.cs | 90 ++-- .../Services/DiscordBotTests.cs | 476 +++++------------- .../Services/DiscordChannelAdapterTests.cs | 75 --- .../Services/DiscordWebhookClientTests.cs | 249 +++++++++ .../Services/EmbedMapperTests.cs | 78 +++ .../Services/TestHttpMessageHandler.cs | 53 ++ src/BuildBot.Discord/BuildBot.Discord.csproj | 2 +- .../DiscordBotConfiguration.cs | 33 +- src/BuildBot.Discord/DiscordSetup.cs | 29 +- src/BuildBot.Discord/IDiscordBot.cs | 10 +- src/BuildBot.Discord/Models/BotMessage.cs | 3 +- .../Models/BotReleaseMessage.cs | 8 - src/BuildBot.Discord/Models/DiscordColour.cs | 11 + src/BuildBot.Discord/Models/DiscordEmbed.cs | 14 + .../Models/DiscordEmbedField.cs | 6 + src/BuildBot.Discord/Models/EmbedUrl.cs | 11 + ...ordBotReleaseMessageNotificationHandler.cs | 21 - src/BuildBot.Discord/Services/BotService.cs | 32 +- src/BuildBot.Discord/Services/DiscordBot.cs | 196 +------- .../Services/DiscordChannelAdapter.cs | 29 -- .../Services/DiscordSocketClientAdapter.cs | 68 --- .../Services/DiscordWebhookClient.cs | 147 ++++++ .../DiscordWebhookSerializationContext.cs | 15 + src/BuildBot.Discord/Services/EmbedMapper.cs | 42 ++ .../Services/IDiscordChannel.cs | 14 - .../Services/IDiscordRawClient.cs | 24 - .../Services/IDiscordWebhookClient.cs | 11 + .../DiscordBotLoggingExtensions.cs | 56 +-- .../DiscordWebhookClientLoggingExtensions.cs | 17 + .../Services/Models/RateLimitResponse.cs | 6 + .../Services/Models/WebhookEmbed.cs | 15 + .../Services/Models/WebhookEmbedAuthor.cs | 7 + .../Services/Models/WebhookEmbedField.cs | 6 + .../Services/Models/WebhookPayload.cs | 7 + .../Services/Models/WebhookSendResult.cs | 7 + .../GithubPushNotificationHandlerTests.cs | 52 ++ .../GithubStatusNotificationHandlerTests.cs | 59 +++ .../GithubPushNotificationHandler.cs | 34 +- .../GithubStatusNotificationHandler.cs | 45 +- ...rPublishMessageNotificationHandlerTests.cs | 25 + ...hTowerPublishMessageNotificationHandler.cs | 11 +- src/BuildBot/.gitignore | 1 + src/BuildBot/DiscordWebhook.http | 29 ++ src/BuildBot/Helpers/ServerStartup.cs | 11 +- src/BuildBot/appsettings.json | 5 +- 59 files changed, 1623 insertions(+), 1179 deletions(-) create mode 100644 src/BuildBot.Discord.Tests/DiscordSetupTests.cs delete mode 100644 src/BuildBot.Discord.Tests/Models/BotReleaseMessageTests.cs create mode 100644 src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs create mode 100644 src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs delete mode 100644 src/BuildBot.Discord.Tests/Publishers/DiscordBotReleaseMessageNotificationHandlerTests.cs delete mode 100644 src/BuildBot.Discord.Tests/Services/DiscordChannelAdapterTests.cs create mode 100644 src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs create mode 100644 src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs create mode 100644 src/BuildBot.Discord.Tests/Services/TestHttpMessageHandler.cs delete mode 100644 src/BuildBot.Discord/Models/BotReleaseMessage.cs create mode 100644 src/BuildBot.Discord/Models/DiscordColour.cs create mode 100644 src/BuildBot.Discord/Models/DiscordEmbed.cs create mode 100644 src/BuildBot.Discord/Models/DiscordEmbedField.cs create mode 100644 src/BuildBot.Discord/Models/EmbedUrl.cs delete mode 100644 src/BuildBot.Discord/Publishers/DiscordBotReleaseMessageNotificationHandler.cs delete mode 100644 src/BuildBot.Discord/Services/DiscordChannelAdapter.cs delete mode 100644 src/BuildBot.Discord/Services/DiscordSocketClientAdapter.cs create mode 100644 src/BuildBot.Discord/Services/DiscordWebhookClient.cs create mode 100644 src/BuildBot.Discord/Services/DiscordWebhookSerializationContext.cs create mode 100644 src/BuildBot.Discord/Services/EmbedMapper.cs delete mode 100644 src/BuildBot.Discord/Services/IDiscordChannel.cs delete mode 100644 src/BuildBot.Discord/Services/IDiscordRawClient.cs create mode 100644 src/BuildBot.Discord/Services/IDiscordWebhookClient.cs create mode 100644 src/BuildBot.Discord/Services/LoggingExtensions/DiscordWebhookClientLoggingExtensions.cs create mode 100644 src/BuildBot.Discord/Services/Models/RateLimitResponse.cs create mode 100644 src/BuildBot.Discord/Services/Models/WebhookEmbed.cs create mode 100644 src/BuildBot.Discord/Services/Models/WebhookEmbedAuthor.cs create mode 100644 src/BuildBot.Discord/Services/Models/WebhookEmbedField.cs create mode 100644 src/BuildBot.Discord/Services/Models/WebhookPayload.cs create mode 100644 src/BuildBot.Discord/Services/Models/WebhookSendResult.cs create mode 100644 src/BuildBot/.gitignore create mode 100644 src/BuildBot/DiscordWebhook.http diff --git a/Dockerfile b/Dockerfile index affef2c4..e5a2e5ae 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -# chiseled-extra is needed over chiseled for Discord +# chiseled-extra is needed over chiseled for ICU, as globalization invariant mode is disabled #FROM mcr.microsoft.com/dotnet/runtime-deps:9.0-noble-chiseled-extra FROM mcr.microsoft.com/dotnet/runtime-deps:11.0-resolute-chiseled-extra diff --git a/README.md b/README.md index bca536ba..8b476a37 100644 --- a/README.md +++ b/README.md @@ -70,3 +70,9 @@ CloudFormation cannot import an existing Route 53 record set, so the existing `b ## Changelog View [changelog](CHANGELOG.md) + +## Discord Configuration + +| Setting | Environment variable | Type | Default | Description | +|----------------------|-----------------------|------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `Discord:WebhookUrl` | `DISCORD__WEBHOOKURL` | URL | (empty) | Discord channel webhook URL that build, deployment and container messages are posted to. Required: startup fails if it is empty or not an absolute `https` URL. The URL contains the webhook token, so treat it as a secret. In AWS it is read from the Secrets Manager key `DISCORD_WEBHOOK`. | diff --git a/cloudformation.json b/cloudformation.json index 9a961f1b..59b8821a 100644 --- a/cloudformation.json +++ b/cloudformation.json @@ -94,61 +94,10 @@ ], "Secrets": [ { - "Name": "DISCORD__TOKEN", + "Name": "DISCORD__WEBHOOKURL", "ValueFrom": { "Fn::Sub": [ - "${SecretsManagerArn}:DISCORD_TOKEN::", - { - "SecretsManagerArn": { - "Fn::FindInMap": [ - "Settings", - "Values", - "SecretsManagerArn" - ] - } - } - ] - } - }, - { - "Name": "DISCORD__SERVER", - "ValueFrom": { - "Fn::Sub": [ - "${SecretsManagerArn}:DISCORD_SERVER::", - { - "SecretsManagerArn": { - "Fn::FindInMap": [ - "Settings", - "Values", - "SecretsManagerArn" - ] - } - } - ] - } - }, - { - "Name": "DISCORD__CHANNEL", - "ValueFrom": { - "Fn::Sub": [ - "${SecretsManagerArn}:DISCORD_CHANNEL::", - { - "SecretsManagerArn": { - "Fn::FindInMap": [ - "Settings", - "Values", - "SecretsManagerArn" - ] - } - } - ] - } - }, - { - "Name": "DISCORD__RELEASE_CHANNEL", - "ValueFrom": { - "Fn::Sub": [ - "${SecretsManagerArn}:DISCORD_RELEASE_CHANNEL::", + "${SecretsManagerArn}:DISCORD_WEBHOOK::", { "SecretsManagerArn": { "Fn::FindInMap": [ diff --git a/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs b/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs index bbc63b15..67c3b542 100644 --- a/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs +++ b/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Threading; using System.Threading.Tasks; using BuildBot.CloudFormation; @@ -215,4 +216,97 @@ CloudFormationMessageReceivedNotificationHandler handler await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } + + private static DiscordEmbed PublishedEmbed(IMediator mediator) + { + return mediator + .ReceivedCalls() + .Select(static call => call.GetArguments()[0]) + .OfType() + .Single() + .Message; + } + + private async Task PublishAsync(Deployment deployment, StackDetails? stackDetails) + { + ( + _, + ICloudFormationDeploymentExtractor extractor, + IAwsCloudFormation aws, + IMediator mediator, + CloudFormationMessageReceivedNotificationHandler handler + ) = this.CreateHandler(); + + extractor.ExtractDeploymentProperties(Arg.Any()).Returns(deployment); + aws.GetStackDetailsAsync(Arg.Any(), Arg.Any()).Returns(stackDetails); + + await handler.Handle(notification: MakeNotification(VALID_ARN), cancellationToken: this.CancellationToken()); + + return PublishedEmbed(mediator); + } + + [Fact] + public async Task Handle_WithoutStackDetails_PublishesExpectedEmbed() + { + DiscordEmbed embed = await this.PublishAsync(deployment: MakeDeployment(), stackDetails: null); + + Assert.Equal(expected: "MyProject was deployed ", actual: embed.Title); + Assert.Equal( + expected: new Uri( + "https://eu-west-1.console.aws.amazon.com/cloudformation/home?region=eu-west-1#/stacks/stackinfo?stackId=arn:aws:cf:eu-west-1:123:stack/MyStack/x" + ), + actual: embed.Url + ); + Assert.Null(embed.Description); + Assert.Equal(expected: 0x2ECC71u, actual: embed.Colour?.Value); + Assert.Equal( + expected: + [ + new DiscordEmbedField(Name: "ARN", Value: "arn:aws:cf:eu-west-1:123:stack/MyStack/x"), + new DiscordEmbedField(Name: "Status", Value: "UPDATE_COMPLETE"), + ], + actual: embed.Fields + ); + } + + [Fact] + public async Task Handle_WithFailedDeploymentAndStackDetails_PublishesExpectedEmbed() + { + Deployment deployment = MakeDeployment() with { Status = "UPDATE_FAILED", Success = false }; + + DiscordEmbed embed = await this.PublishAsync( + deployment: deployment, + stackDetails: new StackDetails(Description: "A deployment", Version: "2.0.0") + ); + + Assert.Equal(expected: "MyProject (2.0.0) failed to deploy", actual: embed.Title); + Assert.Equal(expected: "A deployment", actual: embed.Description); + Assert.Equal(expected: 0xE74C3Cu, actual: embed.Colour?.Value); + Assert.Equal( + expected: + [ + new DiscordEmbedField(Name: "ARN", Value: "arn:aws:cf:eu-west-1:123:stack/MyStack/x"), + new DiscordEmbedField(Name: "Status", Value: "UPDATE_FAILED"), + new DiscordEmbedField(Name: "Version", Value: "2.0.0"), + ], + actual: embed.Fields + ); + } + + [Fact] + public async Task Handle_WithStackDetailsButNoVersion_OmitsVersionField() + { + DiscordEmbed embed = await this.PublishAsync( + deployment: MakeDeployment(), + stackDetails: new StackDetails(Description: "A deployment", Version: null) + ); + + Assert.Equal(expected: "MyProject was deployed ", actual: embed.Title); + Assert.Equal(expected: "A deployment", actual: embed.Description); + Assert.Equal( + expected: ["ARN", "Status"], + actual: embed.Fields.Select(static f => f.Name), + comparer: StringComparer.Ordinal + ); + } } diff --git a/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs b/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs index ec6a23d7..b0799e4b 100644 --- a/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs +++ b/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs @@ -7,7 +7,6 @@ using BuildBot.CloudFormation.Models; using BuildBot.CloudFormation.Publishers.LoggingExtensions; using BuildBot.Discord.Models; -using Discord; using Mediator; using Microsoft.Extensions.Logging; @@ -67,29 +66,22 @@ public async ValueTask Handle(CloudFormationMessageReceived notification, Cancel ); this._logger.BuildingMessage(deployment.Value); - EmbedBuilder embed = BuildStatusMessage(deployment: deployment.Value, stackDetails: stackDetails); + DiscordEmbed embed = BuildStatusMessage(deployment: deployment.Value, stackDetails: stackDetails); this._logger.PublishingMessage(deployment.Value); await this._mediator.Publish(new BotMessage(embed), cancellationToken: cancellationToken); } - private static EmbedBuilder BuildStatusMessage(in Deployment deployment, StackDetails? stackDetails) + private static DiscordEmbed BuildStatusMessage(in Deployment deployment, StackDetails? stackDetails) { - EmbedBuilder builder = new EmbedBuilder() - .WithTitle(BuildTitle(deployment: deployment, stackDetails: stackDetails)) - .WithUrl(BuildStackUrl(deployment).ToString()) - .WithColor(deployment.Success ? Color.Green : Color.Red) - .WithFields(GetFields(deployment)); - - if (stackDetails is not null) - { - builder - .WithDescription(stackDetails.Value.Description) - .AddField(name: "Version", value: stackDetails.Value.Version); - } - - return builder; + return new( + Title: BuildTitle(deployment: deployment, stackDetails: stackDetails), + Url: BuildStackUrl(deployment), + Description: stackDetails?.Description, + Colour: deployment.Success ? DiscordColour.Green : DiscordColour.Red, + Fields: GetFields(deployment: deployment, stackDetails: stackDetails) + ); } private static string BuildTitle(in Deployment deployment, StackDetails? stackDetails) @@ -113,18 +105,27 @@ private static Uri BuildStackUrl(in Deployment deployment) ); } - private static IReadOnlyList GetFields(in Deployment deployment) + private static IReadOnlyList GetFields(in Deployment deployment, StackDetails? stackDetails) + { + string? version = stackDetails?.Version; + + return string.IsNullOrWhiteSpace(version) + ? [AddArnEmbed(deployment), AddStatusEmbed(deployment)] + : [AddArnEmbed(deployment), AddStatusEmbed(deployment), AddVersionEmbed(version)]; + } + + private static DiscordEmbedField AddArnEmbed(in Deployment deployment) { - return [AddArnEmbed(deployment), AddStatusEmbed(deployment)]; + return new(Name: "ARN", Value: deployment.Arn); } - private static EmbedFieldBuilder AddArnEmbed(in Deployment deployment) + private static DiscordEmbedField AddStatusEmbed(in Deployment deployment) { - return new EmbedFieldBuilder().WithName("ARN").WithValue(deployment.Arn); + return new(Name: "Status", Value: deployment.Status); } - private static EmbedFieldBuilder AddStatusEmbed(in Deployment deployment) + private static DiscordEmbedField AddVersionEmbed(string version) { - return new EmbedFieldBuilder().WithName("Status").WithValue(deployment.Status); + return new(Name: "Version", Value: version); } } diff --git a/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs b/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs index 8997602d..1b78dfb5 100644 --- a/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs +++ b/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs @@ -1,8 +1,7 @@ -using BuildBot.Discord; +using System; using BuildBot.Discord.Models; using BuildBot.Discord.Services; using BuildBot.ServiceModel.ComponentStatus; -using Discord; using FunFair.Test.Common; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -12,12 +11,7 @@ namespace BuildBot.Discord.Tests; public sealed class DependencyInjectionTests : DependencyInjectionTestsBase { - private static readonly DiscordBotConfiguration TestConfig = new( - token: "test-token", - server: "test-server", - channel: "test-channel", - releaseChannel: "test-release-channel" - ); + private static readonly DiscordBotConfiguration TestConfig = new(new Uri("https://example.com/webhook")); public DependencyInjectionTests(ITestOutputHelper output) : base(output: output, dependencyInjectionRegistration: Configure) { } @@ -40,44 +34,32 @@ public void DiscordBotMustBeRegisteredAsIComponentStatus() } [Fact] - public void BotMessageChannelMustBeRegistered() - { - this.RequireService>(); - } - - [Fact] - public void BotReleaseMessageChannelMustBeRegistered() + public void DiscordWebhookClientMustBeRegistered() { - this.RequireService>(); + this.RequireService(); } [Fact] - public void BotServiceMustBeRegisteredAsIHostedService() + public void TimeProviderMustBeRegistered() { - this.RequireService(); + this.RequireService(); } [Fact] - public void DiscordRawClientMustBeRegistered() + public void ConfigurationMustBeRegistered() { - this.RequireService(); + this.RequireService(); } [Fact] - public void DiscordRawClient_WhenDisconnected_LoginStateIsLoggedOut() + public void BotMessageChannelMustBeRegistered() { - IDiscordRawClient client = this.GetService(); - Assert.Equal(expected: LoginState.LoggedOut, actual: client.LoginState); + this.RequireService>(); } [Fact] - public void FindChannel_WhenClientIsDisconnected_ReturnsNull() + public void BotServiceMustBeRegisteredAsIHostedService() { - IDiscordRawClient client = this.GetService(); - IDiscordChannel? channel = client.FindChannel( - serverName: "nonexistent-server", - channelName: "nonexistent-channel" - ); - Assert.Null(channel); + this.RequireService(); } } diff --git a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs new file mode 100644 index 00000000..26b889d0 --- /dev/null +++ b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs @@ -0,0 +1,139 @@ +using System; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using BuildBot.Discord.Models; +using BuildBot.Discord.Services; +using BuildBot.Discord.Services.Models; +using BuildBot.Discord.Tests.Services; +using FunFair.Test.Common; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace BuildBot.Discord.Tests; + +public sealed class DiscordSetupTests : TestBase +{ + private const string SECRET = "secret-token"; + private const string CONTROL_CLIENT_NAME = "Control"; + private static readonly Uri WebhookUrl = new("https://example.com/webhook/" + SECRET); + + [Theory] + [InlineData(1, 2)] + [InlineData(2, 4)] + [InlineData(3, 8)] + public static void CalculateRetryDelay_IsExponential(int attempt, int expectedSeconds) + { + Assert.Equal( + expected: TimeSpan.FromSeconds(expectedSeconds), + actual: DiscordSetup.CalculateRetryDelay(attempt) + ); + } + + [Fact] + public async Task WebhookHttpClient_DoesNotLogRequestUrl() + { + ILogger httpLogger = GetSubstitute(); + httpLogger.IsEnabled(Arg.Any()).Returns(true); + + ILoggerProvider loggerProvider = GetSubstitute(); + loggerProvider.CreateLogger(Arg.Any()).Returns(httpLogger); + + ServiceCollection services = new(); + services + .AddLogging(builder => builder.AddProvider(loggerProvider)) + .AddDiscord(new(WebhookUrl)) + .AddHttpClient(DiscordWebhookClient.HttpClientName) + .ConfigurePrimaryHttpMessageHandler(CreateNoContentHandler); + services.AddHttpClient(CONTROL_CLIENT_NAME).ConfigurePrimaryHttpMessageHandler(CreateNoContentHandler); + + await using ServiceProvider serviceProvider = services.BuildServiceProvider(); + IHttpClientFactory factory = serviceProvider.GetRequiredService(); + + using (HttpClient control = factory.CreateClient(CONTROL_CLIENT_NAME)) + { + using HttpResponseMessage controlResponse = await control.GetAsync( + requestUri: WebhookUrl, + cancellationToken: this.CancellationToken() + ); + } + + Assert.Contains( + expectedSubstring: SECRET, + actualString: LoggedText(httpLogger), + comparisonType: StringComparison.Ordinal + ); + httpLogger.ClearReceivedCalls(); + + using (HttpClient client = factory.CreateClient(DiscordWebhookClient.HttpClientName)) + { + using HttpResponseMessage response = await client.GetAsync( + requestUri: WebhookUrl, + cancellationToken: this.CancellationToken() + ); + } + + Assert.DoesNotContain( + expectedSubstring: SECRET, + actualString: LoggedText(httpLogger), + comparisonType: StringComparison.Ordinal + ); + } + + [Fact] + public async Task WebhookClient_RetriesTransientServerErrorsWithSamePayload() + { + using TestHttpMessageHandler handler = new( + static () => new HttpResponseMessage(HttpStatusCode.InternalServerError), + static () => new HttpResponseMessage(HttpStatusCode.NoContent) + ); + + ServiceCollection services = new(); + services + .AddLogging() + .AddDiscord(new(WebhookUrl)) + .AddHttpClient(DiscordWebhookClient.HttpClientName) + .ConfigurePrimaryHttpMessageHandler(() => handler); + + await using ServiceProvider serviceProvider = services.BuildServiceProvider(); + IDiscordWebhookClient client = serviceProvider.GetRequiredService(); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: EmbedMapper.ToPayload( + new DiscordEmbed(Title: "Title", Url: null, Description: null, Colour: null, Fields: []) + ), + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); + Assert.Equal(expected: 2, actual: handler.Requests.Count); + Assert.All( + collection: handler.Requests, + action: static r => + Assert.Equal( + expected: """{"embeds":[{"title":"Title","author":{"name":"FunFair BuildBot","url":"https://funfair.io","icon_url":"https://s2.coinmarketcap.com/static/img/coins/32x32/1757.png"},"fields":[]}]}""", + actual: r.Body + ) + ); + } + + private static HttpMessageHandler CreateNoContentHandler() + { + return new TestHttpMessageHandler(static () => new HttpResponseMessage(HttpStatusCode.NoContent)); + } + + private static string LoggedText(ILogger logger) + { + return string.Join( + separator: '\n', + values: logger + .ReceivedCalls() + .SelectMany(static call => call.GetArguments()) + .Select(static arg => arg?.ToString()) + ); + } +} diff --git a/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs b/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs index 1288c2ce..2f1848b3 100644 --- a/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs +++ b/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs @@ -1,5 +1,4 @@ using BuildBot.Discord.Models; -using Discord; using FunFair.Test.Common; using Xunit; @@ -7,30 +6,35 @@ namespace BuildBot.Discord.Tests.Models; public sealed class BotMessageTests : TestBase { + private static DiscordEmbed CreateEmbed(string title) + { + return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); + } + [Fact] - public void MessageProperty_ReturnsSuppliedBuilder() + public void MessageProperty_ReturnsSuppliedEmbed() { - EmbedBuilder builder = new(); - BotMessage msg = new(builder); + DiscordEmbed embed = CreateEmbed("Message"); + BotMessage msg = new(embed); - Assert.Same(expected: builder, actual: msg.Message); + Assert.Same(expected: embed, actual: msg.Message); } [Fact] - public void TwoInstances_WithSameBuilder_AreEqual() + public void TwoInstances_WithSameEmbed_AreEqual() { - EmbedBuilder builder = new(); - BotMessage msg1 = new(builder); - BotMessage msg2 = new(builder); + DiscordEmbed embed = CreateEmbed("Message"); + BotMessage msg1 = new(embed); + BotMessage msg2 = new(embed); Assert.Equal(expected: msg1, actual: msg2); } [Fact] - public void TwoInstances_WithDifferentBuilders_AreNotEqual() + public void TwoInstances_WithDifferentEmbeds_AreNotEqual() { - BotMessage msg1 = new(new EmbedBuilder().WithTitle("First Message")); - BotMessage msg2 = new(new EmbedBuilder().WithTitle("Second Message")); + BotMessage msg1 = new(CreateEmbed("First Message")); + BotMessage msg2 = new(CreateEmbed("Second Message")); Assert.NotEqual(expected: msg1, actual: msg2); } diff --git a/src/BuildBot.Discord.Tests/Models/BotReleaseMessageTests.cs b/src/BuildBot.Discord.Tests/Models/BotReleaseMessageTests.cs deleted file mode 100644 index 152d977e..00000000 --- a/src/BuildBot.Discord.Tests/Models/BotReleaseMessageTests.cs +++ /dev/null @@ -1,37 +0,0 @@ -using BuildBot.Discord.Models; -using Discord; -using FunFair.Test.Common; -using Xunit; - -namespace BuildBot.Discord.Tests.Models; - -public sealed class BotReleaseMessageTests : TestBase -{ - [Fact] - public void MessageProperty_ReturnsSuppliedBuilder() - { - EmbedBuilder builder = new(); - BotReleaseMessage msg = new(builder); - - Assert.Same(expected: builder, actual: msg.Message); - } - - [Fact] - public void TwoInstances_WithSameBuilder_AreEqual() - { - EmbedBuilder builder = new(); - BotReleaseMessage msg1 = new(builder); - BotReleaseMessage msg2 = new(builder); - - Assert.Equal(expected: msg1, actual: msg2); - } - - [Fact] - public void TwoInstances_WithDifferentBuilders_AreNotEqual() - { - BotReleaseMessage msg1 = new(new EmbedBuilder().WithTitle("First Release")); - BotReleaseMessage msg2 = new(new EmbedBuilder().WithTitle("Second Release")); - - Assert.NotEqual(expected: msg1, actual: msg2); - } -} diff --git a/src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs b/src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs new file mode 100644 index 00000000..d518faea --- /dev/null +++ b/src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs @@ -0,0 +1,20 @@ +using BuildBot.Discord.Models; +using FunFair.Test.Common; +using Xunit; + +namespace BuildBot.Discord.Tests.Models; + +public sealed class DiscordColourTests : TestBase +{ + [Fact] + public void Green_MatchesDiscordGreen() + { + Assert.Equal(expected: 0x2ECC71u, actual: DiscordColour.Green.Value); + } + + [Fact] + public void Red_MatchesDiscordRed() + { + Assert.Equal(expected: 0xE74C3Cu, actual: DiscordColour.Red.Value); + } +} diff --git a/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs b/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs new file mode 100644 index 00000000..886407ca --- /dev/null +++ b/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs @@ -0,0 +1,26 @@ +using System; +using BuildBot.Discord.Models; +using FunFair.Test.Common; +using Xunit; + +namespace BuildBot.Discord.Tests.Models; + +public sealed class EmbedUrlTests : TestBase +{ + [Fact] + public void FromString_WithAbsoluteUrl_ReturnsUri() + { + Uri? uri = EmbedUrl.FromString("https://example.com/build/1"); + + Assert.Equal(expected: new Uri("https://example.com/build/1"), actual: uri); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("not a url")] + public static void FromString_WithMissingOrRelativeUrl_ReturnsNull(string? value) + { + Assert.Null(EmbedUrl.FromString(value)); + } +} diff --git a/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs b/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs index f0945866..960f6c5d 100644 --- a/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs +++ b/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs @@ -2,7 +2,6 @@ using System.Threading.Tasks; using BuildBot.Discord.Models; using BuildBot.Discord.Publishers; -using Discord; using FunFair.Test.Common; using Microsoft.Extensions.Logging; using NSubstitute; @@ -23,14 +22,13 @@ public async Task Handle_LogsAndPublishesToChannel() DiscordBotMessageNotificationHandler handler = new(messageChannel: channel, logger: logger); - EmbedBuilder builder = new EmbedBuilder().WithTitle("Test Notification"); - BotMessage notification = new(builder); + BotMessage notification = new( + new DiscordEmbed(Title: "Test Notification", Url: null, Description: null, Colour: null, Fields: []) + ); await handler.Handle(notification: notification, cancellationToken: this.CancellationToken()); logger.Received(1).IsEnabled(LogLevel.Information); - await channel - .Received(1) - .PublishAsync(message: Arg.Any(), cancellationToken: Arg.Any()); + await channel.Received(1).PublishAsync(message: notification, cancellationToken: Arg.Any()); } } diff --git a/src/BuildBot.Discord.Tests/Publishers/DiscordBotReleaseMessageNotificationHandlerTests.cs b/src/BuildBot.Discord.Tests/Publishers/DiscordBotReleaseMessageNotificationHandlerTests.cs deleted file mode 100644 index c938fef6..00000000 --- a/src/BuildBot.Discord.Tests/Publishers/DiscordBotReleaseMessageNotificationHandlerTests.cs +++ /dev/null @@ -1,30 +0,0 @@ -using System.Threading; -using System.Threading.Tasks; -using BuildBot.Discord.Models; -using BuildBot.Discord.Publishers; -using Discord; -using FunFair.Test.Common; -using NSubstitute; -using Xunit; - -namespace BuildBot.Discord.Tests.Publishers; - -public sealed class DiscordBotReleaseMessageNotificationHandlerTests : TestBase -{ - [Fact] - public async Task Handle_PublishesToChannel() - { - IMessageChannel channel = GetSubstitute>(); - - DiscordBotReleaseMessageNotificationHandler handler = new(channel); - - EmbedBuilder builder = new EmbedBuilder().WithTitle("Release Notification"); - BotReleaseMessage notification = new(builder); - - await handler.Handle(notification: notification, cancellationToken: this.CancellationToken()); - - await channel - .Received(1) - .PublishAsync(message: Arg.Any(), cancellationToken: Arg.Any()); - } -} diff --git a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs index 15e4291c..8d987561 100644 --- a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs +++ b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs @@ -1,10 +1,12 @@ using System; +using System.Net; +using System.Net.Http; using System.Reflection; using System.Threading; using System.Threading.Tasks; using BuildBot.Discord.Models; using BuildBot.Discord.Services; -using Discord; +using BuildBot.Discord.Services.Models; using FunFair.Test.Common; using NSubstitute; using Xunit; @@ -13,55 +15,61 @@ namespace BuildBot.Discord.Tests.Services; public sealed class BotServiceTests : TestBase { - private static ( - BotService Service, - IDiscordBot Bot, - MessageChannel MessageChannel, - MessageChannel ReleaseChannel - ) CreateService() + private static readonly TimeSpan ForwardingWait = TimeSpan.FromSeconds(3); + + private static DiscordEmbed CreateEmbed(string title) { - IDiscordBot bot = GetSubstitute(); - bot.StartAsync(Arg.Any()).Returns(Task.CompletedTask); - bot.StopAsync(Arg.Any()).Returns(Task.CompletedTask); + return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); + } + private static (BotService Service, IDiscordBot Bot, MessageChannel MessageChannel) CreateService() + { + IDiscordBot bot = GetSubstitute(); MessageChannel messageChannel = new(); - MessageChannel releaseChannel = new(); - BotService service = new(bot: bot, botMessageChannel: messageChannel, botReleaseMessageChannel: releaseChannel); + BotService service = new(bot: bot, botMessageChannel: messageChannel); - return (service, bot, messageChannel, releaseChannel); + return (service, bot, messageChannel); } [Fact] - public async Task StartAsync_DelegatesToBot() + public async Task StartAsync_Completes() { - (BotService service, IDiscordBot bot, _, _) = CreateService(); + (BotService service, IDiscordBot bot, _) = CreateService(); using (service) { await service.StartAsync(this.CancellationToken()); - await bot.Received(1).StartAsync(Arg.Any()); + Assert.Empty(bot.ReceivedCalls()); } } [Fact] - public async Task StopAsync_DelegatesToBot() + public async Task StopAsync_StopsForwardingMessages() { - (BotService service, IDiscordBot bot, _, _) = CreateService(); + (BotService service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); using (service) { await service.StopAsync(this.CancellationToken()); - await bot.Received(1).StopAsync(Arg.Any()); + await messageChannel.PublishAsync( + message: new(CreateEmbed("Test")), + cancellationToken: this.CancellationToken() + ); + + await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); + + await bot.DidNotReceive() + .PublishAsync(embed: Arg.Any(), cancellationToken: Arg.Any()); } } [Fact] public void Dispose_DisposesSubscriptions() { - (BotService service, _, _, _) = CreateService(); + (BotService service, _, _) = CreateService(); service.Dispose(); } @@ -69,51 +77,69 @@ public void Dispose_DisposesSubscriptions() [Fact] public async Task BotMessage_IsForwardedToBot_AfterDelay() { - (BotService service, IDiscordBot bot, MessageChannel messageChannel, _) = CreateService(); + (BotService service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); using (service) { - EmbedBuilder builder = new EmbedBuilder().WithTitle("Test"); - BotMessage message = new(builder); + DiscordEmbed embed = CreateEmbed("Test"); - await messageChannel.PublishAsync(message: message, cancellationToken: this.CancellationToken()); + await messageChannel.PublishAsync(message: new(embed), cancellationToken: this.CancellationToken()); - await Task.Delay(TimeSpan.FromSeconds(2), this.CancellationToken()); + await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); - await bot.Received(1) - .PublishAsync(builder: Arg.Any(), cancellationToken: Arg.Any()); + await bot.Received(1).PublishAsync(embed: embed, cancellationToken: Arg.Any()); } } [Fact] - public async Task BotReleaseMessage_IsForwardedToBot_AfterDelay() + public async Task FailedSend_DoesNotStopLaterMessagesBeingSent() { - (BotService service, IDiscordBot bot, _, MessageChannel releaseChannel) = CreateService(); + IDiscordWebhookClient client = GetSubstitute(); + int calls = 0; + client + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() + ) + .Returns(_ => + Interlocked.Increment(ref calls) == 1 + ? throw new HttpRequestException("connection failed") + : new WebhookSendResult(Success: true, StatusCode: HttpStatusCode.NoContent) + ); + + DiscordBot bot = new( + client: client, + botConfiguration: new(new Uri("https://example.com/webhook")), + logger: this.GetTypedLogger() + ); + MessageChannel messageChannel = new(); - using (service) + using (BotService service = new(bot: bot, botMessageChannel: messageChannel)) { - EmbedBuilder builder = new EmbedBuilder().WithTitle("Release"); - BotReleaseMessage message = new(builder); - - await releaseChannel.PublishAsync(message: message, cancellationToken: this.CancellationToken()); - - await Task.Delay(TimeSpan.FromSeconds(2), this.CancellationToken()); - - await bot.Received(1) - .PublishToReleaseChannelAsync( - builder: Arg.Any(), + await messageChannel.PublishAsync( + message: new(CreateEmbed("First")), + cancellationToken: this.CancellationToken() + ); + await messageChannel.PublishAsync( + message: new(CreateEmbed("Second")), + cancellationToken: this.CancellationToken() + ); + + await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); + + await client + .Received(2) + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), cancellationToken: Arg.Any() ); } } public static TheoryData NullConstructorArguments => - new() - { - { 0, "bot" }, - { 1, "botMessageChannel" }, - { 2, "botReleaseMessageChannel" }, - }; + new() { { 0, "bot" }, { 1, "botMessageChannel" } }; [Theory] [MemberData(nameof(NullConstructorArguments))] @@ -125,16 +151,10 @@ string expectedParamName ConstructorInfo? ctor = typeof(BotService).GetConstructor([ typeof(IDiscordBot), typeof(IMessageChannel), - typeof(IMessageChannel), ]); Assert.NotNull(ctor); - object?[] args = - [ - GetSubstitute(), - GetSubstitute>(), - GetSubstitute>(), - ]; + object?[] args = [GetSubstitute(), GetSubstitute>()]; args[nullParameterIndex] = null; TargetInvocationException ex = Assert.Throws(() => ctor.Invoke(args)); diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotConfigurationTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotConfigurationTests.cs index cd7bfd17..924a7a49 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotConfigurationTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotConfigurationTests.cs @@ -1,4 +1,6 @@ -using BuildBot.Discord; +using System; +using System.Diagnostics; +using System.Reflection; using FunFair.Test.Common; using Xunit; @@ -6,55 +8,81 @@ namespace BuildBot.Discord.Tests.Services; public sealed class DiscordBotConfigurationTests : TestBase { + private const string WEBHOOK_URL = "https://example.com/webhook/secret-token"; + [Fact] - public void Token_ReturnsConstructorValue() + public void FromWebhookUrl_WithHttpsUrl_ReturnsConfiguredConfiguration() + { + DiscordBotConfiguration config = DiscordBotConfiguration.FromWebhookUrl(WEBHOOK_URL); + + Assert.Equal(expected: new Uri(WEBHOOK_URL), actual: config.WebhookUrl); + Assert.True(condition: config.IsConfigured, userMessage: "Https webhook URL should be configured"); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("/webhook/secret-token")] + [InlineData("webhook/secret-token")] + [InlineData("http://example.com/webhook/secret-token")] + [InlineData("ftp://example.com/webhook/secret-token")] + public static void FromWebhookUrl_WithMissingOrInvalidUrl_ThrowsWithoutRevealingValue(string? webhookUrl) { - DiscordBotConfiguration config = new( - token: "my-token", - server: "server", - channel: "channel", - releaseChannel: "releases" + InvalidOperationException exception = Assert.Throws(() => + DiscordBotConfiguration.FromWebhookUrl(webhookUrl) ); - Assert.Equal(expected: "my-token", actual: config.Token); + Assert.Equal( + expected: "Discord:WebhookUrl must be configured as an absolute https URL", + actual: exception.Message + ); + Assert.DoesNotContain( + expectedSubstring: "secret-token", + actualString: exception.ToString(), + comparisonType: StringComparison.Ordinal + ); + } + + [Theory] + [InlineData("https://example.com/webhook/secret-token", true)] + [InlineData("http://example.com/webhook/secret-token", false)] + public static void IsConfigured_ReflectsWhetherUrlIsAbsoluteHttps(string webhookUrl, bool expected) + { + DiscordBotConfiguration config = new(new Uri(webhookUrl)); + + Assert.Equal(expected: expected, actual: config.IsConfigured); } [Fact] - public void Server_ReturnsConstructorValue() + public void IsConfigured_WithRelativeUrl_IsFalse() { - DiscordBotConfiguration config = new( - token: "token", - server: "my-server", - channel: "channel", - releaseChannel: "releases" - ); + DiscordBotConfiguration config = new(new Uri(uriString: "webhook/secret-token", uriKind: UriKind.Relative)); - Assert.Equal(expected: "my-server", actual: config.Server); + Assert.False(condition: config.IsConfigured, userMessage: "Relative webhook URL should not be configured"); } [Fact] - public void Channel_ReturnsConstructorValue() + public void ToString_DoesNotRevealWebhookUrl() { - DiscordBotConfiguration config = new( - token: "token", - server: "server", - channel: "my-channel", - releaseChannel: "releases" - ); + DiscordBotConfiguration config = DiscordBotConfiguration.FromWebhookUrl(WEBHOOK_URL); + + string text = config.ToString(); - Assert.Equal(expected: "my-channel", actual: config.Channel); + Assert.Equal(expected: "DiscordBotConfiguration { IsConfigured = True }", actual: text); } [Fact] - public void ReleaseChannel_ReturnsConstructorValue() + public void DebuggerDisplay_DoesNotReferenceWebhookUrl() { - DiscordBotConfiguration config = new( - token: "token", - server: "server", - channel: "channel", - releaseChannel: "my-releases" + DebuggerDisplayAttribute attribute = Assert.Single( + typeof(DiscordBotConfiguration).GetCustomAttributes() ); - Assert.Equal(expected: "my-releases", actual: config.ReleaseChannel); + Assert.DoesNotContain( + expectedSubstring: nameof(DiscordBotConfiguration.WebhookUrl), + actualString: attribute.Value, + comparisonType: StringComparison.Ordinal + ); } } diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs index 4a3b22b0..3056a57d 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs @@ -1,429 +1,219 @@ using System; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading; using System.Threading.Tasks; +using BuildBot.Discord.Models; using BuildBot.Discord.Services; +using BuildBot.Discord.Services.Models; using BuildBot.ServiceModel.ComponentStatus; -using Discord; using FunFair.Test.Common; using Microsoft.Extensions.Logging; using NSubstitute; +using NSubstitute.Core; +using NSubstitute.ExceptionExtensions; using Xunit; namespace BuildBot.Discord.Tests.Services; public sealed class DiscordBotTests : TestBase { - private static readonly DiscordBotConfiguration Config = new( - token: "test-token", - server: "test-server", - channel: "test-channel", - releaseChannel: "test-release-channel" + private const string SECRET = "secret-token"; + private static readonly Uri WebhookUrl = new("https://example.com/webhook/" + SECRET); + private static readonly DiscordBotConfiguration Config = new(WebhookUrl); + + private static readonly DiscordEmbed Embed = new( + Title: "Test Message", + Url: new("https://example.com/build"), + Description: "Description", + Colour: DiscordColour.Green, + Fields: [new(Name: "Name", Value: "Value")] ); - private (IDiscordRawClient Client, ILogger Logger, DiscordBot Bot) CreateBot() + private (IDiscordWebhookClient Client, ILogger Logger, DiscordBot Bot) CreateBot( + DiscordBotConfiguration configuration + ) { - IDiscordRawClient client = GetSubstitute(); + IDiscordWebhookClient client = GetSubstitute(); ILogger logger = this.GetTypedLogger(); logger.IsEnabled(Arg.Any()).Returns(true); - DiscordBot bot = new(client: client, botConfiguration: Config, logger: logger); + DiscordBot bot = new(client: client, botConfiguration: configuration, logger: logger); return (client, logger, bot); } - private (ILogger Logger, Func Handler) CreateBotForLogTests() + private static void MockDiscordWebhookClientSend(IDiscordWebhookClient client, in WebhookSendResult result) { - IDiscordRawClient client = GetSubstitute(); - ILogger logger = this.GetTypedLogger(); - logger.IsEnabled(Arg.Any()).Returns(true); - - Func? capturedHandler = null; - client - .When(x => x.RegisterLogHandler(Arg.Any>())) - .Do(callInfo => capturedHandler = callInfo.Arg>()); - - _ = new DiscordBot(client: client, botConfiguration: Config, logger: logger); - - return ( - logger, - capturedHandler - ?? throw new InvalidOperationException("DiscordBot constructor did not call RegisterLogHandler") - ); - } - - [Fact] - public void GetStatus_WhenLoggedOut_ReturnsDisconnectedStatus() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - client.LoginState.Returns(LoginState.LoggedOut); - - ServiceStatus status = bot.GetStatus(); - - Assert.Equal(expected: "Discord", actual: status.Name); - Assert.False(condition: status.Ok, userMessage: "Discord should not be connected when logged out"); - } - - [Fact] - public void GetStatus_WhenLoggedIn_ReturnsConnectedStatus() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - client.LoginState.Returns(LoginState.LoggedIn); - - ServiceStatus status = bot.GetStatus(); - - Assert.Equal(expected: "Discord", actual: status.Name); - Assert.True(condition: status.Ok, userMessage: "Discord should be connected when logged in"); - } - - [Fact] - public async Task PublishAsync_WhenChannelNotFound_LogsErrorAndReturns() - { - (IDiscordRawClient client, ILogger logger, DiscordBot bot) = this.CreateBot(); - - client - .FindChannel(serverName: Arg.Any(), channelName: Arg.Any()) - .Returns((IDiscordChannel?)null); - - await bot.PublishAsync( - builder: new EmbedBuilder().WithTitle("Test"), - cancellationToken: this.CancellationToken() - ); - - client.Received(1).FindChannel(serverName: Config.Server, channelName: Config.Channel); - logger.Received(1).IsEnabled(LogLevel.Error); - } - - [Fact] - public async Task PublishToReleaseChannelAsync_WhenChannelNotFound_LogsErrorAndReturns() - { - (IDiscordRawClient client, ILogger logger, DiscordBot bot) = this.CreateBot(); - client - .FindChannel(serverName: Arg.Any(), channelName: Arg.Any()) - .Returns((IDiscordChannel?)null); - - await bot.PublishToReleaseChannelAsync( - builder: new EmbedBuilder().WithTitle("Test"), - cancellationToken: this.CancellationToken() - ); - - client.Received(1).FindChannel(serverName: Config.Server, channelName: Config.ReleaseChannel); - logger.Received(1).IsEnabled(LogLevel.Error); - } - - [Fact] - public async Task PublishAsync_WhenChannelFound_SendsMessageAndLogsSuccess() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns(Task.FromResult<(string SentToChannel, string MessageContent)>(("test-channel", string.Empty))); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - - await bot.PublishAsync( - builder: new EmbedBuilder().WithTitle("Test Message"), - cancellationToken: this.CancellationToken() - ); - - await mockChannel.Received(1).SendMessageAsync(Arg.Any()); - } - - [Fact] - public async Task PublishToReleaseChannelAsync_WhenChannelFound_SendsMessage() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-release-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns( - Task.FromResult<(string SentToChannel, string MessageContent)>(("test-release-channel", string.Empty)) - ); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - - await bot.PublishToReleaseChannelAsync( - builder: new EmbedBuilder().WithTitle("Release Test"), - cancellationToken: this.CancellationToken() - ); - - await mockChannel.Received(1).SendMessageAsync(Arg.Any()); + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() + ) + .Returns(result); } - [Fact] - public async Task PublishAsync_WhenSendFails_WhileLoggedIn_ReconnectsAfterLogout() + private static void AssertSecretNeverLogged(ILogger logger) { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns( - Task.FromException<(string SentToChannel, string MessageContent)>( - new InvalidOperationException("send failed") - ) - ); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - client.LoginState.Returns(LoginState.LoggedIn); - - await bot.PublishAsync( - builder: new EmbedBuilder().WithTitle("Test"), - cancellationToken: this.CancellationToken() + string logged = string.Join( + separator: '\n', + values: logger + .ReceivedCalls() + .SelectMany(static call => call.GetArguments()) + .Select(static arg => arg?.ToString()) ); - await client.Received(1).LogoutAsync(); - await client.Received(1).StopAsync(); - await client.Received(1).LoginAsync(tokenType: TokenType.Bot, token: Config.Token); - } - - [Fact] - public async Task PublishAsync_WhenSendFails_WhileNotLoggedIn_ReconnectsWithoutLogout() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns( - Task.FromException<(string SentToChannel, string MessageContent)>( - new InvalidOperationException("send failed") - ) - ); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - client.LoginState.Returns(LoginState.LoggedOut); - - await bot.PublishAsync( - builder: new EmbedBuilder().WithTitle("Test"), - cancellationToken: this.CancellationToken() + Assert.DoesNotContain( + expectedSubstring: SECRET, + actualString: logged, + comparisonType: StringComparison.Ordinal ); - - await client.DidNotReceive().LogoutAsync(); - await client.Received(1).LoginAsync(tokenType: TokenType.Bot, token: Config.Token); } - [Fact] - public async Task PublishAsync_WhenReconnectFails_LogsError() + private static int LogCallsAt(LogLevel level, ILogger logger) { - (IDiscordRawClient client, ILogger logger, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns( - Task.FromException<(string SentToChannel, string MessageContent)>( - new InvalidOperationException("send failed") - ) + return logger + .ReceivedCalls() + .Count(call => + StringComparer.Ordinal.Equals(x: call.GetMethodInfo().Name, y: nameof(ILogger.Log)) + && Equals(call.GetArguments()[0], level) ); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - client.LoginState.Returns(LoginState.LoggedIn); - client.LogoutAsync().Returns(Task.FromException(new InvalidOperationException("logout failed"))); - - await bot.PublishAsync( - builder: new EmbedBuilder().WithTitle("Test"), - cancellationToken: this.CancellationToken() - ); - - logger.Received(2).IsEnabled(LogLevel.Error); - } - - [Fact] - public async Task StartAsync_CallsLoginStartAndSetGame() - { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - await bot.StartAsync(this.CancellationToken()); - - await client.Received(1).LoginAsync(tokenType: TokenType.Bot, token: Config.Token); - await client.Received(1).StartAsync(); - await client.Received(1).SetGameAsync(name: "GitHub", streamUrl: null, type: ActivityType.Watching); } - [Fact] - public async Task StopAsync_CallsLogout() + [Theory] + [InlineData("https://example.com/webhook/secret-token", true)] + [InlineData("http://example.com/webhook/secret-token", false)] + public void GetStatus_ReflectsConfiguration(string webhookUrl, bool expectedOk) { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); + (_, _, DiscordBot bot) = this.CreateBot(new(new Uri(webhookUrl))); - await bot.StopAsync(this.CancellationToken()); - - await client.Received(1).LogoutAsync(); - } - - [Fact] - public async Task LogAsync_Debug_LogsAtDebugLevel() - { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); - - await handler(new LogMessage(LogSeverity.Debug, source: "Test", message: "Debug message")); + ServiceStatus status = bot.GetStatus(); - logger.Received(1).IsEnabled(LogLevel.Debug); + Assert.Equal(expected: new ServiceStatus(Name: "Discord", Ok: expectedOk), actual: status); } [Fact] - public async Task LogAsync_Verbose_LogsAtInformationLevel() + public void GetStatus_WithRelativeUrl_IsNotOk() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); + (_, _, DiscordBot bot) = this.CreateBot(new(new Uri(uriString: "webhook", uriKind: UriKind.Relative))); - await handler(new LogMessage(LogSeverity.Verbose, source: "Test", message: "Verbose message")); - - logger.Received(1).IsEnabled(LogLevel.Information); + Assert.False(condition: bot.GetStatus().Ok, userMessage: "Relative webhook URL should not be Ok"); } [Fact] - public async Task LogAsync_Info_LogsAtInformationLevel() + public async Task PublishAsync_SendsMappedEmbedToWebhookAndLogsSuccess() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + WebhookPayload? captured = null; + client + .SendAsync( + webhookUrl: WebhookUrl, + payload: Arg.Do(p => captured = p), + cancellationToken: Arg.Any() + ) + .Returns(new WebhookSendResult(Success: true, StatusCode: HttpStatusCode.NoContent)); - await handler(new LogMessage(LogSeverity.Info, source: "Test", message: "Info message")); + await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); - logger.Received(1).IsEnabled(LogLevel.Information); + WebhookPayload payload = AssertReallyNotNull(captured); + WebhookEmbed sent = Assert.Single(payload.Embeds); + WebhookEmbed expected = EmbedMapper.ToWebhookEmbed(Embed); + Assert.Equal(expected: expected with { Fields = sent.Fields }, actual: sent); + Assert.Equal(expected: expected.Fields, actual: sent.Fields); + Assert.Equal(expected: 2, actual: LogCallsAt(level: LogLevel.Information, logger: logger)); + Assert.Equal(expected: 0, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); + AssertSecretNeverLogged(logger); } [Fact] - public async Task LogAsync_Warning_LogsAtWarningLevel() + public async Task PublishAsync_WhenWebhookRejectsMessage_LogsError() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + MockDiscordWebhookClientSend( + client: client, + result: new(Success: false, StatusCode: HttpStatusCode.BadRequest) + ); - await handler(new LogMessage(LogSeverity.Warning, source: "Test", message: "Warning message")); + await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); - logger.Received(1).IsEnabled(LogLevel.Warning); + Assert.Equal(expected: 1, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); + AssertSecretNeverLogged(logger); } [Fact] - public async Task LogAsync_ErrorWithoutException_LogsAtErrorLevel() + public async Task PublishAsync_WhenClientThrows_LogsErrorAndDoesNotThrow() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + client + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() + ) + .ThrowsAsync(new HttpRequestException("connection failed")); - await handler(new LogMessage(LogSeverity.Error, source: "Test", message: "Error message")); + await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); - logger.Received(1).IsEnabled(LogLevel.Error); + Assert.Equal(expected: 1, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); + AssertSecretNeverLogged(logger); } [Fact] - public async Task LogAsync_ErrorWithException_LogsAtErrorLevel() + public async Task PublishAsync_WhenCancelled_LogsWarningAndDoesNotThrow() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); - - await handler( - new LogMessage( - LogSeverity.Error, - source: "Test", - message: "Error message", - exception: new InvalidOperationException("test error") + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + using CancellationTokenSource cts = new(); + await cts.CancelAsync(); + client + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() ) - ); + .ThrowsAsync(new OperationCanceledException(cts.Token)); - logger.Received(1).IsEnabled(LogLevel.Error); - } - - [Fact] - public async Task LogAsync_CriticalWithoutException_LogsAtCriticalLevel() - { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); + await bot.PublishAsync(embed: Embed, cancellationToken: cts.Token); - await handler(new LogMessage(LogSeverity.Critical, source: "Test", message: "Critical message")); - - logger.Received(1).IsEnabled(LogLevel.Critical); + Assert.Equal(expected: 1, actual: LogCallsAt(level: LogLevel.Warning, logger: logger)); + Assert.Equal(expected: 0, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); } [Fact] - public async Task LogAsync_CriticalWithException_LogsAtCriticalLevel() + public async Task PublishAsync_WhenClientCancelsWithoutCallerCancellation_LogsError() { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); - - await handler( - new LogMessage( - LogSeverity.Critical, - source: "Test", - message: "Critical message", - exception: new InvalidOperationException("critical error") + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + client + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() ) - ); + .ThrowsAsync(new TaskCanceledException("timed out")); - logger.Received(1).IsEnabled(LogLevel.Critical); - } + await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); - [Fact] - public async Task LogAsync_UnknownSeverity_LogsAsCritical() - { - (ILogger logger, Func handler) = this.CreateBotForLogTests(); - - await handler(new LogMessage((LogSeverity)99, source: "Test", message: "Unknown severity message")); - - logger.Received(1).IsEnabled(LogLevel.Critical); + Assert.Equal(expected: 1, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); } [Fact] - public async Task PublishAsync_WhenChannelFound_WithNullTitle_SendsMessage() + public async Task PublishAsync_WhenNotConfigured_LogsErrorWithoutSending() { - (IDiscordRawClient client, _, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns(Task.FromResult<(string SentToChannel, string MessageContent)>(("test-channel", string.Empty))); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - - await bot.PublishAsync(builder: new EmbedBuilder(), cancellationToken: this.CancellationToken()); + (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot( + new(new Uri("http://example.com/webhook/" + SECRET)) + ); - await mockChannel.Received(1).SendMessageAsync(Arg.Any()); - } + await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); - [Fact] - public async Task PublishAsync_WhenSendFails_WithNullTitle_LogsErrorAndReconnects() - { - (IDiscordRawClient client, ILogger logger, DiscordBot bot) = this.CreateBot(); - - IDiscordChannel mockChannel = GetSubstitute(); - IDisposable mockTypingState = GetSubstitute(); - - mockChannel.Name.Returns("test-channel"); - mockChannel.EnterTypingState().Returns(mockTypingState); - mockChannel - .SendMessageAsync(Arg.Any()) - .Returns( - Task.FromException<(string SentToChannel, string MessageContent)>( - new InvalidOperationException("send failed") - ) + await client + .DidNotReceive() + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() ); - - client.FindChannel(serverName: Arg.Any(), channelName: Arg.Any()).Returns(mockChannel); - client.LoginState.Returns(LoginState.LoggedOut); - - await bot.PublishAsync(builder: new EmbedBuilder(), cancellationToken: this.CancellationToken()); - - logger.Received(1).IsEnabled(LogLevel.Error); - await client.DidNotReceive().LogoutAsync(); + Assert.Equal(expected: 1, actual: LogCallsAt(level: LogLevel.Error, logger: logger)); + AssertSecretNeverLogged(logger); } } diff --git a/src/BuildBot.Discord.Tests/Services/DiscordChannelAdapterTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordChannelAdapterTests.cs deleted file mode 100644 index 34ed239f..00000000 --- a/src/BuildBot.Discord.Tests/Services/DiscordChannelAdapterTests.cs +++ /dev/null @@ -1,75 +0,0 @@ -using System; -using System.Threading.Tasks; -using BuildBot.Discord.Services; -using Discord; -using FunFair.Test.Common; -using NSubstitute; -using Xunit; - -namespace BuildBot.Discord.Tests.Services; - -public sealed class DiscordChannelAdapterTests : TestBase -{ - [Fact] - public void Name_ReturnsChannelName() - { - ITextChannel channel = GetSubstitute(); - channel.Name.Returns("test-channel"); - - DiscordChannelAdapter adapter = new(channel); - - Assert.Equal(expected: "test-channel", actual: adapter.Name); - } - - [Fact] - public void EnterTypingState_ReturnsTypingStateFromChannel() - { - ITextChannel channel = GetSubstitute(); - IDisposable typingState = GetSubstitute(); - channel.EnterTypingState(Arg.Any()).Returns(typingState); - - DiscordChannelAdapter adapter = new(channel); - - IDisposable result = adapter.EnterTypingState(); - - Assert.Same(expected: typingState, actual: result); - } - - [Fact] - public async Task SendMessageAsync_ReturnsChannelNameAndCleanContent() - { - ITextChannel channel = GetSubstitute(); - IUserMessage message = GetSubstitute(); - - channel.Name.Returns("sent-channel"); - - // In production CleanContent is always "" because the adapter sends text: string.Empty (embed-only). - message.CleanContent.Returns("clean content"); - - channel.SendMessageAsync(text: string.Empty, embed: default).ReturnsForAnyArgs(Task.FromResult(message)); - - DiscordChannelAdapter adapter = new(channel); - - Embed embed = new EmbedBuilder().Build(); - (string sentToChannel, string messageContent) = await adapter.SendMessageAsync(embed); - - Assert.Equal(expected: "sent-channel", actual: sentToChannel); - Assert.Equal(expected: "clean content", actual: messageContent); - - await channel - .Received(1) - .SendMessageAsync( - text: string.Empty, - isTTS: Arg.Any(), - embed: embed, - options: Arg.Any(), - allowedMentions: Arg.Any(), - messageReference: Arg.Any(), - components: Arg.Any(), - stickers: Arg.Any(), - embeds: Arg.Any(), - flags: Arg.Any(), - poll: Arg.Any() - ); - } -} diff --git a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs new file mode 100644 index 00000000..8583f287 --- /dev/null +++ b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs @@ -0,0 +1,249 @@ +using System; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using BuildBot.Discord.Services; +using BuildBot.Discord.Services.Models; +using FunFair.Test.Common; +using Microsoft.Extensions.Logging; +using NSubstitute; +using Xunit; + +namespace BuildBot.Discord.Tests.Services; + +public sealed class DiscordWebhookClientTests : TestBase +{ + private static readonly Uri WebhookUrl = new("https://example.com/webhook/secret-token"); + + private static readonly WebhookPayload Payload = new([ + new WebhookEmbed( + Title: "Title", + Url: new("https://example.com/build"), + Description: null, + Color: 0x2ECC71, + Author: new(Name: "Author", Url: new("https://example.com/"), IconUrl: new("https://example.com/icon.png")), + Fields: [new(Name: "Name", Value: "Value", Inline: true)] + ), + ]); + + private const string EXPECTED_JSON = + """{"embeds":[{"title":"Title","url":"https://example.com/build","color":3066993,"author":{"name":"Author","url":"https://example.com/","icon_url":"https://example.com/icon.png"},"fields":[{"name":"Name","value":"Value","inline":true}]}]}"""; + + private (DiscordWebhookClient Client, ILogger Logger) CreateClient( + TestHttpMessageHandler handler + ) + { + IHttpClientFactory factory = GetSubstitute(); + factory + .CreateClient(DiscordWebhookClient.HttpClientName) + .Returns(_ => new HttpClient(handler: handler, disposeHandler: false)); + + ILogger logger = this.GetTypedLogger(); + + return (new(httpClientFactory: factory, timeProvider: TimeProvider.System, logger: logger), logger); + } + + private static Func Respond(HttpStatusCode statusCode) + { + return () => new(statusCode); + } + + private static Func RateLimited( + string body, + TimeSpan? retryAfter = null, + string mediaType = "application/json" + ) + { + return () => + { + HttpResponseMessage response = new(HttpStatusCode.TooManyRequests) + { + Content = new StringContent(content: body, encoding: Encoding.UTF8, mediaType: mediaType), + }; + + if (retryAfter is not null) + { + response.Headers.RetryAfter = new RetryConditionHeaderValue(retryAfter.Value); + } + + return response; + }; + } + + [Fact] + public async Task SendAsync_PostsPayloadAsJsonToWebhookUrl() + { + using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); + (DiscordWebhookClient client, _) = this.CreateClient(handler); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: Payload, + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); + + TestHttpMessageHandler.RecordedRequest request = Assert.Single(handler.Requests); + Assert.Equal(expected: HttpMethod.Post, actual: request.Method); + Assert.Equal(expected: WebhookUrl, actual: request.RequestUri); + Assert.Equal(expected: "application/json", actual: request.ContentType); + Assert.Equal(expected: EXPECTED_JSON, actual: request.Body); + } + + [Theory] + [InlineData(HttpStatusCode.OK, true)] + [InlineData(HttpStatusCode.NoContent, true)] + [InlineData(HttpStatusCode.BadRequest, false)] + [InlineData(HttpStatusCode.NotFound, false)] + [InlineData(HttpStatusCode.InternalServerError, false)] + public async Task SendAsync_ReturnsResultForStatusWithoutThrowingAsync( + HttpStatusCode statusCode, + bool expectedSuccess + ) + { + using TestHttpMessageHandler handler = new(Respond(statusCode)); + (DiscordWebhookClient client, _) = this.CreateClient(handler); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: Payload, + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: expectedSuccess, StatusCode: statusCode), actual: result); + Assert.Single(handler.Requests); + } + + [Fact] + public async Task SendAsync_WhenRateLimited_RetriesAndSucceeds() + { + using TestHttpMessageHandler handler = new( + RateLimited("""{"message":"You are being rate limited.","retry_after":0.001,"global":false}"""), + Respond(HttpStatusCode.NoContent) + ); + (DiscordWebhookClient client, ILogger logger) = this.CreateClient(handler); + logger.IsEnabled(Arg.Any()).Returns(true); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: Payload, + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); + Assert.Equal(expected: 2, actual: handler.Requests.Count); + Assert.All(collection: handler.Requests, action: r => Assert.Equal(expected: EXPECTED_JSON, actual: r.Body)); + Assert.Contains( + expectedSubstring: "retrying in 0.001 seconds", + actualString: LoggedText(logger), + comparisonType: StringComparison.Ordinal + ); + } + + [Fact] + public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries() + { + using TestHttpMessageHandler handler = new( + RateLimited(body: "rate limited", retryAfter: TimeSpan.FromSeconds(2), mediaType: "text/html"), + Respond(HttpStatusCode.NoContent) + ); + (DiscordWebhookClient client, ILogger logger) = this.CreateClient(handler); + logger.IsEnabled(Arg.Any()).Returns(true); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: Payload, + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); + Assert.Equal(expected: 2, actual: handler.Requests.Count); + Assert.Contains( + expectedSubstring: "retrying in 2 seconds", + actualString: LoggedText(logger), + comparisonType: StringComparison.Ordinal + ); + } + + [Fact] + public async Task SendAsync_WhenRateLimitedOnEveryAttempt_GivesUpAfterMaximumAttempts() + { + const string body = """{"retry_after":0.001}"""; + using TestHttpMessageHandler handler = new( + RateLimited(body), + RateLimited(body), + RateLimited(body), + Respond(HttpStatusCode.NoContent) + ); + (DiscordWebhookClient client, _) = this.CreateClient(handler); + + WebhookSendResult result = await client.SendAsync( + webhookUrl: WebhookUrl, + payload: Payload, + cancellationToken: this.CancellationToken() + ); + + Assert.Equal(expected: new(Success: false, StatusCode: HttpStatusCode.TooManyRequests), actual: result); + Assert.Equal(expected: 3, actual: handler.Requests.Count); + } + + [Fact] + public async Task SendAsync_WhenCancelled_Throws() + { + using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); + (DiscordWebhookClient client, _) = this.CreateClient(handler); + + using CancellationTokenSource cts = new(); + await cts.CancelAsync(); + + await Assert.ThrowsAnyAsync(async () => + await client.SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: cts.Token) + ); + } + + private static string LoggedText(ILogger logger) + { + return string.Join( + separator: '\n', + values: logger + .ReceivedCalls() + .SelectMany(static call => call.GetArguments()) + .Select(static arg => arg?.ToString()) + ); + } + + public static TheoryData RateLimitDelays => + new() + { + { 2.5, null, 2.5 }, + { 2.5, TimeSpan.FromSeconds(10), 2.5 }, + { 0.0, TimeSpan.FromSeconds(10), 10 }, + { -1.0, TimeSpan.FromSeconds(10), 10 }, + { null, TimeSpan.FromSeconds(10), 10 }, + { null, TimeSpan.Zero, 1 }, + { null, null, 1 }, + { 1e300, null, 60 }, + { null, TimeSpan.FromHours(1), 60 }, + }; + + [Theory] + [MemberData(nameof(RateLimitDelays))] + public static void CalculateRateLimitDelay_ReturnsExpectedDelay( + double? retryAfterSeconds, + TimeSpan? retryAfterHeader, + double expectedSeconds + ) + { + TimeSpan actual = DiscordWebhookClient.CalculateRateLimitDelay( + retryAfterSeconds: retryAfterSeconds, + retryAfterHeader: retryAfterHeader + ); + + Assert.Equal(expected: TimeSpan.FromSeconds(expectedSeconds), actual: actual); + } +} diff --git a/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs b/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs new file mode 100644 index 00000000..5f1edc98 --- /dev/null +++ b/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs @@ -0,0 +1,78 @@ +using System; +using BuildBot.Discord.Models; +using BuildBot.Discord.Services; +using BuildBot.Discord.Services.Models; +using FunFair.Test.Common; +using Xunit; + +namespace BuildBot.Discord.Tests.Services; + +public sealed class EmbedMapperTests : TestBase +{ + [Fact] + public void ToWebhookEmbed_MapsAllPropertiesAndAddsStandardAuthor() + { + DiscordEmbed embed = new( + Title: "Title", + Url: new("https://example.com/build"), + Description: "Description", + Colour: DiscordColour.Red, + Fields: [new(Name: "First", Value: "One"), new(Name: "Second", Value: "Two", Inline: true)] + ); + + WebhookEmbed result = EmbedMapper.ToWebhookEmbed(embed); + + Assert.Equal(expected: "Title", actual: result.Title); + Assert.Equal(expected: new Uri("https://example.com/build"), actual: result.Url); + Assert.Equal(expected: "Description", actual: result.Description); + Assert.Equal(expected: 0xE74C3Cu, actual: result.Color); + Assert.Same(expected: EmbedMapper.StandardAuthor, actual: result.Author); + Assert.Equal( + expected: + [ + new WebhookEmbedField(Name: "First", Value: "One", Inline: false), + new WebhookEmbedField(Name: "Second", Value: "Two", Inline: true), + ], + actual: result.Fields + ); + } + + [Fact] + public void ToWebhookEmbed_WithoutOptionalValues_LeavesThemNull() + { + DiscordEmbed embed = new(Title: string.Empty, Url: null, Description: string.Empty, Colour: null, Fields: []); + + WebhookEmbed result = EmbedMapper.ToWebhookEmbed(embed); + + Assert.Null(result.Title); + Assert.Null(result.Url); + Assert.Null(result.Description); + Assert.Null(result.Color); + Assert.Empty(result.Fields); + } + + [Fact] + public void ToPayload_ContainsSingleMappedEmbed() + { + DiscordEmbed embed = new(Title: "Title", Url: null, Description: null, Colour: DiscordColour.Green, Fields: []); + + WebhookPayload payload = EmbedMapper.ToPayload(embed); + + WebhookEmbed result = Assert.Single(payload.Embeds); + Assert.Equal(expected: "Title", actual: result.Title); + Assert.Equal(expected: 0x2ECC71u, actual: result.Color); + } + + [Fact] + public void StandardAuthor_IsFunFairBuildBot() + { + WebhookEmbedAuthor author = EmbedMapper.StandardAuthor; + + Assert.Equal(expected: "FunFair BuildBot", actual: author.Name); + Assert.Equal(expected: new Uri("https://funfair.io"), actual: author.Url); + Assert.Equal( + expected: new Uri("https://s2.coinmarketcap.com/static/img/coins/32x32/1757.png"), + actual: author.IconUrl + ); + } +} diff --git a/src/BuildBot.Discord.Tests/Services/TestHttpMessageHandler.cs b/src/BuildBot.Discord.Tests/Services/TestHttpMessageHandler.cs new file mode 100644 index 00000000..96ab8e1a --- /dev/null +++ b/src/BuildBot.Discord.Tests/Services/TestHttpMessageHandler.cs @@ -0,0 +1,53 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; + +namespace BuildBot.Discord.Tests.Services; + +internal sealed class TestHttpMessageHandler : HttpMessageHandler +{ + private readonly ConcurrentQueue> _responses; + private readonly ConcurrentQueue _requests = new(); + + public TestHttpMessageHandler(params IEnumerable> responses) + { + this._responses = new(responses); + } + + public IReadOnlyList Requests => [.. this._requests]; + + protected override async Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken + ) + { + string body = request.Content is null + ? string.Empty + : await request.Content.ReadAsStringAsync(cancellationToken); + + this._requests.Enqueue( + new( + Method: request.Method, + RequestUri: request.RequestUri, + Body: body, + ContentType: request.Content?.Headers.ContentType?.MediaType + ) + ); + + cancellationToken.ThrowIfCancellationRequested(); + + if (!this._responses.TryDequeue(out Func? response)) + { + throw new InvalidOperationException("No more responses configured"); + } + + return response(); + } + + [DebuggerDisplay("{Method} {RequestUri}")] + internal sealed record RecordedRequest(HttpMethod Method, Uri? RequestUri, string Body, string? ContentType); +} diff --git a/src/BuildBot.Discord/BuildBot.Discord.csproj b/src/BuildBot.Discord/BuildBot.Discord.csproj index bf5ef84b..34c4f2ec 100644 --- a/src/BuildBot.Discord/BuildBot.Discord.csproj +++ b/src/BuildBot.Discord/BuildBot.Discord.csproj @@ -41,8 +41,8 @@ - + diff --git a/src/BuildBot.Discord/DiscordBotConfiguration.cs b/src/BuildBot.Discord/DiscordBotConfiguration.cs index 08355927..50903636 100644 --- a/src/BuildBot.Discord/DiscordBotConfiguration.cs +++ b/src/BuildBot.Discord/DiscordBotConfiguration.cs @@ -1,23 +1,30 @@ +using System; using System.Diagnostics; namespace BuildBot.Discord; -[DebuggerDisplay("Server: {Server}, Channel: {Channel}, Release Channel: {ReleaseChannel}")] -public sealed class DiscordBotConfiguration +[DebuggerDisplay("Webhook configured: {IsConfigured}")] +public sealed record DiscordBotConfiguration(Uri WebhookUrl) { - public DiscordBotConfiguration(string token, string server, string channel, string releaseChannel) - { - this.Token = token; - this.Server = server; - this.Channel = channel; - this.ReleaseChannel = releaseChannel; - } + public bool IsConfigured => IsValidWebhookUrl(this.WebhookUrl); - public string Token { get; } + public static DiscordBotConfiguration FromWebhookUrl(string? webhookUrl) + { + if (Uri.TryCreate(uriString: webhookUrl, uriKind: UriKind.Absolute, out Uri? uri) && IsValidWebhookUrl(uri)) + { + return new(uri); + } - public string Server { get; } + throw new InvalidOperationException("Discord:WebhookUrl must be configured as an absolute https URL"); + } - public string Channel { get; } + public override string ToString() + { + return $"{nameof(DiscordBotConfiguration)} {{ {nameof(this.IsConfigured)} = {this.IsConfigured} }}"; + } - public string ReleaseChannel { get; } + private static bool IsValidWebhookUrl(Uri webhookUrl) + { + return webhookUrl.IsAbsoluteUri && StringComparer.Ordinal.Equals(x: webhookUrl.Scheme, y: Uri.UriSchemeHttps); + } } diff --git a/src/BuildBot.Discord/DiscordSetup.cs b/src/BuildBot.Discord/DiscordSetup.cs index 1eee85b4..3601a2c8 100644 --- a/src/BuildBot.Discord/DiscordSetup.cs +++ b/src/BuildBot.Discord/DiscordSetup.cs @@ -1,20 +1,45 @@ +using System; using BuildBot.Discord.Services; using BuildBot.ServiceModel.ComponentStatus; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Polly; namespace BuildBot.Discord; public static class DiscordSetup { + private const int MAX_RETRIES = 3; + public static IServiceCollection AddDiscord(this IServiceCollection services, DiscordBotConfiguration discordConfig) { + services.TryAddSingleton(TimeProvider.System); + return services .AddSingleton(discordConfig) - .AddSingleton(_ => new DiscordSocketClientAdapter()) + .AddSingleton() .AddSingleton() .AddSingleton(s => s.GetRequiredService()) .AddSingleton(s => s.GetRequiredService()) .AddHostedService() - .AddSingleton(typeof(IMessageChannel<>), typeof(MessageChannel<>)); + .AddSingleton(typeof(IMessageChannel<>), typeof(MessageChannel<>)) + .AddDiscordWebhookHttpClient(); + } + + public static TimeSpan CalculateRetryDelay(int attempt) + { + return TimeSpan.FromSeconds(Math.Pow(x: 2, y: attempt)); + } + + private static IServiceCollection AddDiscordWebhookHttpClient(this IServiceCollection services) + { + // The webhook URL embeds its token, so the default request logging (which writes the URL) is removed. + return services + .AddHttpClient(DiscordWebhookClient.HttpClientName) + .RemoveAllLoggers() + .AddTransientHttpErrorPolicy(policyBuilder => + policyBuilder.WaitAndRetryAsync(retryCount: MAX_RETRIES, sleepDurationProvider: CalculateRetryDelay) + ) + .Services; } } diff --git a/src/BuildBot.Discord/IDiscordBot.cs b/src/BuildBot.Discord/IDiscordBot.cs index a2883eca..17a103d2 100644 --- a/src/BuildBot.Discord/IDiscordBot.cs +++ b/src/BuildBot.Discord/IDiscordBot.cs @@ -1,16 +1,10 @@ using System.Threading; using System.Threading.Tasks; -using Discord; +using BuildBot.Discord.Models; namespace BuildBot.Discord; public interface IDiscordBot { - Task StartAsync(CancellationToken cancellationToken); - - Task StopAsync(CancellationToken cancellationToken); - - ValueTask PublishAsync(EmbedBuilder builder, CancellationToken cancellationToken); - - ValueTask PublishToReleaseChannelAsync(EmbedBuilder builder, CancellationToken cancellationToken); + ValueTask PublishAsync(DiscordEmbed embed, CancellationToken cancellationToken); } diff --git a/src/BuildBot.Discord/Models/BotMessage.cs b/src/BuildBot.Discord/Models/BotMessage.cs index cd4de023..3c4c40fd 100644 --- a/src/BuildBot.Discord/Models/BotMessage.cs +++ b/src/BuildBot.Discord/Models/BotMessage.cs @@ -1,8 +1,7 @@ using System.Diagnostics; -using Discord; using Mediator; namespace BuildBot.Discord.Models; [DebuggerDisplay("{Message.Title}")] -public sealed record BotMessage(EmbedBuilder Message) : INotification; +public sealed record BotMessage(DiscordEmbed Message) : INotification; diff --git a/src/BuildBot.Discord/Models/BotReleaseMessage.cs b/src/BuildBot.Discord/Models/BotReleaseMessage.cs deleted file mode 100644 index d95072b4..00000000 --- a/src/BuildBot.Discord/Models/BotReleaseMessage.cs +++ /dev/null @@ -1,8 +0,0 @@ -using System.Diagnostics; -using Discord; -using Mediator; - -namespace BuildBot.Discord.Models; - -[DebuggerDisplay("{Message.Title}")] -public sealed record BotReleaseMessage(EmbedBuilder Message) : INotification; diff --git a/src/BuildBot.Discord/Models/DiscordColour.cs b/src/BuildBot.Discord/Models/DiscordColour.cs new file mode 100644 index 00000000..ce44fba1 --- /dev/null +++ b/src/BuildBot.Discord/Models/DiscordColour.cs @@ -0,0 +1,11 @@ +using System.Diagnostics; + +namespace BuildBot.Discord.Models; + +[DebuggerDisplay("{Value}")] +public readonly record struct DiscordColour(uint Value) +{ + public static DiscordColour Green { get; } = new(0x2ECC71); + + public static DiscordColour Red { get; } = new(0xE74C3C); +} diff --git a/src/BuildBot.Discord/Models/DiscordEmbed.cs b/src/BuildBot.Discord/Models/DiscordEmbed.cs new file mode 100644 index 00000000..5ff7ab80 --- /dev/null +++ b/src/BuildBot.Discord/Models/DiscordEmbed.cs @@ -0,0 +1,14 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; + +namespace BuildBot.Discord.Models; + +[DebuggerDisplay("{Title}: {Url} {Description} {Colour}")] +public sealed record DiscordEmbed( + string Title, + Uri? Url, + string? Description, + DiscordColour? Colour, + IReadOnlyList Fields +); diff --git a/src/BuildBot.Discord/Models/DiscordEmbedField.cs b/src/BuildBot.Discord/Models/DiscordEmbedField.cs new file mode 100644 index 00000000..0d363c98 --- /dev/null +++ b/src/BuildBot.Discord/Models/DiscordEmbedField.cs @@ -0,0 +1,6 @@ +using System.Diagnostics; + +namespace BuildBot.Discord.Models; + +[DebuggerDisplay("{Name}: {Value} (Inline: {Inline})")] +public readonly record struct DiscordEmbedField(string Name, string Value, bool Inline = false); diff --git a/src/BuildBot.Discord/Models/EmbedUrl.cs b/src/BuildBot.Discord/Models/EmbedUrl.cs new file mode 100644 index 00000000..918a94f9 --- /dev/null +++ b/src/BuildBot.Discord/Models/EmbedUrl.cs @@ -0,0 +1,11 @@ +using System; + +namespace BuildBot.Discord.Models; + +public static class EmbedUrl +{ + public static Uri? FromString(string? value) + { + return Uri.TryCreate(uriString: value, uriKind: UriKind.Absolute, out Uri? uri) ? uri : null; + } +} diff --git a/src/BuildBot.Discord/Publishers/DiscordBotReleaseMessageNotificationHandler.cs b/src/BuildBot.Discord/Publishers/DiscordBotReleaseMessageNotificationHandler.cs deleted file mode 100644 index b9f93c58..00000000 --- a/src/BuildBot.Discord/Publishers/DiscordBotReleaseMessageNotificationHandler.cs +++ /dev/null @@ -1,21 +0,0 @@ -using System.Threading; -using System.Threading.Tasks; -using BuildBot.Discord.Models; -using Mediator; - -namespace BuildBot.Discord.Publishers; - -public sealed class DiscordBotReleaseMessageNotificationHandler : INotificationHandler -{ - private readonly IMessageChannel _messageChannel; - - public DiscordBotReleaseMessageNotificationHandler(IMessageChannel messageChannel) - { - this._messageChannel = messageChannel; - } - - public ValueTask Handle(BotReleaseMessage notification, CancellationToken cancellationToken) - { - return this._messageChannel.PublishAsync(message: notification, cancellationToken: cancellationToken); - } -} diff --git a/src/BuildBot.Discord/Services/BotService.cs b/src/BuildBot.Discord/Services/BotService.cs index 62e3d4a5..b2af4e75 100644 --- a/src/BuildBot.Discord/Services/BotService.cs +++ b/src/BuildBot.Discord/Services/BotService.cs @@ -13,19 +13,12 @@ public sealed class BotService : IHostedService, IDisposable private static readonly TimeSpan InterMessageDelay = TimeSpan.FromSeconds(1); private readonly IDiscordBot _bot; private readonly IMessageChannel _botMessageChannel; - private readonly IMessageChannel _botReleaseMessageChannel; private readonly CancellationTokenSource _cancellationTokenSource = new(); - public BotService( - IDiscordBot bot, - IMessageChannel botMessageChannel, - IMessageChannel botReleaseMessageChannel - ) + public BotService(IDiscordBot bot, IMessageChannel botMessageChannel) { this._bot = bot ?? throw new ArgumentNullException(nameof(bot)); this._botMessageChannel = botMessageChannel ?? throw new ArgumentNullException(nameof(botMessageChannel)); - this._botReleaseMessageChannel = - botReleaseMessageChannel ?? throw new ArgumentNullException(nameof(botReleaseMessageChannel)); this._botMessageChannel.ReadAllAsync(this._cancellationTokenSource.Token) .ToObservable() @@ -35,15 +28,6 @@ IMessageChannel botReleaseMessageChannel ) .Concat() .Subscribe(this._cancellationTokenSource.Token); - - this._botReleaseMessageChannel.ReadAllAsync(this._cancellationTokenSource.Token) - .ToObservable() - .Delay(InterMessageDelay) - .Select(message => - Observable.FromAsync(ct => this.PublishMessageAsync(message: message, cancellationToken: ct).AsTask()) - ) - .Concat() - .Subscribe(this._cancellationTokenSource.Token); } public void Dispose() @@ -54,22 +38,16 @@ public void Dispose() public Task StartAsync(CancellationToken cancellationToken) { - return this._bot.StartAsync(cancellationToken); + return Task.CompletedTask; } - public async Task StopAsync(CancellationToken cancellationToken) + public Task StopAsync(CancellationToken cancellationToken) { - await this._cancellationTokenSource.CancelAsync(); - await this._bot.StopAsync(cancellationToken); + return this._cancellationTokenSource.CancelAsync(); } private ValueTask PublishMessageAsync(BotMessage message, in CancellationToken cancellationToken) { - return this._bot.PublishAsync(builder: message.Message, cancellationToken: cancellationToken); - } - - private ValueTask PublishMessageAsync(BotReleaseMessage message, in CancellationToken cancellationToken) - { - return this._bot.PublishToReleaseChannelAsync(builder: message.Message, cancellationToken: cancellationToken); + return this._bot.PublishAsync(embed: message.Message, cancellationToken: cancellationToken); } } diff --git a/src/BuildBot.Discord/Services/DiscordBot.cs b/src/BuildBot.Discord/Services/DiscordBot.cs index 3da7918d..d47293e8 100644 --- a/src/BuildBot.Discord/Services/DiscordBot.cs +++ b/src/BuildBot.Discord/Services/DiscordBot.cs @@ -1,219 +1,77 @@ using System; using System.Threading; using System.Threading.Tasks; +using BuildBot.Discord.Models; using BuildBot.Discord.Services.LoggingExtensions; +using BuildBot.Discord.Services.Models; using BuildBot.ServiceModel.ComponentStatus; -using Discord; using Microsoft.Extensions.Logging; namespace BuildBot.Discord.Services; public sealed class DiscordBot : IDiscordBot, IComponentStatus { - private static readonly TimeSpan TypingDelay = TimeSpan.FromSeconds(2); private readonly DiscordBotConfiguration _botConfiguration; - private readonly IDiscordRawClient _client; + private readonly IDiscordWebhookClient _client; private readonly ILogger _logger; - private readonly SemaphoreSlim _semaphore = new(1); - public DiscordBot(IDiscordRawClient client, DiscordBotConfiguration botConfiguration, ILogger logger) + public DiscordBot( + IDiscordWebhookClient client, + DiscordBotConfiguration botConfiguration, + ILogger logger + ) { this._client = client; this._botConfiguration = botConfiguration; this._logger = logger; - this._client.RegisterLogHandler(this.LogAsync); } - private static EmbedAuthorBuilder Author { get; } = - new EmbedAuthorBuilder() - .WithName("FunFair BuildBot") - .WithUrl("https://funfair.io") - .WithIconUrl("https://s2.coinmarketcap.com/static/img/coins/32x32/1757.png"); - public ServiceStatus GetStatus() { - return new(Name: "Discord", this._client.LoginState == LoginState.LoggedIn); + return new(Name: "Discord", Ok: this._botConfiguration.IsConfigured); } - public async ValueTask PublishAsync(EmbedBuilder builder, CancellationToken cancellationToken) + public async ValueTask PublishAsync(DiscordEmbed embed, CancellationToken cancellationToken) { - IDiscordChannel? channel = this.GetChannel(this._botConfiguration.Channel); - - if (channel is null) + if (!this._botConfiguration.IsConfigured) { - this._logger.LogDiscordChannelNotFound( - channelName: this._botConfiguration.Channel, - serverName: this._botConfiguration.Server - ); + this._logger.LogWebhookNotConfigured(embed.Title); return; } - await this.PublishCommonAsync(builder: builder, channel: channel, cancellationToken: cancellationToken); - } - - public async ValueTask PublishToReleaseChannelAsync(EmbedBuilder builder, CancellationToken cancellationToken) - { - IDiscordChannel? channel = this.GetChannel(this._botConfiguration.ReleaseChannel); - - if (channel is null) - { - this._logger.LogDiscordChannelNotFound( - channelName: this._botConfiguration.Channel, - serverName: this._botConfiguration.Server - ); - - return; - } - - await this.PublishCommonAsync(builder: builder, channel: channel, cancellationToken: cancellationToken); - } - - private async ValueTask PublishCommonAsync( - EmbedBuilder builder, - IDiscordChannel channel, - CancellationToken cancellationToken - ) - { + // Nothing may escape: BotService sends from an Rx subscription which an exception would terminate. try { - this._logger.LogSendingMessage(channelName: channel.Name, message: builder.Title ?? string.Empty); - - using (channel.EnterTypingState()) - { - Embed embed = IncludeStandardParameters(builder); + this._logger.LogSendingMessage(embed.Title); - (string sentToChannel, string messageContent) = await channel.SendMessageAsync(embed); - this.LogSent(channelName: sentToChannel, content: messageContent); - await Task.Delay(delay: TypingDelay, cancellationToken: cancellationToken); - } - } - catch (Exception exception) - { - this._logger.FailedToPublishMessage( - channelName: this._botConfiguration.Channel, - title: builder.Title ?? string.Empty, - message: exception.Message, - exception: exception + WebhookSendResult result = await this._client.SendAsync( + webhookUrl: this._botConfiguration.WebhookUrl, + payload: EmbedMapper.ToPayload(embed), + cancellationToken: cancellationToken ); - await this.ReconnectAsync(CancellationToken.None); + this.LogResult(embed: embed, result: result); } - } - - private void LogSent(string channelName, string content) - { - this._logger.LogSentMessage(channelName: channelName, message: content); - } - - private static Embed IncludeStandardParameters(EmbedBuilder builder) - { - return builder.WithAuthor(Author).Build(); - } - - private IDiscordChannel? GetChannel(string channelName) - { - return this._client.FindChannel(serverName: this._botConfiguration.Server, channelName: channelName); - } - - private Task LogAsync(LogMessage arg) - { - return arg.Severity switch + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - LogSeverity.Debug => this.LogDebugAsync(arg), - LogSeverity.Verbose => this.LogInformationAsync(arg), - LogSeverity.Info => this.LogInformationAsync(arg), - LogSeverity.Warning => this.LogWarningAsync(arg), - LogSeverity.Error => this.LogErrorAsync(arg), - LogSeverity.Critical => this.LogCriticalAsync(arg), - _ => this.LogCriticalAsync(arg), - }; - } - - private Task LogCriticalAsync(LogMessage arg) - { - if (arg.Exception is not null) - { - this._logger.DiscordCriticalError(message: arg.Message, exception: arg.Exception); + this._logger.LogSendCancelled(embed.Title); } - else + catch (Exception exception) { - this._logger.DiscordCriticalError(arg.Message); + this._logger.LogSendFailed(title: embed.Title, exception: exception); } - - return Task.CompletedTask; } - private Task LogErrorAsync(LogMessage arg) + private void LogResult(DiscordEmbed embed, in WebhookSendResult result) { - if (arg.Exception is not null) + if (result.Success) { - this._logger.DiscordError(message: arg.Message, exception: arg.Exception); + this._logger.LogSentMessage(embed.Title); } else { - this._logger.DiscordError(arg.Message); - } - - return Task.CompletedTask; - } - - private Task LogWarningAsync(LogMessage arg) - { - this._logger.DiscordWarning(arg.Message); - - return Task.CompletedTask; - } - - private Task LogInformationAsync(LogMessage arg) - { - this._logger.DiscordInfo(arg.Message); - - return Task.CompletedTask; - } - - private Task LogDebugAsync(LogMessage arg) - { - this._logger.DiscordDebug(arg.Message); - - return Task.CompletedTask; - } - - public async Task StartAsync(CancellationToken cancellationToken) - { - await this._client.LoginAsync(tokenType: TokenType.Bot, token: this._botConfiguration.Token); - await this._client.StartAsync(); - await this._client.SetGameAsync(name: "GitHub", streamUrl: null, type: ActivityType.Watching); - } - - public Task StopAsync(CancellationToken cancellationToken) - { - return this._client.LogoutAsync(); - } - - private async ValueTask ReconnectAsync(CancellationToken cancellationToken) - { - await this._semaphore.WaitAsync(cancellationToken); - - try - { - if (this._client.LoginState == LoginState.LoggedIn) - { - await this._client.LogoutAsync(); - await this._client.StopAsync(); - } - - await this._client.LoginAsync(tokenType: TokenType.Bot, token: this._botConfiguration.Token); - await this._client.StartAsync(); - await this._client.SetGameAsync(name: "GitHub", streamUrl: null, type: ActivityType.Watching); - } - catch (Exception exception) - { - this._logger.DiscordError(message: "Failed to reconnect", exception: exception); - } - finally - { - this._semaphore.Release(); + this._logger.LogSendRejected(title: embed.Title, statusCode: result.StatusCode); } } } diff --git a/src/BuildBot.Discord/Services/DiscordChannelAdapter.cs b/src/BuildBot.Discord/Services/DiscordChannelAdapter.cs deleted file mode 100644 index a9e0fd99..00000000 --- a/src/BuildBot.Discord/Services/DiscordChannelAdapter.cs +++ /dev/null @@ -1,29 +0,0 @@ -using System; -using System.Threading.Tasks; -using Discord; - -namespace BuildBot.Discord.Services; - -public sealed class DiscordChannelAdapter : IDiscordChannel -{ - private readonly ITextChannel _channel; - - public DiscordChannelAdapter(ITextChannel channel) - { - this._channel = channel ?? throw new ArgumentNullException(nameof(channel)); - } - - public string Name => this._channel.Name; - - public IDisposable EnterTypingState() - { - return this._channel.EnterTypingState(); - } - - public async Task<(string SentToChannel, string MessageContent)> SendMessageAsync(Embed embed) - { - IUserMessage msg = await this._channel.SendMessageAsync(text: string.Empty, embed: embed); - - return (this._channel.Name, msg.CleanContent); - } -} diff --git a/src/BuildBot.Discord/Services/DiscordSocketClientAdapter.cs b/src/BuildBot.Discord/Services/DiscordSocketClientAdapter.cs deleted file mode 100644 index bc7c47ad..00000000 --- a/src/BuildBot.Discord/Services/DiscordSocketClientAdapter.cs +++ /dev/null @@ -1,68 +0,0 @@ -using System; -using System.Linq; -using System.Threading.Tasks; -using Discord; -using Discord.WebSocket; - -namespace BuildBot.Discord.Services; - -internal sealed class DiscordSocketClientAdapter : IDiscordRawClient -{ - private readonly DiscordSocketClient _client; - - public DiscordSocketClientAdapter() - { - this._client = new( - new() - { - GatewayIntents = - GatewayIntents.Guilds | GatewayIntents.GuildMessageTyping | GatewayIntents.GuildMessages, - } - ); - } - - public LoginState LoginState => this._client.LoginState; - - public IDiscordChannel? FindChannel(string serverName, string channelName) - { - SocketGuild? guild = this._client.Guilds.FirstOrDefault(predicate: g => - StringComparer.Ordinal.Equals(x: g.Name, y: serverName) - ); - - SocketTextChannel? textChannel = guild?.TextChannels.FirstOrDefault(predicate: c => - StringComparer.OrdinalIgnoreCase.Equals(x: c.Name, y: channelName) - ); - - return textChannel is null ? null : new DiscordChannelAdapter(textChannel); - } - - public Task LoginAsync(TokenType tokenType, string token) - { - return this._client.LoginAsync(tokenType: tokenType, token: token); - } - - public Task StartAsync() - { - return this._client.StartAsync(); - } - - public Task StopAsync() - { - return this._client.StopAsync(); - } - - public Task LogoutAsync() - { - return this._client.LogoutAsync(); - } - - public Task SetGameAsync(string name, string? streamUrl, ActivityType type) - { - return this._client.SetGameAsync(name: name, streamUrl: streamUrl, type: type); - } - - public void RegisterLogHandler(Func handler) - { - this._client.Log += handler; - } -} diff --git a/src/BuildBot.Discord/Services/DiscordWebhookClient.cs b/src/BuildBot.Discord/Services/DiscordWebhookClient.cs new file mode 100644 index 00000000..4854553c --- /dev/null +++ b/src/BuildBot.Discord/Services/DiscordWebhookClient.cs @@ -0,0 +1,147 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Net.Http.Json; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using BuildBot.Discord.Services.LoggingExtensions; +using BuildBot.Discord.Services.Models; +using Microsoft.Extensions.Logging; + +namespace BuildBot.Discord.Services; + +public sealed class DiscordWebhookClient : IDiscordWebhookClient +{ + public const string HttpClientName = "DiscordWebhook"; + + private const int MAX_ATTEMPTS = 3; + private static readonly TimeSpan DefaultRateLimitDelay = TimeSpan.FromSeconds(1); + private static readonly TimeSpan MaximumRateLimitDelay = TimeSpan.FromSeconds(60); + + private readonly IHttpClientFactory _httpClientFactory; + private readonly ILogger _logger; + private readonly TimeProvider _timeProvider; + + public DiscordWebhookClient( + IHttpClientFactory httpClientFactory, + TimeProvider timeProvider, + ILogger logger + ) + { + this._httpClientFactory = httpClientFactory; + this._timeProvider = timeProvider; + this._logger = logger; + } + + public async ValueTask SendAsync( + Uri webhookUrl, + WebhookPayload payload, + CancellationToken cancellationToken + ) + { + HttpClient client = this._httpClientFactory.CreateClient(HttpClientName); + + for (int attempt = 1; attempt < MAX_ATTEMPTS; ++attempt) + { + using HttpResponseMessage response = await PostAsync( + client: client, + webhookUrl: webhookUrl, + payload: payload, + cancellationToken: cancellationToken + ); + + if (response.StatusCode != HttpStatusCode.TooManyRequests) + { + return ToResult(response); + } + + TimeSpan delay = await GetRateLimitDelayAsync(response: response, cancellationToken: cancellationToken); + + this._logger.LogRateLimited(attempt: attempt, delaySeconds: delay.TotalSeconds); + + await Task.Delay(delay: delay, timeProvider: this._timeProvider, cancellationToken: cancellationToken); + } + + using HttpResponseMessage finalResponse = await PostAsync( + client: client, + webhookUrl: webhookUrl, + payload: payload, + cancellationToken: cancellationToken + ); + + return ToResult(finalResponse); + } + + public static TimeSpan CalculateRateLimitDelay(double? retryAfterSeconds, TimeSpan? retryAfterHeader) + { + if (retryAfterSeconds is > 0) + { + return TimeSpan.FromSeconds( + Math.Min(val1: retryAfterSeconds.Value, val2: MaximumRateLimitDelay.TotalSeconds) + ); + } + + if (retryAfterHeader is { } header && header > TimeSpan.Zero) + { + return header < MaximumRateLimitDelay ? header : MaximumRateLimitDelay; + } + + return DefaultRateLimitDelay; + } + + private static WebhookSendResult ToResult(HttpResponseMessage response) + { + return new(Success: response.IsSuccessStatusCode, StatusCode: response.StatusCode); + } + + private static async Task PostAsync( + HttpClient client, + Uri webhookUrl, + WebhookPayload payload, + CancellationToken cancellationToken + ) + { + using JsonContent content = JsonContent.Create( + inputValue: payload, + jsonTypeInfo: DiscordWebhookSerializationContext.Default.WebhookPayload + ); + + return await client.PostAsync(requestUri: webhookUrl, content: content, cancellationToken: cancellationToken); + } + + private static async ValueTask GetRateLimitDelayAsync( + HttpResponseMessage response, + CancellationToken cancellationToken + ) + { + RateLimitResponse? rateLimit = await ReadRateLimitResponseAsync( + response: response, + cancellationToken: cancellationToken + ); + + return CalculateRateLimitDelay( + retryAfterSeconds: rateLimit?.RetryAfter, + retryAfterHeader: response.Headers.RetryAfter?.Delta + ); + } + + private static async ValueTask ReadRateLimitResponseAsync( + HttpResponseMessage response, + CancellationToken cancellationToken + ) + { + try + { + return await response.Content.ReadFromJsonAsync( + jsonTypeInfo: DiscordWebhookSerializationContext.Default.RateLimitResponse, + cancellationToken: cancellationToken + ); + } + catch (JsonException) + { + // Rate limits applied at Discord's edge return HTML rather than JSON; fall back to the header. + return null; + } + } +} diff --git a/src/BuildBot.Discord/Services/DiscordWebhookSerializationContext.cs b/src/BuildBot.Discord/Services/DiscordWebhookSerializationContext.cs new file mode 100644 index 00000000..084a4571 --- /dev/null +++ b/src/BuildBot.Discord/Services/DiscordWebhookSerializationContext.cs @@ -0,0 +1,15 @@ +using System.Text.Json.Serialization; +using BuildBot.Discord.Services.Models; + +namespace BuildBot.Discord.Services; + +[JsonSourceGenerationOptions( + GenerationMode = JsonSourceGenerationMode.Serialization | JsonSourceGenerationMode.Metadata, + PropertyNamingPolicy = JsonKnownNamingPolicy.SnakeCaseLower, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + WriteIndented = false, + IncludeFields = false +)] +[JsonSerializable(typeof(WebhookPayload))] +[JsonSerializable(typeof(RateLimitResponse))] +internal sealed partial class DiscordWebhookSerializationContext : JsonSerializerContext; diff --git a/src/BuildBot.Discord/Services/EmbedMapper.cs b/src/BuildBot.Discord/Services/EmbedMapper.cs new file mode 100644 index 00000000..c0df09ac --- /dev/null +++ b/src/BuildBot.Discord/Services/EmbedMapper.cs @@ -0,0 +1,42 @@ +using System.Linq; +using BuildBot.Discord.Models; +using BuildBot.Discord.Services.Models; + +namespace BuildBot.Discord.Services; + +public static class EmbedMapper +{ + public static WebhookEmbedAuthor StandardAuthor { get; } = + new( + Name: "FunFair BuildBot", + Url: new("https://funfair.io"), + IconUrl: new("https://s2.coinmarketcap.com/static/img/coins/32x32/1757.png") + ); + + public static WebhookPayload ToPayload(DiscordEmbed embed) + { + return new([ToWebhookEmbed(embed)]); + } + + public static WebhookEmbed ToWebhookEmbed(DiscordEmbed embed) + { + return new( + Title: NullIfEmpty(embed.Title), + Url: embed.Url, + Description: NullIfEmpty(embed.Description), + Color: embed.Colour?.Value, + Author: StandardAuthor, + Fields: [.. embed.Fields.Select(ToWebhookEmbedField)] + ); + } + + private static WebhookEmbedField ToWebhookEmbedField(DiscordEmbedField field) + { + return new(Name: field.Name, Value: field.Value, Inline: field.Inline); + } + + private static string? NullIfEmpty(string? value) + { + return string.IsNullOrEmpty(value) ? null : value; + } +} diff --git a/src/BuildBot.Discord/Services/IDiscordChannel.cs b/src/BuildBot.Discord/Services/IDiscordChannel.cs deleted file mode 100644 index 210ea45b..00000000 --- a/src/BuildBot.Discord/Services/IDiscordChannel.cs +++ /dev/null @@ -1,14 +0,0 @@ -using System; -using System.Threading.Tasks; -using Discord; - -namespace BuildBot.Discord.Services; - -public interface IDiscordChannel -{ - string Name { get; } - - IDisposable EnterTypingState(); - - Task<(string SentToChannel, string MessageContent)> SendMessageAsync(Embed embed); -} diff --git a/src/BuildBot.Discord/Services/IDiscordRawClient.cs b/src/BuildBot.Discord/Services/IDiscordRawClient.cs deleted file mode 100644 index c3213659..00000000 --- a/src/BuildBot.Discord/Services/IDiscordRawClient.cs +++ /dev/null @@ -1,24 +0,0 @@ -using System; -using System.Threading.Tasks; -using Discord; - -namespace BuildBot.Discord.Services; - -public interface IDiscordRawClient -{ - LoginState LoginState { get; } - - IDiscordChannel? FindChannel(string serverName, string channelName); - - Task LoginAsync(TokenType tokenType, string token); - - Task StartAsync(); - - Task StopAsync(); - - Task LogoutAsync(); - - Task SetGameAsync(string name, string? streamUrl, ActivityType type); - - void RegisterLogHandler(Func handler); -} diff --git a/src/BuildBot.Discord/Services/IDiscordWebhookClient.cs b/src/BuildBot.Discord/Services/IDiscordWebhookClient.cs new file mode 100644 index 00000000..59580fdf --- /dev/null +++ b/src/BuildBot.Discord/Services/IDiscordWebhookClient.cs @@ -0,0 +1,11 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using BuildBot.Discord.Services.Models; + +namespace BuildBot.Discord.Services; + +public interface IDiscordWebhookClient +{ + ValueTask SendAsync(Uri webhookUrl, WebhookPayload payload, CancellationToken cancellationToken); +} diff --git a/src/BuildBot.Discord/Services/LoggingExtensions/DiscordBotLoggingExtensions.cs b/src/BuildBot.Discord/Services/LoggingExtensions/DiscordBotLoggingExtensions.cs index c7cd70dd..effe2a1c 100644 --- a/src/BuildBot.Discord/Services/LoggingExtensions/DiscordBotLoggingExtensions.cs +++ b/src/BuildBot.Discord/Services/LoggingExtensions/DiscordBotLoggingExtensions.cs @@ -1,4 +1,5 @@ using System; +using System.Net; using Microsoft.Extensions.Logging; namespace BuildBot.Discord.Services.LoggingExtensions; @@ -8,59 +9,30 @@ internal static partial class DiscordBotLoggingExtensions [LoggerMessage( EventId = 1, Level = LogLevel.Error, - Message = "DISCORD: {channelName}: Could not find channel on server {serverName}" + Message = "DISCORD: Webhook is not configured; dropping message {title}" )] - public static partial void LogDiscordChannelNotFound( - this ILogger logger, - string channelName, - string serverName - ); + public static partial void LogWebhookNotConfigured(this ILogger logger, string title); - [LoggerMessage( - EventId = 2, - Level = LogLevel.Warning, - Message = "DISCORD: {channelName}: Sending message {message}" - )] - public static partial void LogSendingMessage(this ILogger logger, string channelName, string message); + [LoggerMessage(EventId = 2, Level = LogLevel.Information, Message = "DISCORD: Sending message {title}")] + public static partial void LogSendingMessage(this ILogger logger, string title); - [LoggerMessage(EventId = 3, Level = LogLevel.Warning, Message = "DISCORD: {channelName}: Sent message {message}")] - public static partial void LogSentMessage(this ILogger logger, string channelName, string message); + [LoggerMessage(EventId = 3, Level = LogLevel.Information, Message = "DISCORD: Sent message {title}")] + public static partial void LogSentMessage(this ILogger logger, string title); [LoggerMessage( EventId = 4, Level = LogLevel.Error, - Message = "DISCORD: {channelName}: Failed to send message {title}: {message}" + Message = "DISCORD: Failed to send message {title}: HTTP {statusCode}" )] - public static partial void FailedToPublishMessage( + public static partial void LogSendRejected( this ILogger logger, - string channelName, string title, - string message, - Exception exception + HttpStatusCode statusCode ); - [LoggerMessage(EventId = 6, Level = LogLevel.Debug, Message = "DISCORD: DEBUG: {message}")] - public static partial void DiscordDebug(this ILogger logger, string message); - - [LoggerMessage(EventId = 7, Level = LogLevel.Information, Message = "DISCORD: INFO: {message}")] - public static partial void DiscordInfo(this ILogger logger, string message); - - [LoggerMessage(EventId = 8, Level = LogLevel.Warning, Message = "DISCORD: WARNING: {message}")] - public static partial void DiscordWarning(this ILogger logger, string message); + [LoggerMessage(EventId = 5, Level = LogLevel.Error, Message = "DISCORD: Failed to send message {title}")] + public static partial void LogSendFailed(this ILogger logger, string title, Exception exception); - [LoggerMessage(EventId = 9, Level = LogLevel.Error, Message = "DISCORD: ERROR: {message}")] - public static partial void DiscordError(this ILogger logger, string message); - - [LoggerMessage(EventId = 9, Level = LogLevel.Error, Message = "DISCORD: ERROR: {message}")] - public static partial void DiscordError(this ILogger logger, string message, Exception exception); - - [LoggerMessage(EventId = 10, Level = LogLevel.Critical, Message = "DISCORD: CRITICAL ERROR: {message}")] - public static partial void DiscordCriticalError(this ILogger logger, string message); - - [LoggerMessage(EventId = 10, Level = LogLevel.Critical, Message = "DISCORD: CRITICAL ERROR: {message}")] - public static partial void DiscordCriticalError( - this ILogger logger, - string message, - Exception exception - ); + [LoggerMessage(EventId = 6, Level = LogLevel.Warning, Message = "DISCORD: Sending message {title} was cancelled")] + public static partial void LogSendCancelled(this ILogger logger, string title); } diff --git a/src/BuildBot.Discord/Services/LoggingExtensions/DiscordWebhookClientLoggingExtensions.cs b/src/BuildBot.Discord/Services/LoggingExtensions/DiscordWebhookClientLoggingExtensions.cs new file mode 100644 index 00000000..f8e31dc0 --- /dev/null +++ b/src/BuildBot.Discord/Services/LoggingExtensions/DiscordWebhookClientLoggingExtensions.cs @@ -0,0 +1,17 @@ +using Microsoft.Extensions.Logging; + +namespace BuildBot.Discord.Services.LoggingExtensions; + +internal static partial class DiscordWebhookClientLoggingExtensions +{ + [LoggerMessage( + EventId = 1, + Level = LogLevel.Warning, + Message = "DISCORD: Rate limited on attempt {attempt}: retrying in {delaySeconds} seconds" + )] + public static partial void LogRateLimited( + this ILogger logger, + int attempt, + double delaySeconds + ); +} diff --git a/src/BuildBot.Discord/Services/Models/RateLimitResponse.cs b/src/BuildBot.Discord/Services/Models/RateLimitResponse.cs new file mode 100644 index 00000000..ff81cb15 --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/RateLimitResponse.cs @@ -0,0 +1,6 @@ +using System.Diagnostics; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("Retry after: {RetryAfter}s, Global: {Global}")] +public sealed record RateLimitResponse(double? RetryAfter, bool Global); diff --git a/src/BuildBot.Discord/Services/Models/WebhookEmbed.cs b/src/BuildBot.Discord/Services/Models/WebhookEmbed.cs new file mode 100644 index 00000000..cb528cac --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/WebhookEmbed.cs @@ -0,0 +1,15 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("{Title}: {Url} {Description} {Color}")] +public sealed record WebhookEmbed( + string? Title, + Uri? Url, + string? Description, + uint? Color, + WebhookEmbedAuthor Author, + IReadOnlyList Fields +); diff --git a/src/BuildBot.Discord/Services/Models/WebhookEmbedAuthor.cs b/src/BuildBot.Discord/Services/Models/WebhookEmbedAuthor.cs new file mode 100644 index 00000000..544c9edc --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/WebhookEmbedAuthor.cs @@ -0,0 +1,7 @@ +using System; +using System.Diagnostics; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("{Name}: {Url} {IconUrl}")] +public sealed record WebhookEmbedAuthor(string Name, Uri Url, Uri IconUrl); diff --git a/src/BuildBot.Discord/Services/Models/WebhookEmbedField.cs b/src/BuildBot.Discord/Services/Models/WebhookEmbedField.cs new file mode 100644 index 00000000..4abf3d4c --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/WebhookEmbedField.cs @@ -0,0 +1,6 @@ +using System.Diagnostics; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("{Name}: {Value} (Inline: {Inline})")] +public readonly record struct WebhookEmbedField(string Name, string Value, bool Inline); diff --git a/src/BuildBot.Discord/Services/Models/WebhookPayload.cs b/src/BuildBot.Discord/Services/Models/WebhookPayload.cs new file mode 100644 index 00000000..03750e29 --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/WebhookPayload.cs @@ -0,0 +1,7 @@ +using System.Collections.Generic; +using System.Diagnostics; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("Embeds: {Embeds.Count}")] +public sealed record WebhookPayload(IReadOnlyList Embeds); diff --git a/src/BuildBot.Discord/Services/Models/WebhookSendResult.cs b/src/BuildBot.Discord/Services/Models/WebhookSendResult.cs new file mode 100644 index 00000000..164750b3 --- /dev/null +++ b/src/BuildBot.Discord/Services/Models/WebhookSendResult.cs @@ -0,0 +1,7 @@ +using System.Diagnostics; +using System.Net; + +namespace BuildBot.Discord.Services.Models; + +[DebuggerDisplay("Success: {Success}, Status: {StatusCode}")] +public readonly record struct WebhookSendResult(bool Success, HttpStatusCode StatusCode); diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs index 2a269e8c..c7fc9aa6 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading; using System.Threading.Tasks; using BuildBot.Discord.Models; @@ -185,4 +186,55 @@ public async Task SingleCommitWithNullUsernameShouldPublishAsync() await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } + + private static DiscordEmbed PublishedEmbed(IMediator mediator) + { + return mediator + .ReceivedCalls() + .Select(static call => call.GetArguments()[0]) + .OfType() + .Single() + .Message; + } + + [Fact] + public async Task PublishedEmbedHasExpectedContentAsync() + { + (IMediator mediator, GithubPushNotificationHandler handler) = this.CreateHandler(); + + Commit commit1 = MakeCommit(message: "First commit", username: "devuser"); + Commit commit2 = MakeCommit(message: "Second commit", username: null); + Push push = MakePush(repoName: NORMAL_REPO, gitRef: NORMAL_REF, commits: [commit1, commit2]); + + await handler.Handle(notification: new(push), cancellationToken: this.CancellationToken()); + + DiscordEmbed embed = PublishedEmbed(mediator); + Assert.Equal(expected: "testpusher pushed 2 commits to BuildBot (feature/my-feature)", actual: embed.Title); + Assert.Equal(expected: new Uri("https://github.com/example/repo/compare/before...after"), actual: embed.Url); + Assert.Null(embed.Description); + Assert.Null(embed.Colour); + Assert.Equal( + expected: + [ + new DiscordEmbedField(Name: "**devuser** - First commit", Value: "0 added, 0 modified, 0 removed"), + new DiscordEmbedField(Name: "**Test User** - Second commit", Value: "0 added, 0 modified, 0 removed"), + ], + actual: embed.Fields + ); + } + + [Fact] + public async Task PublishedEmbedForSingleCommitUsesSingularTitleAsync() + { + (IMediator mediator, GithubPushNotificationHandler handler) = this.CreateHandler(); + + Push push = MakePush(repoName: NORMAL_REPO, gitRef: NORMAL_REF, commits: [MakeCommit("Only commit")]); + + await handler.Handle(notification: new(push), cancellationToken: this.CancellationToken()); + + Assert.Equal( + expected: "testpusher pushed 1 commit to BuildBot (feature/my-feature)", + actual: PublishedEmbed(mediator).Title + ); + } } diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs index c7706795..174de64e 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs @@ -1,6 +1,8 @@ using System; +using System.Linq; using System.Threading; using System.Threading.Tasks; +using BuildBot.Discord.Models; using BuildBot.GitHub.Models; using BuildBot.GitHub.Publishers; using BuildBot.ServiceModel.GitHub; @@ -118,4 +120,61 @@ public async Task NonPendingStateWithNoBranchesShouldPublishAsync(string state) await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } + + private static DiscordEmbed PublishedEmbed(IMediator mediator) + { + return mediator + .ReceivedCalls() + .Select(static call => call.GetArguments()[0]) + .OfType() + .Single() + .Message; + } + + public static TheoryData StateColours => + new() + { + { "success", 0x2ECC71 }, + { "SUCCESS", 0x2ECC71 }, + { "failure", 0xE74C3C }, + { "error", null }, + }; + + [Theory] + [MemberData(nameof(StateColours))] + public async Task PublishedEmbedHasExpectedContentAsync(string state, uint? expectedColour) + { + (IMediator mediator, GithubStatusNotificationHandler handler) = this.CreateHandler(); + + await handler.Handle(notification: new(MakeStatus(state: state)), cancellationToken: this.CancellationToken()); + + DiscordEmbed embed = PublishedEmbed(mediator); + Assert.Equal(expected: "Build finished for ci/build from BuildBot (main)", actual: embed.Title); + Assert.Equal(expected: new Uri("https://ci.example.com/builds/123"), actual: embed.Url); + Assert.Equal(expected: "Built at abc123", actual: embed.Description); + Assert.Equal(expected: expectedColour, actual: embed.Colour?.Value); + Assert.Equal( + expected: + [ + new DiscordEmbedField(Name: "Head commit", Value: "testuser - Fix something"), + new DiscordEmbedField(Name: "Branch", Value: "main"), + ], + actual: embed.Fields + ); + } + + [Fact] + public async Task PublishedEmbedWithNoBranchesUsesUnknownAsync() + { + (IMediator mediator, GithubStatusNotificationHandler handler) = this.CreateHandler(); + + await handler.Handle( + notification: new(MakeStatusWithNoBranches(state: "success")), + cancellationToken: this.CancellationToken() + ); + + DiscordEmbed embed = PublishedEmbed(mediator); + Assert.Equal(expected: "Build finished for ci/build from BuildBot ((unknown))", actual: embed.Title); + Assert.Equal(expected: new DiscordEmbedField(Name: "Branch", Value: "(unknown)"), actual: embed.Fields[1]); + } } diff --git a/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs b/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs index df50bee8..0ff29bb8 100644 --- a/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs +++ b/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -7,7 +8,6 @@ using BuildBot.GitHub.Models; using BuildBot.GitHub.Publishers.LoggingExtensions; using BuildBot.ServiceModel.GitHub; -using Discord; using Mediator; using Microsoft.Extensions.Logging; @@ -52,9 +52,9 @@ private async ValueTask PublishAsync(Push message, CancellationToken cancellatio return; } - EmbedBuilder builder = BuildPushEmbed(message); + DiscordEmbed embed = BuildPushEmbed(message); - await this._mediator.Publish(new BotMessage(builder), cancellationToken: cancellationToken); + await this._mediator.Publish(new BotMessage(embed), cancellationToken: cancellationToken); } private static bool IsIgnoredRepo(in Push message) @@ -82,29 +82,31 @@ static bool IsPackageUpdate(Commit commit) } } - private static EmbedBuilder BuildPushEmbed(in Push message) - { - return message.Commits.Aggregate(CreateBasicEmbed(message), func: AddCommit); - } - - private static EmbedBuilder CreateBasicEmbed(in Push message) + private static DiscordEmbed BuildPushEmbed(in Push message) { string commitString = GetCommitString(message); string title = GetCommitTitle(message: message, commitString: commitString); - return new EmbedBuilder().WithTitle(title).WithUrl(message.CompareUrl); + return new( + Title: title, + Url: EmbedUrl.FromString(message.CompareUrl), + Description: null, + Colour: null, + Fields: CreateCommitFields(message.Commits) + ); } - private static EmbedBuilder AddCommit(EmbedBuilder current, Commit commit) + private static IReadOnlyList CreateCommitFields(IReadOnlyList commits) { - return current.AddField(CreateCommitEmbed(commit)); + return [.. commits.Select(CreateCommitEmbed)]; } - private static EmbedFieldBuilder CreateCommitEmbed(in Commit commit) + private static DiscordEmbedField CreateCommitEmbed(Commit commit) { - return new EmbedFieldBuilder() - .WithName($"**{commit.Author.Username ?? commit.Author.Name}** - {commit.Message}") - .WithValue($"{commit.Added.Count} added, {commit.Modified.Count} modified, {commit.Removed.Count} removed"); + return new( + Name: $"**{commit.Author.Username ?? commit.Author.Name}** - {commit.Message}", + Value: $"{commit.Added.Count} added, {commit.Modified.Count} modified, {commit.Removed.Count} removed" + ); } private static string GetCommitTitle(in Push message, string commitString) diff --git a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs index ccf038b8..2c31b72e 100644 --- a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs +++ b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs @@ -8,7 +8,6 @@ using BuildBot.GitHub.Models; using BuildBot.GitHub.Publishers.LoggingExtensions; using BuildBot.ServiceModel.GitHub; -using Discord; using Mediator; using Microsoft.Extensions.Logging; @@ -18,8 +17,8 @@ public sealed class GithubStatusNotificationHandler : INotificationHandler ColourMappings = [ - new(State: "success", Colour: Color.Green), - new(State: "failure", Colour: Color.Red), + new(State: "success", Colour: DiscordColour.Green), + new(State: "failure", Colour: DiscordColour.Red), ]; private readonly ILogger _logger; @@ -51,49 +50,45 @@ private static bool IsPendingBuild(in Status message) return StringComparer.OrdinalIgnoreCase.Equals(x: message.State, y: "pending"); } - private static EmbedBuilder BuildStatusMessage(in Status message) + private static DiscordEmbed BuildStatusMessage(in Status message) { Branch? lastBranch = message.Branches.Count > 0 ? message.Branches[^1] : null; + string branchName = lastBranch?.Name ?? "(unknown)"; - return new EmbedBuilder() - .WithTitle( - $"{message.Description} for {message.Context} from {message.Repository.Name} ({lastBranch?.Name ?? "(unknown)"})" - ) - .WithUrl(message.TargetUrl) - .WithDescription($"Built at {message.StatusCommit.Sha}") - .WithColor(GetEmbedColor(message)) - .WithFields(GetFields(message)); + return new( + Title: $"{message.Description} for {message.Context} from {message.Repository.Name} ({branchName})", + Url: EmbedUrl.FromString(message.TargetUrl), + Description: $"Built at {message.StatusCommit.Sha}", + Colour: GetEmbedColour(message), + Fields: GetFields(message) + ); } - private static IReadOnlyList GetFields(in Status message) + private static IReadOnlyList GetFields(in Status message) { return [AddHeadCommitEmbed(message.StatusCommit), AddBranchEmbed(message)]; } - private static EmbedFieldBuilder AddBranchEmbed(in Status message) + private static DiscordEmbedField AddBranchEmbed(in Status message) { - return new EmbedFieldBuilder() - .WithName("Branch") - .WithValue(message.Branches.Select(static b => b.Name).FirstOrDefault() ?? "(unknown)"); + return new(Name: "Branch", Value: message.Branches.Select(static b => b.Name).FirstOrDefault() ?? "(unknown)"); } - private static EmbedFieldBuilder AddHeadCommitEmbed(in StatusCommit statusCommit) + private static DiscordEmbedField AddHeadCommitEmbed(in StatusCommit statusCommit) { - return new EmbedFieldBuilder() - .WithName("Head commit") - .WithValue($"{statusCommit.Author.Login} - {statusCommit.Commit.Message}"); + return new(Name: "Head commit", Value: $"{statusCommit.Author.Login} - {statusCommit.Commit.Message}"); } - private static Color GetEmbedColor(Status message) + private static DiscordColour? GetEmbedColour(Status message) { return ColourMappings .Where(mapping => mapping.IsMatch(message)) - .Select(mapping => mapping.Colour) - .FirstOrDefault(Color.Default); + .Select(static DiscordColour? (mapping) => mapping.Colour) + .FirstOrDefault(); } [DebuggerDisplay("{State} => {Colour}")] - private readonly record struct ColourMapping(string State, Color Colour) + private readonly record struct ColourMapping(string State, DiscordColour Colour) { public bool IsMatch(in Status message) { diff --git a/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs b/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs index 94e3925a..bee0f447 100644 --- a/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs +++ b/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs @@ -1,3 +1,4 @@ +using System.Linq; using System.Threading; using System.Threading.Tasks; using BuildBot.Discord.Models; @@ -26,4 +27,28 @@ public async Task HandlePublishesBotMessageViaMediatorAsync() await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } + + [Fact] + public async Task PublishedEmbedHasExpectedContentAsync() + { + IMediator mediator = GetSubstitute(); + WatchTowerPublishMessageNotificationHandler handler = new(mediator); + + await handler.Handle( + notification: new(new WatchTowerMessage(Message: "Service deployed", Title: "Deployment")), + cancellationToken: this.CancellationToken() + ); + + DiscordEmbed embed = mediator + .ReceivedCalls() + .Select(static call => call.GetArguments()[0]) + .OfType() + .Single() + .Message; + Assert.Equal(expected: "Deployment", actual: embed.Title); + Assert.Equal(expected: "Service deployed", actual: embed.Description); + Assert.Null(embed.Url); + Assert.Null(embed.Colour); + Assert.Empty(embed.Fields); + } } diff --git a/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs b/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs index 8110b731..d8f5677f 100644 --- a/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs +++ b/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs @@ -2,7 +2,6 @@ using System.Threading.Tasks; using BuildBot.Discord.Models; using BuildBot.Watchtower.Models; -using Discord; using Mediator; namespace BuildBot.Watchtower.Publishers; @@ -21,8 +20,14 @@ public ValueTask Handle(WatchTowerPublishMessage notification, CancellationToken return this._mediator.Publish(new BotMessage(BuildMessage(notification)), cancellationToken: cancellationToken); } - private static EmbedBuilder BuildMessage(WatchTowerPublishMessage notification) + private static DiscordEmbed BuildMessage(WatchTowerPublishMessage notification) { - return new EmbedBuilder().WithTitle(notification.Model.Title).WithDescription(notification.Model.Message); + return new( + Title: notification.Model.Title, + Url: null, + Description: notification.Model.Message, + Colour: null, + Fields: [] + ); } } diff --git a/src/BuildBot/.gitignore b/src/BuildBot/.gitignore new file mode 100644 index 00000000..95914b98 --- /dev/null +++ b/src/BuildBot/.gitignore @@ -0,0 +1 @@ +http-client.private.env.json diff --git a/src/BuildBot/DiscordWebhook.http b/src/BuildBot/DiscordWebhook.http new file mode 100644 index 00000000..a0f48e93 --- /dev/null +++ b/src/BuildBot/DiscordWebhook.http @@ -0,0 +1,29 @@ +# Discord webhook (consumed API). +# The webhook URL contains its token, so it is a credential: supply DiscordWebhookUrl from an +# uncommitted http-client.private.env.json, e.g. { "dev": { "DiscordWebhookUrl": "https://..." } } + +### Discord webhook - execute (expects 204 No Content) +POST {{DiscordWebhookUrl}} +Content-Type: application/json + +{ + "embeds": [ + { + "title": "BuildBot webhook test", + "description": "Test message from DiscordWebhook.http", + "color": 3066993, + "author": { + "name": "FunFair BuildBot", + "url": "https://funfair.io", + "icon_url": "https://s2.coinmarketcap.com/static/img/coins/32x32/1757.png" + }, + "fields": [ + { + "name": "Field", + "value": "Value", + "inline": false + } + ] + } + ] +} diff --git a/src/BuildBot/Helpers/ServerStartup.cs b/src/BuildBot/Helpers/ServerStartup.cs index 0217e032..0fc573c0 100644 --- a/src/BuildBot/Helpers/ServerStartup.cs +++ b/src/BuildBot/Helpers/ServerStartup.cs @@ -68,9 +68,7 @@ public static WebApplication CreateApp(string[] args) IConfigurationRoot config = LoadConfiguration(configPath); DiscordBotConfiguration discordConfig = LoadDiscordConfig(config); - Console.WriteLine($"Discord Server: {discordConfig.Server}"); - Console.WriteLine($"Discord Channel: {discordConfig.Channel}"); - Console.WriteLine($"Discord Release Channel: {discordConfig.ReleaseChannel}"); + Console.WriteLine($"Discord Webhook configured: {(discordConfig.IsConfigured ? "Yes" : "No")}"); SnsNotificationOptions snsConfiguration = LoadSnsNotificationConfig(config); @@ -157,12 +155,7 @@ private static IConfigurationRoot LoadConfiguration(string configPath) private static DiscordBotConfiguration LoadDiscordConfig(IConfigurationRoot configuration) { - return new( - configuration["Discord:Token"] ?? string.Empty, - configuration["Discord:Server"] ?? string.Empty, - configuration["Discord:Channel"] ?? string.Empty, - configuration["Discord:ReleaseChannel"] ?? string.Empty - ); + return DiscordBotConfiguration.FromWebhookUrl(configuration["Discord:WebhookUrl"]); } private static GitHubConfiguration LoadGitHubConfig(IConfigurationRoot configuration) diff --git a/src/BuildBot/appsettings.json b/src/BuildBot/appsettings.json index 96c600e7..139f00ad 100644 --- a/src/BuildBot/appsettings.json +++ b/src/BuildBot/appsettings.json @@ -7,10 +7,7 @@ }, "AllowedHosts": "*", "Discord": { - "Token": "", - "Server": "", - "Channel": "", - "ReleaseChannel": "" + "WebhookUrl": "" }, "CloudFormation": { "TopicArn": "", From 5a31f3901ff28b5d83efbb2514c32f1aa9ad8cc2 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 15:54:13 +0100 Subject: [PATCH 03/39] test(discord): share logged-text helper across Discord tests The logger-argument join helper was duplicated three times in BuildBot.Discord.Tests. It now lives once as LoggedText(this ILogger) in LoggerTestExtensions.cs. Construct: ReceivedCalls().SelectMany(GetArguments) joined into a single logged string - src/BuildBot.Discord.Tests/DiscordSetupTests.cs: held a copy of the join, now calls the shared LoggedText helper - src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs: held a copy of the join, now calls the shared LoggedText helper - src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs: held a copy of the join, now calls the shared LoggedText helper - src/BuildBot.Discord.Tests/LoggerTestExtensions.cs: new home of the shared LoggedText(this ILogger) helper Refs #423 --- .../DiscordSetupTests.cs | 16 ++-------------- .../LoggerTestExtensions.cs | 19 +++++++++++++++++++ .../Services/DiscordBotTests.cs | 10 +--------- .../Services/DiscordWebhookClientTests.cs | 16 ++-------------- 4 files changed, 24 insertions(+), 37 deletions(-) create mode 100644 src/BuildBot.Discord.Tests/LoggerTestExtensions.cs diff --git a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs index 26b889d0..1f83156e 100644 --- a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs +++ b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; @@ -63,7 +62,7 @@ public async Task WebhookHttpClient_DoesNotLogRequestUrl() Assert.Contains( expectedSubstring: SECRET, - actualString: LoggedText(httpLogger), + actualString: httpLogger.LoggedText(), comparisonType: StringComparison.Ordinal ); httpLogger.ClearReceivedCalls(); @@ -78,7 +77,7 @@ public async Task WebhookHttpClient_DoesNotLogRequestUrl() Assert.DoesNotContain( expectedSubstring: SECRET, - actualString: LoggedText(httpLogger), + actualString: httpLogger.LoggedText(), comparisonType: StringComparison.Ordinal ); } @@ -125,15 +124,4 @@ private static HttpMessageHandler CreateNoContentHandler() { return new TestHttpMessageHandler(static () => new HttpResponseMessage(HttpStatusCode.NoContent)); } - - private static string LoggedText(ILogger logger) - { - return string.Join( - separator: '\n', - values: logger - .ReceivedCalls() - .SelectMany(static call => call.GetArguments()) - .Select(static arg => arg?.ToString()) - ); - } } diff --git a/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs b/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs new file mode 100644 index 00000000..ef5e5d21 --- /dev/null +++ b/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs @@ -0,0 +1,19 @@ +using System.Linq; +using Microsoft.Extensions.Logging; +using NSubstitute; + +namespace BuildBot.Discord.Tests; + +internal static class LoggerTestExtensions +{ + public static string LoggedText(this ILogger logger) + { + return string.Join( + separator: '\n', + values: logger + .ReceivedCalls() + .SelectMany(static call => call.GetArguments()) + .Select(static arg => arg?.ToString()) + ); + } +} diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs index 3056a57d..56f38b2e 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs @@ -57,17 +57,9 @@ private static void MockDiscordWebhookClientSend(IDiscordWebhookClient client, i private static void AssertSecretNeverLogged(ILogger logger) { - string logged = string.Join( - separator: '\n', - values: logger - .ReceivedCalls() - .SelectMany(static call => call.GetArguments()) - .Select(static arg => arg?.ToString()) - ); - Assert.DoesNotContain( expectedSubstring: SECRET, - actualString: logged, + actualString: logger.LoggedText(), comparisonType: StringComparison.Ordinal ); } diff --git a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs index 8583f287..35dddd8b 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Net; using System.Net.Http; using System.Net.Http.Headers; @@ -140,7 +139,7 @@ public async Task SendAsync_WhenRateLimited_RetriesAndSucceeds() Assert.All(collection: handler.Requests, action: r => Assert.Equal(expected: EXPECTED_JSON, actual: r.Body)); Assert.Contains( expectedSubstring: "retrying in 0.001 seconds", - actualString: LoggedText(logger), + actualString: logger.LoggedText(), comparisonType: StringComparison.Ordinal ); } @@ -165,7 +164,7 @@ public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries( Assert.Equal(expected: 2, actual: handler.Requests.Count); Assert.Contains( expectedSubstring: "retrying in 2 seconds", - actualString: LoggedText(logger), + actualString: logger.LoggedText(), comparisonType: StringComparison.Ordinal ); } @@ -206,17 +205,6 @@ await client.SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationTok ); } - private static string LoggedText(ILogger logger) - { - return string.Join( - separator: '\n', - values: logger - .ReceivedCalls() - .SelectMany(static call => call.GetArguments()) - .Select(static arg => arg?.ToString()) - ); - } - public static TheoryData RateLimitDelays => new() { From ca91c2cd8493e30fdbac18f6d851e67a92786a74 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:01:36 +0100 Subject: [PATCH 04/39] fix(discord): only accept http(s) embed URLs EmbedUrl.FromString returned any absolute Uri; on Linux a relative path such as "/builds/123" parses as file:///builds/123 and Discord rejects non-http(s) embed URLs with 400, dropping the whole message. Now returns null unless the scheme is http or https. Pattern Sweep: searched non-test src for Uri.TryCreate/UriKind.Absolute/new Uri( parsing external URLs without a scheme check; 2 found, 1 changed (EmbedUrl.cs); DiscordBotConfiguration.FromWebhookUrl already enforces https so no change needed. Refs #423. --- .../Models/EmbedUrlTests.cs | 15 ++++++++++----- src/BuildBot.Discord/Models/EmbedUrl.cs | 9 ++++++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs b/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs index 886407ca..6d38939b 100644 --- a/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs +++ b/src/BuildBot.Discord.Tests/Models/EmbedUrlTests.cs @@ -7,19 +7,24 @@ namespace BuildBot.Discord.Tests.Models; public sealed class EmbedUrlTests : TestBase { - [Fact] - public void FromString_WithAbsoluteUrl_ReturnsUri() + [Theory] + [InlineData("https://example.com/build/1")] + [InlineData("http://example.com/build/1")] + public static void FromString_WithWebUrl_ReturnsUri(string value) { - Uri? uri = EmbedUrl.FromString("https://example.com/build/1"); + Uri? uri = EmbedUrl.FromString(value); - Assert.Equal(expected: new Uri("https://example.com/build/1"), actual: uri); + Assert.Equal(expected: new Uri(value), actual: uri); } [Theory] [InlineData(null)] [InlineData("")] [InlineData("not a url")] - public static void FromString_WithMissingOrRelativeUrl_ReturnsNull(string? value) + [InlineData("/builds/123")] + [InlineData("ftp://example.com/build/1")] + [InlineData("file:///builds/123")] + public static void FromString_WithMissingRelativeOrNonWebUrl_ReturnsNull(string? value) { Assert.Null(EmbedUrl.FromString(value)); } diff --git a/src/BuildBot.Discord/Models/EmbedUrl.cs b/src/BuildBot.Discord/Models/EmbedUrl.cs index 918a94f9..6b7a499e 100644 --- a/src/BuildBot.Discord/Models/EmbedUrl.cs +++ b/src/BuildBot.Discord/Models/EmbedUrl.cs @@ -6,6 +6,13 @@ public static class EmbedUrl { public static Uri? FromString(string? value) { - return Uri.TryCreate(uriString: value, uriKind: UriKind.Absolute, out Uri? uri) ? uri : null; + return Uri.TryCreate(uriString: value, uriKind: UriKind.Absolute, out Uri? uri) && IsWebUrl(uri) ? uri : null; + } + + // Discord rejects the whole message when an embed URL is not http(s); on Linux "/path" parses as a file URI. + private static bool IsWebUrl(Uri uri) + { + return StringComparer.Ordinal.Equals(x: uri.Scheme, y: Uri.UriSchemeHttps) + || StringComparer.Ordinal.Equals(x: uri.Scheme, y: Uri.UriSchemeHttp); } } From ce0d31f21cc46b4f5c57d3dd24abea348b4ebda7 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:12:49 +0100 Subject: [PATCH 05/39] test(discord): use a fake clock in webhook rate-limit tests The 429 retry tests injected TimeProvider.System and waited in real time. They now use FunFair.Test.Infrastructure MockDateTimeSources.Past (a FakeTimeProvider, no new package) and advance it past the retry delay, asserting no retry before the delay and exactly one after. Pattern Sweep: searched all test projects for TimeProvider.System, Task.Delay, Thread.Sleep, DateTime(Offset).(Utc)Now, Stopwatch, SpinWait. Fixed DiscordWebhookClientTests.cs. Excluded DiscordSetup.cs (production DI registration). BotServiceTests.cs real Task.Delay waits are a different construct (BotService has no time abstraction; needs a production change) and are not changed here. Refs #423. --- .../Services/DiscordWebhookClientTests.cs | 105 +++++++++++++----- 1 file changed, 80 insertions(+), 25 deletions(-) diff --git a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs index 35dddd8b..e1432604 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs @@ -8,7 +8,9 @@ using BuildBot.Discord.Services; using BuildBot.Discord.Services.Models; using FunFair.Test.Common; +using FunFair.Test.Infrastructure.Mocks; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Time.Testing; using NSubstitute; using Xunit; @@ -32,9 +34,11 @@ public sealed class DiscordWebhookClientTests : TestBase private const string EXPECTED_JSON = """{"embeds":[{"title":"Title","url":"https://example.com/build","color":3066993,"author":{"name":"Author","url":"https://example.com/","icon_url":"https://example.com/icon.png"},"fields":[{"name":"Name","value":"Value","inline":true}]}]}"""; - private (DiscordWebhookClient Client, ILogger Logger) CreateClient( - TestHttpMessageHandler handler - ) + private ( + DiscordWebhookClient Client, + ILogger Logger, + FakeTimeProvider TimeProvider + ) CreateClient(TestHttpMessageHandler handler) { IHttpClientFactory factory = GetSubstitute(); factory @@ -42,8 +46,9 @@ TestHttpMessageHandler handler .Returns(_ => new HttpClient(handler: handler, disposeHandler: false)); ILogger logger = this.GetTypedLogger(); + FakeTimeProvider timeProvider = MockDateTimeSources.Past; - return (new(httpClientFactory: factory, timeProvider: TimeProvider.System, logger: logger), logger); + return (new(httpClientFactory: factory, timeProvider: timeProvider, logger: logger), logger, timeProvider); } private static Func Respond(HttpStatusCode statusCode) @@ -73,11 +78,28 @@ private static Func RateLimited( }; } + private static void AdvancePastRateLimitDelay( + FakeTimeProvider timeProvider, + TestHttpMessageHandler handler, + Task sending, + in TimeSpan expectedDelay + ) + { + int requestsBeforeDelay = handler.Requests.Count; + + timeProvider.Advance(expectedDelay - TimeSpan.FromTicks(1)); + Assert.False(condition: sending.IsCompleted, userMessage: "Retried before the rate limit delay elapsed"); + Assert.Equal(expected: requestsBeforeDelay, actual: handler.Requests.Count); + + timeProvider.Advance(TimeSpan.FromTicks(1)); + Assert.Equal(expected: requestsBeforeDelay + 1, actual: handler.Requests.Count); + } + [Fact] public async Task SendAsync_PostsPayloadAsJsonToWebhookUrl() { using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); - (DiscordWebhookClient client, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = this.CreateClient(handler); WebhookSendResult result = await client.SendAsync( webhookUrl: WebhookUrl, @@ -106,7 +128,7 @@ bool expectedSuccess ) { using TestHttpMessageHandler handler = new(Respond(statusCode)); - (DiscordWebhookClient client, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = this.CreateClient(handler); WebhookSendResult result = await client.SendAsync( webhookUrl: WebhookUrl, @@ -122,23 +144,32 @@ bool expectedSuccess public async Task SendAsync_WhenRateLimited_RetriesAndSucceeds() { using TestHttpMessageHandler handler = new( - RateLimited("""{"message":"You are being rate limited.","retry_after":0.001,"global":false}"""), + RateLimited("""{"message":"You are being rate limited.","retry_after":1.5,"global":false}"""), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, ILogger logger) = this.CreateClient(handler); + (DiscordWebhookClient client, ILogger logger, FakeTimeProvider timeProvider) = + this.CreateClient(handler); logger.IsEnabled(Arg.Any()).Returns(true); - WebhookSendResult result = await client.SendAsync( - webhookUrl: WebhookUrl, - payload: Payload, - cancellationToken: this.CancellationToken() + Task sending = client + .SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: this.CancellationToken()) + .AsTask(); + + Assert.Single(handler.Requests); + AdvancePastRateLimitDelay( + timeProvider: timeProvider, + handler: handler, + sending: sending, + expectedDelay: TimeSpan.FromSeconds(1.5) ); + WebhookSendResult result = await sending; + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); Assert.Equal(expected: 2, actual: handler.Requests.Count); Assert.All(collection: handler.Requests, action: r => Assert.Equal(expected: EXPECTED_JSON, actual: r.Body)); Assert.Contains( - expectedSubstring: "retrying in 0.001 seconds", + expectedSubstring: "retrying in 1.5 seconds", actualString: logger.LoggedText(), comparisonType: StringComparison.Ordinal ); @@ -151,15 +182,24 @@ public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries( RateLimited(body: "rate limited", retryAfter: TimeSpan.FromSeconds(2), mediaType: "text/html"), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, ILogger logger) = this.CreateClient(handler); + (DiscordWebhookClient client, ILogger logger, FakeTimeProvider timeProvider) = + this.CreateClient(handler); logger.IsEnabled(Arg.Any()).Returns(true); - WebhookSendResult result = await client.SendAsync( - webhookUrl: WebhookUrl, - payload: Payload, - cancellationToken: this.CancellationToken() + Task sending = client + .SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: this.CancellationToken()) + .AsTask(); + + Assert.Single(handler.Requests); + AdvancePastRateLimitDelay( + timeProvider: timeProvider, + handler: handler, + sending: sending, + expectedDelay: TimeSpan.FromSeconds(2) ); + WebhookSendResult result = await sending; + Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); Assert.Equal(expected: 2, actual: handler.Requests.Count); Assert.Contains( @@ -172,21 +212,36 @@ public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries( [Fact] public async Task SendAsync_WhenRateLimitedOnEveryAttempt_GivesUpAfterMaximumAttempts() { - const string body = """{"retry_after":0.001}"""; + const string body = """{"retry_after":5}"""; using TestHttpMessageHandler handler = new( RateLimited(body), RateLimited(body), RateLimited(body), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, FakeTimeProvider timeProvider) = this.CreateClient(handler); + TimeSpan expectedDelay = TimeSpan.FromSeconds(5); - WebhookSendResult result = await client.SendAsync( - webhookUrl: WebhookUrl, - payload: Payload, - cancellationToken: this.CancellationToken() + Task sending = client + .SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: this.CancellationToken()) + .AsTask(); + + Assert.Single(handler.Requests); + AdvancePastRateLimitDelay( + timeProvider: timeProvider, + handler: handler, + sending: sending, + expectedDelay: expectedDelay + ); + AdvancePastRateLimitDelay( + timeProvider: timeProvider, + handler: handler, + sending: sending, + expectedDelay: expectedDelay ); + WebhookSendResult result = await sending; + Assert.Equal(expected: new(Success: false, StatusCode: HttpStatusCode.TooManyRequests), actual: result); Assert.Equal(expected: 3, actual: handler.Requests.Count); } @@ -195,7 +250,7 @@ public async Task SendAsync_WhenRateLimitedOnEveryAttempt_GivesUpAfterMaximumAtt public async Task SendAsync_WhenCancelled_Throws() { using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); - (DiscordWebhookClient client, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = this.CreateClient(handler); using CancellationTokenSource cts = new(); await cts.CancelAsync(); From ff79dee9784317bdd38030adbf3c74692d76eea3 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:23:08 +0100 Subject: [PATCH 06/39] test(discord): use CapturingLogger instead of custom LoggedText helper Replaces the hand-rolled LoggedText(ILogger) ReceivedCalls() join with FunFair.Test.Infrastructure CapturingLogger (no new package); secret-never-logged and retry-log assertions now inspect CapturingLogger Entries. TestLoggerExtension.Received/DidNotReceive need exact full-message matches so were not used. Pattern Sweep: construct = hand-rolled ILogger ReceivedCalls() inspection. Fixed LoggerTestExtensions.cs, its three call sites and LogCallsAt in DiscordBotTests. Other ReceivedCalls() uses (CloudFormation, GitHub, Watchtower publisher tests, BotServiceTests) inspect IMediator/IDiscordBot, not ILogger, so are a different construct. Refs #423. --- .../DiscordSetupTests.cs | 23 +++++----- .../LoggerTestExtensions.cs | 19 -------- .../Services/DiscordBotTests.cs | 43 ++++++++----------- .../Services/DiscordWebhookClientTests.cs | 37 ++++++++-------- 4 files changed, 47 insertions(+), 75 deletions(-) delete mode 100644 src/BuildBot.Discord.Tests/LoggerTestExtensions.cs diff --git a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs index 1f83156e..6c184205 100644 --- a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs +++ b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; @@ -7,6 +8,7 @@ using BuildBot.Discord.Services.Models; using BuildBot.Discord.Tests.Services; using FunFair.Test.Common; +using FunFair.Test.Infrastructure.Mocks; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NSubstitute; @@ -35,8 +37,7 @@ public static void CalculateRetryDelay_IsExponential(int attempt, int expectedSe [Fact] public async Task WebhookHttpClient_DoesNotLogRequestUrl() { - ILogger httpLogger = GetSubstitute(); - httpLogger.IsEnabled(Arg.Any()).Returns(true); + CapturingLogger httpLogger = new(); ILoggerProvider loggerProvider = GetSubstitute(); loggerProvider.CreateLogger(Arg.Any()).Returns(httpLogger); @@ -60,12 +61,8 @@ public async Task WebhookHttpClient_DoesNotLogRequestUrl() ); } - Assert.Contains( - expectedSubstring: SECRET, - actualString: httpLogger.LoggedText(), - comparisonType: StringComparison.Ordinal - ); - httpLogger.ClearReceivedCalls(); + Assert.Contains(collection: httpLogger.Entries, filter: static entry => ContainsSecret(entry)); + int controlEntryCount = httpLogger.EntryCount; using (HttpClient client = factory.CreateClient(DiscordWebhookClient.HttpClientName)) { @@ -76,12 +73,16 @@ public async Task WebhookHttpClient_DoesNotLogRequestUrl() } Assert.DoesNotContain( - expectedSubstring: SECRET, - actualString: httpLogger.LoggedText(), - comparisonType: StringComparison.Ordinal + collection: httpLogger.Entries.Skip(controlEntryCount), + filter: static entry => ContainsSecret(entry) ); } + private static bool ContainsSecret(in CapturedLogEntry entry) + { + return entry.Message.Contains(value: SECRET, comparisonType: StringComparison.Ordinal); + } + [Fact] public async Task WebhookClient_RetriesTransientServerErrorsWithSamePayload() { diff --git a/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs b/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs deleted file mode 100644 index ef5e5d21..00000000 --- a/src/BuildBot.Discord.Tests/LoggerTestExtensions.cs +++ /dev/null @@ -1,19 +0,0 @@ -using System.Linq; -using Microsoft.Extensions.Logging; -using NSubstitute; - -namespace BuildBot.Discord.Tests; - -internal static class LoggerTestExtensions -{ - public static string LoggedText(this ILogger logger) - { - return string.Join( - separator: '\n', - values: logger - .ReceivedCalls() - .SelectMany(static call => call.GetArguments()) - .Select(static arg => arg?.ToString()) - ); - } -} diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs index 56f38b2e..87dd4f51 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs @@ -9,9 +9,9 @@ using BuildBot.Discord.Services.Models; using BuildBot.ServiceModel.ComponentStatus; using FunFair.Test.Common; +using FunFair.Test.Infrastructure.Mocks; using Microsoft.Extensions.Logging; using NSubstitute; -using NSubstitute.Core; using NSubstitute.ExceptionExtensions; using Xunit; @@ -31,13 +31,12 @@ public sealed class DiscordBotTests : TestBase Fields: [new(Name: "Name", Value: "Value")] ); - private (IDiscordWebhookClient Client, ILogger Logger, DiscordBot Bot) CreateBot( + private static (IDiscordWebhookClient Client, CapturingLogger Logger, DiscordBot Bot) CreateBot( DiscordBotConfiguration configuration ) { IDiscordWebhookClient client = GetSubstitute(); - ILogger logger = this.GetTypedLogger(); - logger.IsEnabled(Arg.Any()).Returns(true); + CapturingLogger logger = new(); DiscordBot bot = new(client: client, botConfiguration: configuration, logger: logger); @@ -55,31 +54,25 @@ private static void MockDiscordWebhookClientSend(IDiscordWebhookClient client, i .Returns(result); } - private static void AssertSecretNeverLogged(ILogger logger) + private static void AssertSecretNeverLogged(CapturingLogger logger) { Assert.DoesNotContain( - expectedSubstring: SECRET, - actualString: logger.LoggedText(), - comparisonType: StringComparison.Ordinal + collection: logger.Entries, + filter: static entry => entry.Message.Contains(value: SECRET, comparisonType: StringComparison.Ordinal) ); } - private static int LogCallsAt(LogLevel level, ILogger logger) + private static int LogCallsAt(LogLevel level, CapturingLogger logger) { - return logger - .ReceivedCalls() - .Count(call => - StringComparer.Ordinal.Equals(x: call.GetMethodInfo().Name, y: nameof(ILogger.Log)) - && Equals(call.GetArguments()[0], level) - ); + return logger.Entries.Count(entry => entry.Level == level); } [Theory] [InlineData("https://example.com/webhook/secret-token", true)] [InlineData("http://example.com/webhook/secret-token", false)] - public void GetStatus_ReflectsConfiguration(string webhookUrl, bool expectedOk) + public static void GetStatus_ReflectsConfiguration(string webhookUrl, bool expectedOk) { - (_, _, DiscordBot bot) = this.CreateBot(new(new Uri(webhookUrl))); + (_, _, DiscordBot bot) = CreateBot(new(new Uri(webhookUrl))); ServiceStatus status = bot.GetStatus(); @@ -87,9 +80,9 @@ public void GetStatus_ReflectsConfiguration(string webhookUrl, bool expectedOk) } [Fact] - public void GetStatus_WithRelativeUrl_IsNotOk() + public static void GetStatus_WithRelativeUrl_IsNotOk() { - (_, _, DiscordBot bot) = this.CreateBot(new(new Uri(uriString: "webhook", uriKind: UriKind.Relative))); + (_, _, DiscordBot bot) = CreateBot(new(new Uri(uriString: "webhook", uriKind: UriKind.Relative))); Assert.False(condition: bot.GetStatus().Ok, userMessage: "Relative webhook URL should not be Ok"); } @@ -97,7 +90,7 @@ public void GetStatus_WithRelativeUrl_IsNotOk() [Fact] public async Task PublishAsync_SendsMappedEmbedToWebhookAndLogsSuccess() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); WebhookPayload? captured = null; client .SendAsync( @@ -122,7 +115,7 @@ public async Task PublishAsync_SendsMappedEmbedToWebhookAndLogsSuccess() [Fact] public async Task PublishAsync_WhenWebhookRejectsMessage_LogsError() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); MockDiscordWebhookClientSend( client: client, result: new(Success: false, StatusCode: HttpStatusCode.BadRequest) @@ -137,7 +130,7 @@ public async Task PublishAsync_WhenWebhookRejectsMessage_LogsError() [Fact] public async Task PublishAsync_WhenClientThrows_LogsErrorAndDoesNotThrow() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); client .SendAsync( webhookUrl: Arg.Any(), @@ -155,7 +148,7 @@ public async Task PublishAsync_WhenClientThrows_LogsErrorAndDoesNotThrow() [Fact] public async Task PublishAsync_WhenCancelled_LogsWarningAndDoesNotThrow() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); using CancellationTokenSource cts = new(); await cts.CancelAsync(); client @@ -175,7 +168,7 @@ public async Task PublishAsync_WhenCancelled_LogsWarningAndDoesNotThrow() [Fact] public async Task PublishAsync_WhenClientCancelsWithoutCallerCancellation_LogsError() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot(Config); + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); client .SendAsync( webhookUrl: Arg.Any(), @@ -192,7 +185,7 @@ public async Task PublishAsync_WhenClientCancelsWithoutCallerCancellation_LogsEr [Fact] public async Task PublishAsync_WhenNotConfigured_LogsErrorWithoutSending() { - (IDiscordWebhookClient client, ILogger logger, DiscordBot bot) = this.CreateBot( + (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot( new(new Uri("http://example.com/webhook/" + SECRET)) ); diff --git a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs index e1432604..00594f6a 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordWebhookClientTests.cs @@ -9,7 +9,6 @@ using BuildBot.Discord.Services.Models; using FunFair.Test.Common; using FunFair.Test.Infrastructure.Mocks; -using Microsoft.Extensions.Logging; using Microsoft.Extensions.Time.Testing; using NSubstitute; using Xunit; @@ -34,9 +33,9 @@ public sealed class DiscordWebhookClientTests : TestBase private const string EXPECTED_JSON = """{"embeds":[{"title":"Title","url":"https://example.com/build","color":3066993,"author":{"name":"Author","url":"https://example.com/","icon_url":"https://example.com/icon.png"},"fields":[{"name":"Name","value":"Value","inline":true}]}]}"""; - private ( + private static ( DiscordWebhookClient Client, - ILogger Logger, + CapturingLogger Logger, FakeTimeProvider TimeProvider ) CreateClient(TestHttpMessageHandler handler) { @@ -45,7 +44,7 @@ FakeTimeProvider TimeProvider .CreateClient(DiscordWebhookClient.HttpClientName) .Returns(_ => new HttpClient(handler: handler, disposeHandler: false)); - ILogger logger = this.GetTypedLogger(); + CapturingLogger logger = new(); FakeTimeProvider timeProvider = MockDateTimeSources.Past; return (new(httpClientFactory: factory, timeProvider: timeProvider, logger: logger), logger, timeProvider); @@ -99,7 +98,7 @@ in TimeSpan expectedDelay public async Task SendAsync_PostsPayloadAsJsonToWebhookUrl() { using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); - (DiscordWebhookClient client, _, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = CreateClient(handler); WebhookSendResult result = await client.SendAsync( webhookUrl: WebhookUrl, @@ -128,7 +127,7 @@ bool expectedSuccess ) { using TestHttpMessageHandler handler = new(Respond(statusCode)); - (DiscordWebhookClient client, _, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = CreateClient(handler); WebhookSendResult result = await client.SendAsync( webhookUrl: WebhookUrl, @@ -147,9 +146,8 @@ public async Task SendAsync_WhenRateLimited_RetriesAndSucceeds() RateLimited("""{"message":"You are being rate limited.","retry_after":1.5,"global":false}"""), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, ILogger logger, FakeTimeProvider timeProvider) = - this.CreateClient(handler); - logger.IsEnabled(Arg.Any()).Returns(true); + (DiscordWebhookClient client, CapturingLogger logger, FakeTimeProvider timeProvider) = + CreateClient(handler); Task sending = client .SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: this.CancellationToken()) @@ -169,9 +167,9 @@ public async Task SendAsync_WhenRateLimited_RetriesAndSucceeds() Assert.Equal(expected: 2, actual: handler.Requests.Count); Assert.All(collection: handler.Requests, action: r => Assert.Equal(expected: EXPECTED_JSON, actual: r.Body)); Assert.Contains( - expectedSubstring: "retrying in 1.5 seconds", - actualString: logger.LoggedText(), - comparisonType: StringComparison.Ordinal + collection: logger.Entries, + filter: static entry => + entry.Message.Contains(value: "retrying in 1.5 seconds", comparisonType: StringComparison.Ordinal) ); } @@ -182,9 +180,8 @@ public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries( RateLimited(body: "rate limited", retryAfter: TimeSpan.FromSeconds(2), mediaType: "text/html"), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, ILogger logger, FakeTimeProvider timeProvider) = - this.CreateClient(handler); - logger.IsEnabled(Arg.Any()).Returns(true); + (DiscordWebhookClient client, CapturingLogger logger, FakeTimeProvider timeProvider) = + CreateClient(handler); Task sending = client .SendAsync(webhookUrl: WebhookUrl, payload: Payload, cancellationToken: this.CancellationToken()) @@ -203,9 +200,9 @@ public async Task SendAsync_WhenRateLimitedWithNonJsonBody_UsesHeaderAndRetries( Assert.Equal(expected: new(Success: true, StatusCode: HttpStatusCode.NoContent), actual: result); Assert.Equal(expected: 2, actual: handler.Requests.Count); Assert.Contains( - expectedSubstring: "retrying in 2 seconds", - actualString: logger.LoggedText(), - comparisonType: StringComparison.Ordinal + collection: logger.Entries, + filter: static entry => + entry.Message.Contains(value: "retrying in 2 seconds", comparisonType: StringComparison.Ordinal) ); } @@ -219,7 +216,7 @@ public async Task SendAsync_WhenRateLimitedOnEveryAttempt_GivesUpAfterMaximumAtt RateLimited(body), Respond(HttpStatusCode.NoContent) ); - (DiscordWebhookClient client, _, FakeTimeProvider timeProvider) = this.CreateClient(handler); + (DiscordWebhookClient client, _, FakeTimeProvider timeProvider) = CreateClient(handler); TimeSpan expectedDelay = TimeSpan.FromSeconds(5); Task sending = client @@ -250,7 +247,7 @@ public async Task SendAsync_WhenRateLimitedOnEveryAttempt_GivesUpAfterMaximumAtt public async Task SendAsync_WhenCancelled_Throws() { using TestHttpMessageHandler handler = new(Respond(HttpStatusCode.NoContent)); - (DiscordWebhookClient client, _, _) = this.CreateClient(handler); + (DiscordWebhookClient client, _, _) = CreateClient(handler); using CancellationTokenSource cts = new(); await cts.CancelAsync(); From c5a13eb4106323d08309c6fe9adc563ab84a11e4 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:28:13 +0100 Subject: [PATCH 07/39] test(github): share PublishedEmbed helper across GitHub publisher tests The PublishedEmbed(IMediator) helper was duplicated in both GitHub publisher test classes; it is now one internal extension in BuildBot.GitHub.Tests. No FunFair.Test.* equivalent exists. Pattern Sweep: construct = private helper extracting the published DiscordEmbed from an IMediator substitute's received calls. Fixed the two BuildBot.GitHub.Tests copies. Found, not changed: CloudFormationMessageReceivedNotificationHandlerTests.cs (private PublishedEmbed) and WatchTowerPublishMessageNotificationHandlerTests.cs (inline chain) are in other test projects; sharing would need a new test-support project. Refs #423. --- .../GithubPushNotificationHandlerTests.cs | 15 ++------------- .../GithubStatusNotificationHandlerTests.cs | 15 ++------------- .../Publishers/MediatorTestExtensions.cs | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+), 26 deletions(-) create mode 100644 src/BuildBot.GitHub.Tests/Publishers/MediatorTestExtensions.cs diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs index c7fc9aa6..fec0eae4 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Threading; using System.Threading.Tasks; using BuildBot.Discord.Models; @@ -187,16 +186,6 @@ public async Task SingleCommitWithNullUsernameShouldPublishAsync() await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } - private static DiscordEmbed PublishedEmbed(IMediator mediator) - { - return mediator - .ReceivedCalls() - .Select(static call => call.GetArguments()[0]) - .OfType() - .Single() - .Message; - } - [Fact] public async Task PublishedEmbedHasExpectedContentAsync() { @@ -208,7 +197,7 @@ public async Task PublishedEmbedHasExpectedContentAsync() await handler.Handle(notification: new(push), cancellationToken: this.CancellationToken()); - DiscordEmbed embed = PublishedEmbed(mediator); + DiscordEmbed embed = mediator.PublishedEmbed(); Assert.Equal(expected: "testpusher pushed 2 commits to BuildBot (feature/my-feature)", actual: embed.Title); Assert.Equal(expected: new Uri("https://github.com/example/repo/compare/before...after"), actual: embed.Url); Assert.Null(embed.Description); @@ -234,7 +223,7 @@ public async Task PublishedEmbedForSingleCommitUsesSingularTitleAsync() Assert.Equal( expected: "testpusher pushed 1 commit to BuildBot (feature/my-feature)", - actual: PublishedEmbed(mediator).Title + actual: mediator.PublishedEmbed().Title ); } } diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs index 174de64e..03207fba 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs @@ -1,5 +1,4 @@ using System; -using System.Linq; using System.Threading; using System.Threading.Tasks; using BuildBot.Discord.Models; @@ -121,16 +120,6 @@ public async Task NonPendingStateWithNoBranchesShouldPublishAsync(string state) await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } - private static DiscordEmbed PublishedEmbed(IMediator mediator) - { - return mediator - .ReceivedCalls() - .Select(static call => call.GetArguments()[0]) - .OfType() - .Single() - .Message; - } - public static TheoryData StateColours => new() { @@ -148,7 +137,7 @@ public async Task PublishedEmbedHasExpectedContentAsync(string state, uint? expe await handler.Handle(notification: new(MakeStatus(state: state)), cancellationToken: this.CancellationToken()); - DiscordEmbed embed = PublishedEmbed(mediator); + DiscordEmbed embed = mediator.PublishedEmbed(); Assert.Equal(expected: "Build finished for ci/build from BuildBot (main)", actual: embed.Title); Assert.Equal(expected: new Uri("https://ci.example.com/builds/123"), actual: embed.Url); Assert.Equal(expected: "Built at abc123", actual: embed.Description); @@ -173,7 +162,7 @@ await handler.Handle( cancellationToken: this.CancellationToken() ); - DiscordEmbed embed = PublishedEmbed(mediator); + DiscordEmbed embed = mediator.PublishedEmbed(); Assert.Equal(expected: "Build finished for ci/build from BuildBot ((unknown))", actual: embed.Title); Assert.Equal(expected: new DiscordEmbedField(Name: "Branch", Value: "(unknown)"), actual: embed.Fields[1]); } diff --git a/src/BuildBot.GitHub.Tests/Publishers/MediatorTestExtensions.cs b/src/BuildBot.GitHub.Tests/Publishers/MediatorTestExtensions.cs new file mode 100644 index 00000000..0ffb086d --- /dev/null +++ b/src/BuildBot.GitHub.Tests/Publishers/MediatorTestExtensions.cs @@ -0,0 +1,19 @@ +using System.Linq; +using BuildBot.Discord.Models; +using Mediator; +using NSubstitute; + +namespace BuildBot.GitHub.Tests.Publishers; + +internal static class MediatorTestExtensions +{ + public static DiscordEmbed PublishedEmbed(this IMediator mediator) + { + return mediator + .ReceivedCalls() + .Select(static call => call.GetArguments()[0]) + .OfType() + .Single() + .Message; + } +} From 839bde72941bcfeb022f3756181616fa0a7ee337 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:32:59 +0100 Subject: [PATCH 08/39] test(discord): extract MockDiscordWebhookClientSendThrows helper Three DiscordBot tests repeated the IDiscordWebhookClient.SendAsync(Arg.Any...).ThrowsAsync setup inline; they now use a MockDiscordWebhookClientSendThrows helper alongside MockDiscordWebhookClientSend. Pattern Sweep: construct = repeated inline NSubstitute setup of IDiscordWebhookClient.SendAsync. Fixed the 3 DiscordBotTests hits. Not changed: BotServiceTests FailedSend test (single call-counted throw-then-succeed setup, different construct), DiscordSetupTests (real client call), DiscordWebhookClientTests (concrete client, no mock). Construct: repeated inline NSubstitute setup of IDiscordWebhookClient.SendAsync Swept: src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs (3 inline throwing setups) Refs #423. --- .../Services/DiscordBotTests.cs | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs index 87dd4f51..55c61553 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs @@ -54,6 +54,17 @@ private static void MockDiscordWebhookClientSend(IDiscordWebhookClient client, i .Returns(result); } + private static void MockDiscordWebhookClientSendThrows(IDiscordWebhookClient client, Exception exception) + { + client + .SendAsync( + webhookUrl: Arg.Any(), + payload: Arg.Any(), + cancellationToken: Arg.Any() + ) + .ThrowsAsync(exception); + } + private static void AssertSecretNeverLogged(CapturingLogger logger) { Assert.DoesNotContain( @@ -131,13 +142,7 @@ public async Task PublishAsync_WhenWebhookRejectsMessage_LogsError() public async Task PublishAsync_WhenClientThrows_LogsErrorAndDoesNotThrow() { (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); - client - .SendAsync( - webhookUrl: Arg.Any(), - payload: Arg.Any(), - cancellationToken: Arg.Any() - ) - .ThrowsAsync(new HttpRequestException("connection failed")); + MockDiscordWebhookClientSendThrows(client: client, exception: new HttpRequestException("connection failed")); await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); @@ -151,13 +156,7 @@ public async Task PublishAsync_WhenCancelled_LogsWarningAndDoesNotThrow() (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); using CancellationTokenSource cts = new(); await cts.CancelAsync(); - client - .SendAsync( - webhookUrl: Arg.Any(), - payload: Arg.Any(), - cancellationToken: Arg.Any() - ) - .ThrowsAsync(new OperationCanceledException(cts.Token)); + MockDiscordWebhookClientSendThrows(client: client, exception: new OperationCanceledException(cts.Token)); await bot.PublishAsync(embed: Embed, cancellationToken: cts.Token); @@ -169,13 +168,7 @@ public async Task PublishAsync_WhenCancelled_LogsWarningAndDoesNotThrow() public async Task PublishAsync_WhenClientCancelsWithoutCallerCancellation_LogsError() { (IDiscordWebhookClient client, CapturingLogger logger, DiscordBot bot) = CreateBot(Config); - client - .SendAsync( - webhookUrl: Arg.Any(), - payload: Arg.Any(), - cancellationToken: Arg.Any() - ) - .ThrowsAsync(new TaskCanceledException("timed out")); + MockDiscordWebhookClientSendThrows(client: client, exception: new TaskCanceledException("timed out")); await bot.PublishAsync(embed: Embed, cancellationToken: this.CancellationToken()); From f4ff73bd85887ecda9e91b031efab73f82d69069 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:38:12 +0100 Subject: [PATCH 09/39] test(discord): share title-only embed factory across Discord tests CreateEmbed(string title) was duplicated in BotServiceTests and BotMessageTests; both now use TestEmbeds.WithTitle. Pattern Sweep: construct = hand-built title-only test DiscordEmbed. Folded in DiscordSetupTests and DiscordBotMessageNotificationHandlerTests inline embeds. Not changed: EmbedMapperTests (deliberate empty description / colour cases), DiscordBotTests and DiscordWebhookClientTests (fully populated embeds); no title-only factories in other test projects. Refs #423. --- src/BuildBot.Discord.Tests/DiscordSetupTests.cs | 4 +--- .../Models/BotMessageTests.cs | 13 ++++--------- .../DiscordBotMessageNotificationHandlerTests.cs | 4 +--- .../Services/BotServiceTests.cs | 13 ++++--------- src/BuildBot.Discord.Tests/TestEmbeds.cs | 11 +++++++++++ 5 files changed, 21 insertions(+), 24 deletions(-) create mode 100644 src/BuildBot.Discord.Tests/TestEmbeds.cs diff --git a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs index 6c184205..a27fdb7a 100644 --- a/src/BuildBot.Discord.Tests/DiscordSetupTests.cs +++ b/src/BuildBot.Discord.Tests/DiscordSetupTests.cs @@ -103,9 +103,7 @@ public async Task WebhookClient_RetriesTransientServerErrorsWithSamePayload() WebhookSendResult result = await client.SendAsync( webhookUrl: WebhookUrl, - payload: EmbedMapper.ToPayload( - new DiscordEmbed(Title: "Title", Url: null, Description: null, Colour: null, Fields: []) - ), + payload: EmbedMapper.ToPayload(TestEmbeds.WithTitle("Title")), cancellationToken: this.CancellationToken() ); diff --git a/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs b/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs index 2f1848b3..3a408475 100644 --- a/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs +++ b/src/BuildBot.Discord.Tests/Models/BotMessageTests.cs @@ -6,15 +6,10 @@ namespace BuildBot.Discord.Tests.Models; public sealed class BotMessageTests : TestBase { - private static DiscordEmbed CreateEmbed(string title) - { - return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); - } - [Fact] public void MessageProperty_ReturnsSuppliedEmbed() { - DiscordEmbed embed = CreateEmbed("Message"); + DiscordEmbed embed = TestEmbeds.WithTitle("Message"); BotMessage msg = new(embed); Assert.Same(expected: embed, actual: msg.Message); @@ -23,7 +18,7 @@ public void MessageProperty_ReturnsSuppliedEmbed() [Fact] public void TwoInstances_WithSameEmbed_AreEqual() { - DiscordEmbed embed = CreateEmbed("Message"); + DiscordEmbed embed = TestEmbeds.WithTitle("Message"); BotMessage msg1 = new(embed); BotMessage msg2 = new(embed); @@ -33,8 +28,8 @@ public void TwoInstances_WithSameEmbed_AreEqual() [Fact] public void TwoInstances_WithDifferentEmbeds_AreNotEqual() { - BotMessage msg1 = new(CreateEmbed("First Message")); - BotMessage msg2 = new(CreateEmbed("Second Message")); + BotMessage msg1 = new(TestEmbeds.WithTitle("First Message")); + BotMessage msg2 = new(TestEmbeds.WithTitle("Second Message")); Assert.NotEqual(expected: msg1, actual: msg2); } diff --git a/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs b/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs index 960f6c5d..b8ea612f 100644 --- a/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs +++ b/src/BuildBot.Discord.Tests/Publishers/DiscordBotMessageNotificationHandlerTests.cs @@ -22,9 +22,7 @@ public async Task Handle_LogsAndPublishesToChannel() DiscordBotMessageNotificationHandler handler = new(messageChannel: channel, logger: logger); - BotMessage notification = new( - new DiscordEmbed(Title: "Test Notification", Url: null, Description: null, Colour: null, Fields: []) - ); + BotMessage notification = new(TestEmbeds.WithTitle("Test Notification")); await handler.Handle(notification: notification, cancellationToken: this.CancellationToken()); diff --git a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs index 8d987561..fc83b81f 100644 --- a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs +++ b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs @@ -17,11 +17,6 @@ public sealed class BotServiceTests : TestBase { private static readonly TimeSpan ForwardingWait = TimeSpan.FromSeconds(3); - private static DiscordEmbed CreateEmbed(string title) - { - return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); - } - private static (BotService Service, IDiscordBot Bot, MessageChannel MessageChannel) CreateService() { IDiscordBot bot = GetSubstitute(); @@ -55,7 +50,7 @@ public async Task StopAsync_StopsForwardingMessages() await service.StopAsync(this.CancellationToken()); await messageChannel.PublishAsync( - message: new(CreateEmbed("Test")), + message: new(TestEmbeds.WithTitle("Test")), cancellationToken: this.CancellationToken() ); @@ -81,7 +76,7 @@ public async Task BotMessage_IsForwardedToBot_AfterDelay() using (service) { - DiscordEmbed embed = CreateEmbed("Test"); + DiscordEmbed embed = TestEmbeds.WithTitle("Test"); await messageChannel.PublishAsync(message: new(embed), cancellationToken: this.CancellationToken()); @@ -118,11 +113,11 @@ public async Task FailedSend_DoesNotStopLaterMessagesBeingSent() using (BotService service = new(bot: bot, botMessageChannel: messageChannel)) { await messageChannel.PublishAsync( - message: new(CreateEmbed("First")), + message: new(TestEmbeds.WithTitle("First")), cancellationToken: this.CancellationToken() ); await messageChannel.PublishAsync( - message: new(CreateEmbed("Second")), + message: new(TestEmbeds.WithTitle("Second")), cancellationToken: this.CancellationToken() ); diff --git a/src/BuildBot.Discord.Tests/TestEmbeds.cs b/src/BuildBot.Discord.Tests/TestEmbeds.cs new file mode 100644 index 00000000..069ea924 --- /dev/null +++ b/src/BuildBot.Discord.Tests/TestEmbeds.cs @@ -0,0 +1,11 @@ +using BuildBot.Discord.Models; + +namespace BuildBot.Discord.Tests; + +internal static class TestEmbeds +{ + public static DiscordEmbed WithTitle(string title) + { + return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); + } +} From 8523b808a71859eeded1510c6fad9a6d2552f43d Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:44:26 +0100 Subject: [PATCH 10/39] refactor(discord): collapse webhook retry loop into a single post/result path Refs #423 --- .../Services/DiscordWebhookClient.cs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/BuildBot.Discord/Services/DiscordWebhookClient.cs b/src/BuildBot.Discord/Services/DiscordWebhookClient.cs index 4854553c..33281b12 100644 --- a/src/BuildBot.Discord/Services/DiscordWebhookClient.cs +++ b/src/BuildBot.Discord/Services/DiscordWebhookClient.cs @@ -42,7 +42,9 @@ CancellationToken cancellationToken { HttpClient client = this._httpClientFactory.CreateClient(HttpClientName); - for (int attempt = 1; attempt < MAX_ATTEMPTS; ++attempt) + int attempt = 1; + + while (true) { using HttpResponseMessage response = await PostAsync( client: client, @@ -51,7 +53,7 @@ CancellationToken cancellationToken cancellationToken: cancellationToken ); - if (response.StatusCode != HttpStatusCode.TooManyRequests) + if (response.StatusCode != HttpStatusCode.TooManyRequests || attempt == MAX_ATTEMPTS) { return ToResult(response); } @@ -61,16 +63,9 @@ CancellationToken cancellationToken this._logger.LogRateLimited(attempt: attempt, delaySeconds: delay.TotalSeconds); await Task.Delay(delay: delay, timeProvider: this._timeProvider, cancellationToken: cancellationToken); - } - - using HttpResponseMessage finalResponse = await PostAsync( - client: client, - webhookUrl: webhookUrl, - payload: payload, - cancellationToken: cancellationToken - ); - return ToResult(finalResponse); + ++attempt; + } } public static TimeSpan CalculateRateLimitDelay(double? retryAfterSeconds, TimeSpan? retryAfterHeader) From ef39f7abafe8a0e314696bbcbda5ef204c3e64fe Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:50:34 +0100 Subject: [PATCH 11/39] fix(publish): pass Discord webhook URL instead of removed bot token settings Refs #423 --- src/publish | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/publish b/src/publish index e41e7368..fea1ca92 100755 --- a/src/publish +++ b/src/publish @@ -8,7 +8,7 @@ die() { PLATFORM=linux-x64 OUTFOLDER="$HOME/work/funfair/BuildBot/src/server-dist/$PLATFORM" -[ -z "$DISCORDTOKEN" ] && die "DISCORDTOKEN not set" +[ -z "$DISCORDWEBHOOKURL" ] && die "DISCORDWEBHOOKURL not set" echo "Clear $OUTFOLDER" \ && rm -fr "$HOME/work/funfair/BuildBot/src/server-dist" \ @@ -50,9 +50,6 @@ echo "Clear $OUTFOLDER" \ && sudo docker run \ --name buildbot \ -p 8080:8080 \ - -e "DISCORD__TOKEN=$DISCORDTOKEN" \ - -e "DISCORD__SERVER=FunFair Internal" \ - -e "DISCORD__CHANNEL=build" \ - -e "DISCORD__RELEASE_CHANNEL=release-notification" \ + -e "DISCORD__WEBHOOKURL=$DISCORDWEBHOOKURL" \ -it \ --rm "funfair/buildbot:latest" From 9f4f73a532b1c6c0458ac0a21385a9c4bd105b25 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 16:56:30 +0100 Subject: [PATCH 12/39] refactor(discord): use platform US spelling for colour identifiers Rename DiscordColour to DiscordColor, DiscordEmbed.Colour to Color and restore GithubStatusNotificationHandler.GetEmbedColor, per the cs-template Code Identifiers rule. Refs #423 --- ...rmationMessageReceivedNotificationHandlerTests.cs | 4 ++-- ...oudFormationMessageReceivedNotificationHandler.cs | 2 +- .../{DiscordColourTests.cs => DiscordColorTests.cs} | 6 +++--- .../Services/DiscordBotTests.cs | 2 +- .../Services/EmbedMapperTests.cs | 6 +++--- src/BuildBot.Discord.Tests/TestEmbeds.cs | 2 +- src/BuildBot.Discord/Models/DiscordColor.cs | 11 +++++++++++ src/BuildBot.Discord/Models/DiscordColour.cs | 11 ----------- src/BuildBot.Discord/Models/DiscordEmbed.cs | 4 ++-- src/BuildBot.Discord/Services/EmbedMapper.cs | 2 +- .../Publishers/GithubPushNotificationHandlerTests.cs | 2 +- .../GithubStatusNotificationHandlerTests.cs | 8 ++++---- .../Publishers/GithubPushNotificationHandler.cs | 2 +- .../Publishers/GithubStatusNotificationHandler.cs | 12 ++++++------ ...tchTowerPublishMessageNotificationHandlerTests.cs | 2 +- .../WatchTowerPublishMessageNotificationHandler.cs | 2 +- 16 files changed, 39 insertions(+), 39 deletions(-) rename src/BuildBot.Discord.Tests/Models/{DiscordColourTests.cs => DiscordColorTests.cs} (54%) create mode 100644 src/BuildBot.Discord/Models/DiscordColor.cs delete mode 100644 src/BuildBot.Discord/Models/DiscordColour.cs diff --git a/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs b/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs index 67c3b542..fa6d71c6 100644 --- a/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs +++ b/src/BuildBot.CloudFormation.Tests/Publishers/CloudFormationMessageReceivedNotificationHandlerTests.cs @@ -258,7 +258,7 @@ public async Task Handle_WithoutStackDetails_PublishesExpectedEmbed() actual: embed.Url ); Assert.Null(embed.Description); - Assert.Equal(expected: 0x2ECC71u, actual: embed.Colour?.Value); + Assert.Equal(expected: 0x2ECC71u, actual: embed.Color?.Value); Assert.Equal( expected: [ @@ -281,7 +281,7 @@ public async Task Handle_WithFailedDeploymentAndStackDetails_PublishesExpectedEm Assert.Equal(expected: "MyProject (2.0.0) failed to deploy", actual: embed.Title); Assert.Equal(expected: "A deployment", actual: embed.Description); - Assert.Equal(expected: 0xE74C3Cu, actual: embed.Colour?.Value); + Assert.Equal(expected: 0xE74C3Cu, actual: embed.Color?.Value); Assert.Equal( expected: [ diff --git a/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs b/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs index b0799e4b..ef5aa043 100644 --- a/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs +++ b/src/BuildBot.CloudFormation/Publishers/CloudFormationMessageReceivedNotificationHandler.cs @@ -79,7 +79,7 @@ private static DiscordEmbed BuildStatusMessage(in Deployment deployment, StackDe Title: BuildTitle(deployment: deployment, stackDetails: stackDetails), Url: BuildStackUrl(deployment), Description: stackDetails?.Description, - Colour: deployment.Success ? DiscordColour.Green : DiscordColour.Red, + Color: deployment.Success ? DiscordColor.Green : DiscordColor.Red, Fields: GetFields(deployment: deployment, stackDetails: stackDetails) ); } diff --git a/src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs b/src/BuildBot.Discord.Tests/Models/DiscordColorTests.cs similarity index 54% rename from src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs rename to src/BuildBot.Discord.Tests/Models/DiscordColorTests.cs index d518faea..84dc8f59 100644 --- a/src/BuildBot.Discord.Tests/Models/DiscordColourTests.cs +++ b/src/BuildBot.Discord.Tests/Models/DiscordColorTests.cs @@ -4,17 +4,17 @@ namespace BuildBot.Discord.Tests.Models; -public sealed class DiscordColourTests : TestBase +public sealed class DiscordColorTests : TestBase { [Fact] public void Green_MatchesDiscordGreen() { - Assert.Equal(expected: 0x2ECC71u, actual: DiscordColour.Green.Value); + Assert.Equal(expected: 0x2ECC71u, actual: DiscordColor.Green.Value); } [Fact] public void Red_MatchesDiscordRed() { - Assert.Equal(expected: 0xE74C3Cu, actual: DiscordColour.Red.Value); + Assert.Equal(expected: 0xE74C3Cu, actual: DiscordColor.Red.Value); } } diff --git a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs index 55c61553..7a93f428 100644 --- a/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordBotTests.cs @@ -27,7 +27,7 @@ public sealed class DiscordBotTests : TestBase Title: "Test Message", Url: new("https://example.com/build"), Description: "Description", - Colour: DiscordColour.Green, + Color: DiscordColor.Green, Fields: [new(Name: "Name", Value: "Value")] ); diff --git a/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs b/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs index 5f1edc98..d216d82a 100644 --- a/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs +++ b/src/BuildBot.Discord.Tests/Services/EmbedMapperTests.cs @@ -16,7 +16,7 @@ public void ToWebhookEmbed_MapsAllPropertiesAndAddsStandardAuthor() Title: "Title", Url: new("https://example.com/build"), Description: "Description", - Colour: DiscordColour.Red, + Color: DiscordColor.Red, Fields: [new(Name: "First", Value: "One"), new(Name: "Second", Value: "Two", Inline: true)] ); @@ -40,7 +40,7 @@ public void ToWebhookEmbed_MapsAllPropertiesAndAddsStandardAuthor() [Fact] public void ToWebhookEmbed_WithoutOptionalValues_LeavesThemNull() { - DiscordEmbed embed = new(Title: string.Empty, Url: null, Description: string.Empty, Colour: null, Fields: []); + DiscordEmbed embed = new(Title: string.Empty, Url: null, Description: string.Empty, Color: null, Fields: []); WebhookEmbed result = EmbedMapper.ToWebhookEmbed(embed); @@ -54,7 +54,7 @@ public void ToWebhookEmbed_WithoutOptionalValues_LeavesThemNull() [Fact] public void ToPayload_ContainsSingleMappedEmbed() { - DiscordEmbed embed = new(Title: "Title", Url: null, Description: null, Colour: DiscordColour.Green, Fields: []); + DiscordEmbed embed = new(Title: "Title", Url: null, Description: null, Color: DiscordColor.Green, Fields: []); WebhookPayload payload = EmbedMapper.ToPayload(embed); diff --git a/src/BuildBot.Discord.Tests/TestEmbeds.cs b/src/BuildBot.Discord.Tests/TestEmbeds.cs index 069ea924..3aa2bf34 100644 --- a/src/BuildBot.Discord.Tests/TestEmbeds.cs +++ b/src/BuildBot.Discord.Tests/TestEmbeds.cs @@ -6,6 +6,6 @@ internal static class TestEmbeds { public static DiscordEmbed WithTitle(string title) { - return new(Title: title, Url: null, Description: null, Colour: null, Fields: []); + return new(Title: title, Url: null, Description: null, Color: null, Fields: []); } } diff --git a/src/BuildBot.Discord/Models/DiscordColor.cs b/src/BuildBot.Discord/Models/DiscordColor.cs new file mode 100644 index 00000000..34f0d9e2 --- /dev/null +++ b/src/BuildBot.Discord/Models/DiscordColor.cs @@ -0,0 +1,11 @@ +using System.Diagnostics; + +namespace BuildBot.Discord.Models; + +[DebuggerDisplay("{Value}")] +public readonly record struct DiscordColor(uint Value) +{ + public static DiscordColor Green { get; } = new(0x2ECC71); + + public static DiscordColor Red { get; } = new(0xE74C3C); +} diff --git a/src/BuildBot.Discord/Models/DiscordColour.cs b/src/BuildBot.Discord/Models/DiscordColour.cs deleted file mode 100644 index ce44fba1..00000000 --- a/src/BuildBot.Discord/Models/DiscordColour.cs +++ /dev/null @@ -1,11 +0,0 @@ -using System.Diagnostics; - -namespace BuildBot.Discord.Models; - -[DebuggerDisplay("{Value}")] -public readonly record struct DiscordColour(uint Value) -{ - public static DiscordColour Green { get; } = new(0x2ECC71); - - public static DiscordColour Red { get; } = new(0xE74C3C); -} diff --git a/src/BuildBot.Discord/Models/DiscordEmbed.cs b/src/BuildBot.Discord/Models/DiscordEmbed.cs index 5ff7ab80..040ff227 100644 --- a/src/BuildBot.Discord/Models/DiscordEmbed.cs +++ b/src/BuildBot.Discord/Models/DiscordEmbed.cs @@ -4,11 +4,11 @@ namespace BuildBot.Discord.Models; -[DebuggerDisplay("{Title}: {Url} {Description} {Colour}")] +[DebuggerDisplay("{Title}: {Url} {Description} {Color}")] public sealed record DiscordEmbed( string Title, Uri? Url, string? Description, - DiscordColour? Colour, + DiscordColor? Color, IReadOnlyList Fields ); diff --git a/src/BuildBot.Discord/Services/EmbedMapper.cs b/src/BuildBot.Discord/Services/EmbedMapper.cs index c0df09ac..d4bdb2b1 100644 --- a/src/BuildBot.Discord/Services/EmbedMapper.cs +++ b/src/BuildBot.Discord/Services/EmbedMapper.cs @@ -24,7 +24,7 @@ public static WebhookEmbed ToWebhookEmbed(DiscordEmbed embed) Title: NullIfEmpty(embed.Title), Url: embed.Url, Description: NullIfEmpty(embed.Description), - Color: embed.Colour?.Value, + Color: embed.Color?.Value, Author: StandardAuthor, Fields: [.. embed.Fields.Select(ToWebhookEmbedField)] ); diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs index fec0eae4..866c3d5b 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubPushNotificationHandlerTests.cs @@ -201,7 +201,7 @@ public async Task PublishedEmbedHasExpectedContentAsync() Assert.Equal(expected: "testpusher pushed 2 commits to BuildBot (feature/my-feature)", actual: embed.Title); Assert.Equal(expected: new Uri("https://github.com/example/repo/compare/before...after"), actual: embed.Url); Assert.Null(embed.Description); - Assert.Null(embed.Colour); + Assert.Null(embed.Color); Assert.Equal( expected: [ diff --git a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs index 03207fba..a78e00f9 100644 --- a/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs +++ b/src/BuildBot.GitHub.Tests/Publishers/GithubStatusNotificationHandlerTests.cs @@ -120,7 +120,7 @@ public async Task NonPendingStateWithNoBranchesShouldPublishAsync(string state) await mediator.Received(1).Publish(Arg.Any(), Arg.Any()); } - public static TheoryData StateColours => + public static TheoryData StateColors => new() { { "success", 0x2ECC71 }, @@ -130,8 +130,8 @@ public async Task NonPendingStateWithNoBranchesShouldPublishAsync(string state) }; [Theory] - [MemberData(nameof(StateColours))] - public async Task PublishedEmbedHasExpectedContentAsync(string state, uint? expectedColour) + [MemberData(nameof(StateColors))] + public async Task PublishedEmbedHasExpectedContentAsync(string state, uint? expectedColor) { (IMediator mediator, GithubStatusNotificationHandler handler) = this.CreateHandler(); @@ -141,7 +141,7 @@ public async Task PublishedEmbedHasExpectedContentAsync(string state, uint? expe Assert.Equal(expected: "Build finished for ci/build from BuildBot (main)", actual: embed.Title); Assert.Equal(expected: new Uri("https://ci.example.com/builds/123"), actual: embed.Url); Assert.Equal(expected: "Built at abc123", actual: embed.Description); - Assert.Equal(expected: expectedColour, actual: embed.Colour?.Value); + Assert.Equal(expected: expectedColor, actual: embed.Color?.Value); Assert.Equal( expected: [ diff --git a/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs b/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs index 0ff29bb8..243574e1 100644 --- a/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs +++ b/src/BuildBot.GitHub/Publishers/GithubPushNotificationHandler.cs @@ -91,7 +91,7 @@ private static DiscordEmbed BuildPushEmbed(in Push message) Title: title, Url: EmbedUrl.FromString(message.CompareUrl), Description: null, - Colour: null, + Color: null, Fields: CreateCommitFields(message.Commits) ); } diff --git a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs index 2c31b72e..c63d6964 100644 --- a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs +++ b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs @@ -17,8 +17,8 @@ public sealed class GithubStatusNotificationHandler : INotificationHandler ColourMappings = [ - new(State: "success", Colour: DiscordColour.Green), - new(State: "failure", Colour: DiscordColour.Red), + new(State: "success", Colour: DiscordColor.Green), + new(State: "failure", Colour: DiscordColor.Red), ]; private readonly ILogger _logger; @@ -59,7 +59,7 @@ private static DiscordEmbed BuildStatusMessage(in Status message) Title: $"{message.Description} for {message.Context} from {message.Repository.Name} ({branchName})", Url: EmbedUrl.FromString(message.TargetUrl), Description: $"Built at {message.StatusCommit.Sha}", - Colour: GetEmbedColour(message), + Color: GetEmbedColor(message), Fields: GetFields(message) ); } @@ -79,16 +79,16 @@ private static DiscordEmbedField AddHeadCommitEmbed(in StatusCommit statusCommit return new(Name: "Head commit", Value: $"{statusCommit.Author.Login} - {statusCommit.Commit.Message}"); } - private static DiscordColour? GetEmbedColour(Status message) + private static DiscordColor? GetEmbedColor(Status message) { return ColourMappings .Where(mapping => mapping.IsMatch(message)) - .Select(static DiscordColour? (mapping) => mapping.Colour) + .Select(static DiscordColor? (mapping) => mapping.Colour) .FirstOrDefault(); } [DebuggerDisplay("{State} => {Colour}")] - private readonly record struct ColourMapping(string State, DiscordColour Colour) + private readonly record struct ColourMapping(string State, DiscordColor Colour) { public bool IsMatch(in Status message) { diff --git a/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs b/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs index bee0f447..9e7ff83f 100644 --- a/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs +++ b/src/BuildBot.Watchtower.Tests/Publishers/WatchTowerPublishMessageNotificationHandlerTests.cs @@ -48,7 +48,7 @@ await handler.Handle( Assert.Equal(expected: "Deployment", actual: embed.Title); Assert.Equal(expected: "Service deployed", actual: embed.Description); Assert.Null(embed.Url); - Assert.Null(embed.Colour); + Assert.Null(embed.Color); Assert.Empty(embed.Fields); } } diff --git a/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs b/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs index d8f5677f..acea8277 100644 --- a/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs +++ b/src/BuildBot.Watchtower/Publishers/WatchTowerPublishMessageNotificationHandler.cs @@ -26,7 +26,7 @@ private static DiscordEmbed BuildMessage(WatchTowerPublishMessage notification) Title: notification.Model.Title, Url: null, Description: notification.Model.Message, - Colour: null, + Color: null, Fields: [] ); } From 89787feaf924accca7dc9d7f387f046f727859a8 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 17:13:24 +0100 Subject: [PATCH 13/39] refactor(github): use platform US spelling for status colour mapping Pattern Sweep for 97c618a3 (UK-spelled code identifiers for the Discord color concept, against the language rule that identifiers follow platform convention). Renamed the pre-existing ColourMapping record struct, its Colour member and the ColourMappings field to ColorMapping / Color / ColorMappings. Searched the repo for Colour excluding .git/obj/bin/CHANGELOG.md; no Colour identifiers remain in src. Construct: UK-spelled Colour identifiers for the Discord color concept src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs: declares the ColourMapping record struct, its Colour member and the ColourMappings field. Refs #423. --- .../Publishers/GithubStatusNotificationHandler.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs index c63d6964..a3dd2f99 100644 --- a/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs +++ b/src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs @@ -15,10 +15,10 @@ namespace BuildBot.GitHub.Publishers; public sealed class GithubStatusNotificationHandler : INotificationHandler { - private static readonly IReadOnlyList ColourMappings = + private static readonly IReadOnlyList ColorMappings = [ - new(State: "success", Colour: DiscordColor.Green), - new(State: "failure", Colour: DiscordColor.Red), + new(State: "success", Color: DiscordColor.Green), + new(State: "failure", Color: DiscordColor.Red), ]; private readonly ILogger _logger; @@ -81,14 +81,14 @@ private static DiscordEmbedField AddHeadCommitEmbed(in StatusCommit statusCommit private static DiscordColor? GetEmbedColor(Status message) { - return ColourMappings + return ColorMappings .Where(mapping => mapping.IsMatch(message)) - .Select(static DiscordColor? (mapping) => mapping.Colour) + .Select(static DiscordColor? (mapping) => mapping.Color) .FirstOrDefault(); } - [DebuggerDisplay("{State} => {Colour}")] - private readonly record struct ColourMapping(string State, DiscordColor Colour) + [DebuggerDisplay("{State} => {Color}")] + private readonly record struct ColorMapping(string State, DiscordColor Color) { public bool IsMatch(in Status message) { From 6427b27c8d033888a7105c752ec29d7a11a92c16 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 17:35:49 +0100 Subject: [PATCH 14/39] refactor(discord): remove inter-message delay from BotService Owner request Q9 on PR #428. The Rx .Delay(InterMessageDelay) only shifted each message by 1s; it did not space messages. Concat still sends one at a time in order, and Discord rate limiting is handled by DiscordWebhookClient's 429 retry_after retry. BotServiceTests now wait on TaskCompletionSource signals bounded by WaitAsync timeouts instead of fixed 3s sleeps. Pattern Sweep: searched src for Rx .Delay, Task.Delay, Thread.Sleep, Observable.Timer/Interval, Throttle, Sample, SpinWait. Only remaining hit is DiscordWebhookClient's 429 back-off via the injected TimeProvider (different construct, required). Construct: Rx .Delay / fixed sleeps used as inter-message spacing Swept: none Refs #423. --- .../Services/BotServiceTests.cs | 61 ++++++++++++++----- src/BuildBot.Discord/Services/BotService.cs | 2 - 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs index fc83b81f..50080901 100644 --- a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs +++ b/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net; using System.Net.Http; using System.Reflection; @@ -15,7 +16,7 @@ namespace BuildBot.Discord.Tests.Services; public sealed class BotServiceTests : TestBase { - private static readonly TimeSpan ForwardingWait = TimeSpan.FromSeconds(3); + private static readonly TimeSpan CompletionWait = TimeSpan.FromSeconds(10); private static (BotService Service, IDiscordBot Bot, MessageChannel MessageChannel) CreateService() { @@ -47,14 +48,24 @@ public async Task StopAsync_StopsForwardingMessages() using (service) { - await service.StopAsync(this.CancellationToken()); - - await messageChannel.PublishAsync( - message: new(TestEmbeds.WithTitle("Test")), - cancellationToken: this.CancellationToken() + CancellationToken cancellationToken = this.CancellationToken(); + await service.StopAsync(cancellationToken); + + BotMessage message = new(TestEmbeds.WithTitle("Test")); + await messageChannel.PublishAsync(message: message, cancellationToken: cancellationToken); + + // The stopped service no longer reads the channel, so the message is still there for us to read. + await using IAsyncEnumerator reader = messageChannel + .ReadAllAsync(cancellationToken) + .GetAsyncEnumerator(cancellationToken); + Assert.True( + await reader + .MoveNextAsync() + .AsTask() + .WaitAsync(timeout: CompletionWait, cancellationToken: cancellationToken), + userMessage: "Message should remain unread in the channel" ); - - await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); + Assert.Same(expected: message, actual: reader.Current); await bot.DidNotReceive() .PublishAsync(embed: Arg.Any(), cancellationToken: Arg.Any()); @@ -70,9 +81,14 @@ public void Dispose_DisposesSubscriptions() } [Fact] - public async Task BotMessage_IsForwardedToBot_AfterDelay() + public async Task BotMessage_IsForwardedToBot() { (BotService service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); + TaskCompletionSource published = new(TaskCreationOptions.RunContinuationsAsynchronously); + bot.When(async x => + await x.PublishAsync(embed: Arg.Any(), cancellationToken: Arg.Any()) + ) + .Do(_ => published.TrySetResult()); using (service) { @@ -80,7 +96,7 @@ public async Task BotMessage_IsForwardedToBot_AfterDelay() await messageChannel.PublishAsync(message: new(embed), cancellationToken: this.CancellationToken()); - await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); + await published.Task.WaitAsync(timeout: CompletionWait, cancellationToken: this.CancellationToken()); await bot.Received(1).PublishAsync(embed: embed, cancellationToken: Arg.Any()); } @@ -91,6 +107,7 @@ public async Task FailedSend_DoesNotStopLaterMessagesBeingSent() { IDiscordWebhookClient client = GetSubstitute(); int calls = 0; + TaskCompletionSource secondSendAttempted = new(TaskCreationOptions.RunContinuationsAsynchronously); client .SendAsync( webhookUrl: Arg.Any(), @@ -98,10 +115,21 @@ public async Task FailedSend_DoesNotStopLaterMessagesBeingSent() cancellationToken: Arg.Any() ) .Returns(_ => - Interlocked.Increment(ref calls) == 1 - ? throw new HttpRequestException("connection failed") - : new WebhookSendResult(Success: true, StatusCode: HttpStatusCode.NoContent) - ); + { + int call = Interlocked.Increment(ref calls); + + if (call == 1) + { + throw new HttpRequestException("connection failed"); + } + + if (call == 2) + { + secondSendAttempted.TrySetResult(); + } + + return new WebhookSendResult(Success: true, StatusCode: HttpStatusCode.NoContent); + }); DiscordBot bot = new( client: client, @@ -121,7 +149,10 @@ await messageChannel.PublishAsync( cancellationToken: this.CancellationToken() ); - await Task.Delay(delay: ForwardingWait, cancellationToken: this.CancellationToken()); + await secondSendAttempted.Task.WaitAsync( + timeout: CompletionWait, + cancellationToken: this.CancellationToken() + ); await client .Received(2) diff --git a/src/BuildBot.Discord/Services/BotService.cs b/src/BuildBot.Discord/Services/BotService.cs index b2af4e75..f6c50ad4 100644 --- a/src/BuildBot.Discord/Services/BotService.cs +++ b/src/BuildBot.Discord/Services/BotService.cs @@ -10,7 +10,6 @@ namespace BuildBot.Discord.Services; public sealed class BotService : IHostedService, IDisposable { - private static readonly TimeSpan InterMessageDelay = TimeSpan.FromSeconds(1); private readonly IDiscordBot _bot; private readonly IMessageChannel _botMessageChannel; private readonly CancellationTokenSource _cancellationTokenSource = new(); @@ -22,7 +21,6 @@ public BotService(IDiscordBot bot, IMessageChannel botMessageChannel this._botMessageChannel.ReadAllAsync(this._cancellationTokenSource.Token) .ToObservable() - .Delay(InterMessageDelay) .Select(message => Observable.FromAsync(ct => this.PublishMessageAsync(message: message, cancellationToken: ct).AsTask()) ) From 54e50dcd26342359ffa7e4d530a7faf8c1ce8bce Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 17:41:22 +0100 Subject: [PATCH 15/39] refactor(discord): rename BotService to DiscordMessageSender Owner request Q10 on PR #428. With no bot connection left, the hosted service only drains the message queue and sends each message through the webhook client, so it is renamed to DiscordMessageSender (file, tests, DI registration, DI test name and a comment). DiscordBot, IDiscordBot and BotMessage are unchanged. Pattern Sweep: grep for BotService across the repo (excluding bin/obj/.git/CHANGELOG.md) finds no remaining references. Refs #423. --- .../DependencyInjectionTests.cs | 2 +- ...eTests.cs => DiscordMessageSenderTests.cs} | 22 +++++++++++-------- src/BuildBot.Discord/DiscordSetup.cs | 2 +- src/BuildBot.Discord/Services/DiscordBot.cs | 2 +- ...{BotService.cs => DiscordMessageSender.cs} | 4 ++-- 5 files changed, 18 insertions(+), 14 deletions(-) rename src/BuildBot.Discord.Tests/Services/{BotServiceTests.cs => DiscordMessageSenderTests.cs} (87%) rename src/BuildBot.Discord/Services/{BotService.cs => DiscordMessageSender.cs} (90%) diff --git a/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs b/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs index 1b78dfb5..0ebe8263 100644 --- a/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs +++ b/src/BuildBot.Discord.Tests/DependencyInjectionTests.cs @@ -58,7 +58,7 @@ public void BotMessageChannelMustBeRegistered() } [Fact] - public void BotServiceMustBeRegisteredAsIHostedService() + public void DiscordMessageSenderMustBeRegisteredAsIHostedService() { this.RequireService(); } diff --git a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs b/src/BuildBot.Discord.Tests/Services/DiscordMessageSenderTests.cs similarity index 87% rename from src/BuildBot.Discord.Tests/Services/BotServiceTests.cs rename to src/BuildBot.Discord.Tests/Services/DiscordMessageSenderTests.cs index 50080901..a0fc80c3 100644 --- a/src/BuildBot.Discord.Tests/Services/BotServiceTests.cs +++ b/src/BuildBot.Discord.Tests/Services/DiscordMessageSenderTests.cs @@ -14,16 +14,20 @@ namespace BuildBot.Discord.Tests.Services; -public sealed class BotServiceTests : TestBase +public sealed class DiscordMessageSenderTests : TestBase { private static readonly TimeSpan CompletionWait = TimeSpan.FromSeconds(10); - private static (BotService Service, IDiscordBot Bot, MessageChannel MessageChannel) CreateService() + private static ( + DiscordMessageSender Service, + IDiscordBot Bot, + MessageChannel MessageChannel + ) CreateService() { IDiscordBot bot = GetSubstitute(); MessageChannel messageChannel = new(); - BotService service = new(bot: bot, botMessageChannel: messageChannel); + DiscordMessageSender service = new(bot: bot, botMessageChannel: messageChannel); return (service, bot, messageChannel); } @@ -31,7 +35,7 @@ private static (BotService Service, IDiscordBot Bot, MessageChannel [Fact] public async Task StartAsync_Completes() { - (BotService service, IDiscordBot bot, _) = CreateService(); + (DiscordMessageSender service, IDiscordBot bot, _) = CreateService(); using (service) { @@ -44,7 +48,7 @@ public async Task StartAsync_Completes() [Fact] public async Task StopAsync_StopsForwardingMessages() { - (BotService service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); + (DiscordMessageSender service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); using (service) { @@ -75,7 +79,7 @@ await bot.DidNotReceive() [Fact] public void Dispose_DisposesSubscriptions() { - (BotService service, _, _) = CreateService(); + (DiscordMessageSender service, _, _) = CreateService(); service.Dispose(); } @@ -83,7 +87,7 @@ public void Dispose_DisposesSubscriptions() [Fact] public async Task BotMessage_IsForwardedToBot() { - (BotService service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); + (DiscordMessageSender service, IDiscordBot bot, MessageChannel messageChannel) = CreateService(); TaskCompletionSource published = new(TaskCreationOptions.RunContinuationsAsynchronously); bot.When(async x => await x.PublishAsync(embed: Arg.Any(), cancellationToken: Arg.Any()) @@ -138,7 +142,7 @@ public async Task FailedSend_DoesNotStopLaterMessagesBeingSent() ); MessageChannel messageChannel = new(); - using (BotService service = new(bot: bot, botMessageChannel: messageChannel)) + using (DiscordMessageSender service = new(bot: bot, botMessageChannel: messageChannel)) { await messageChannel.PublishAsync( message: new(TestEmbeds.WithTitle("First")), @@ -174,7 +178,7 @@ public static void Constructor_ThrowsArgumentNullException_WhenParameterIsNull( string expectedParamName ) { - ConstructorInfo? ctor = typeof(BotService).GetConstructor([ + ConstructorInfo? ctor = typeof(DiscordMessageSender).GetConstructor([ typeof(IDiscordBot), typeof(IMessageChannel), ]); diff --git a/src/BuildBot.Discord/DiscordSetup.cs b/src/BuildBot.Discord/DiscordSetup.cs index 3601a2c8..bbd8b33a 100644 --- a/src/BuildBot.Discord/DiscordSetup.cs +++ b/src/BuildBot.Discord/DiscordSetup.cs @@ -21,7 +21,7 @@ public static IServiceCollection AddDiscord(this IServiceCollection services, Di .AddSingleton() .AddSingleton(s => s.GetRequiredService()) .AddSingleton(s => s.GetRequiredService()) - .AddHostedService() + .AddHostedService() .AddSingleton(typeof(IMessageChannel<>), typeof(MessageChannel<>)) .AddDiscordWebhookHttpClient(); } diff --git a/src/BuildBot.Discord/Services/DiscordBot.cs b/src/BuildBot.Discord/Services/DiscordBot.cs index d47293e8..73be4ccb 100644 --- a/src/BuildBot.Discord/Services/DiscordBot.cs +++ b/src/BuildBot.Discord/Services/DiscordBot.cs @@ -40,7 +40,7 @@ public async ValueTask PublishAsync(DiscordEmbed embed, CancellationToken cancel return; } - // Nothing may escape: BotService sends from an Rx subscription which an exception would terminate. + // Nothing may escape: DiscordMessageSender sends from an Rx subscription which an exception would terminate. try { this._logger.LogSendingMessage(embed.Title); diff --git a/src/BuildBot.Discord/Services/BotService.cs b/src/BuildBot.Discord/Services/DiscordMessageSender.cs similarity index 90% rename from src/BuildBot.Discord/Services/BotService.cs rename to src/BuildBot.Discord/Services/DiscordMessageSender.cs index f6c50ad4..f6e07541 100644 --- a/src/BuildBot.Discord/Services/BotService.cs +++ b/src/BuildBot.Discord/Services/DiscordMessageSender.cs @@ -8,13 +8,13 @@ namespace BuildBot.Discord.Services; -public sealed class BotService : IHostedService, IDisposable +public sealed class DiscordMessageSender : IHostedService, IDisposable { private readonly IDiscordBot _bot; private readonly IMessageChannel _botMessageChannel; private readonly CancellationTokenSource _cancellationTokenSource = new(); - public BotService(IDiscordBot bot, IMessageChannel botMessageChannel) + public DiscordMessageSender(IDiscordBot bot, IMessageChannel botMessageChannel) { this._bot = bot ?? throw new ArgumentNullException(nameof(bot)); this._botMessageChannel = botMessageChannel ?? throw new ArgumentNullException(nameof(botMessageChannel)); From 26d831fba40845e4bfd04c0c6b363d8ae1833d19 Mon Sep 17 00:00:00 2001 From: Mark Ridgwell Date: Thu, 15 Oct 2026 17:55:19 +0100 Subject: [PATCH 16/39] docs(changelog): describe Discord webhook replacement for #423 Replaces the placeholder with Changed, Removed and Deployment Changes entries for the Discord.Net to webhook refactor. Refs #423. --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca86670d..4c4dd895 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,11 +87,13 @@ Please ADD ALL Changes to the UNRELEASED SECTION and not a specific release - CloudFormation template restructured into the shared format, with parameters only where the deploy workflow passes them and all other settings in a Settings mapping (#427) - ECS application logs in /ecs// are kept for 30 days instead of never expiring, using the same LogRetentionInDays setting as the API Gateway access logs (#421) - The ECS service is now named after the application, so it can be found by name in the DiscordBots cluster (#421) -- TBD - to be finalized after review +- Discord notifications are sent through a channel webhook (Discord:WebhookUrl / DISCORD__WEBHOOKURL) instead of a Discord.Net bot login (#423) +- Discord health status reflects whether the webhook URL is configured; startup fails if it is missing or not an absolute https URL (#423) ### Deprecated ### Removed - Removed NuGet audit suppressions for Scriban 6.2.0 vulnerabilities now that the transitive dependency has been dropped - TaskExecutionRole CloudFormation parameter (#419) +- Discord.Net dependency, release channel support and the Discord:Token, Discord:Server, Discord:Channel and Discord:ReleaseChannel settings (#423) ### Deployment Changes - ECS service deployment circuit breaker with rollback, so tasks that fail to start (e.g. a missing secret key) fail the deploy quickly (#419) - ECS tasks now read their secrets at start-up using the ecsTaskExecutionRole execution role, which must have secretsmanager:GetSecretValue on the BuildBot secret (#419) @@ -100,6 +102,7 @@ Please ADD ALL Changes to the UNRELEASED SECTION and not a specific release - This deploy replaces the ECS service, target group and listener rule (#421) - Payloads over 10 MB are rejected by API Gateway (#421) - The deploy user needs logs:PutRetentionPolicy on the /ecs/* log group, because the stack now sets a 30-day retention on it (#421) +- The BuildBot Secrets Manager secret must contain a DISCORD_WEBHOOK key (an https Discord webhook URL) before deploying; the DISCORD_TOKEN, DISCORD_SERVER, DISCORD_CHANNEL and DISCORD_RELEASE_CHANNEL keys are no longer used (#423)