feat: make DiscordChannelAdapter testable and add unit tests

[dnyw4l3n13]

Summary

Makes DiscordChannelAdapter testable by:

• Changing constructor parameter from the sealed SocketTextChannel to the ITextChannel interface
• Widening RestUserMessage return type to IUserMessage in SendMessageAsync
• Adding [assembly: InternalsVisibleTo("BuildBot.Discord.Tests")] so tests can instantiate the internal class
• Adding explicit Discord.Net package reference to the test project
• Adding unit tests for Name, EnterTypingState, and SendMessageAsync in DiscordChannelAdapter

Test plan

• Name_ReturnsChannelName — verifies Name delegates to ITextChannel.Name
• EnterTypingState_ReturnsTypingStateFromChannel — verifies EnterTypingState() returns the IDisposable from the channel
• SendMessageAsync_ReturnsChannelNameAndCleanContent — verifies the tuple mapping from IUserMessage

Closes #374

[dnyw4l3n13]

Implemented missing code changes in commit b44b951: changed DiscordChannelAdapter constructor from SocketTextChannel to ITextChannel, widened return type to IUserMessage, made class public (InternalsVisibleTo is prohibited by FFS0051), added explicit Discord.Net reference to test project, and added unit tests for Name, EnterTypingState, and SendMessageAsync. CI running; next cycle will continue.

[dnyw4l3n13]

The test sets up the mock with Arg.Any<>() for all parameters, so it doesn't verify that SendMessageAsync was called with the correct embed or text: string.Empty. A broken adapter that passed default(Embed) or "hello" as text would still make both assertions pass.

Add a Received() call to verify the channel was called with the expected arguments:

await channel
    .Received(1)
    .SendMessageAsync(
        text: string.Empty,
        tts: false,
        embed: embed,
        options: Arg.Any<RequestOptions>(),
        allowedMentions: Arg.Any<AllowedMentions>(),
        messageReference: Arg.Any<MessageReference>(),
        components: Arg.Any<MessageComponent>(),
        stickers: Arg.Any<ISticker[]>(),
        embeds: Arg.Any<Embed[]>(),
        flags: Arg.Any<MessageFlags>(),
        poll: Arg.Any<PollProperties>()
    );

[dnyw4l3n13]

GatewayIntents.MessageContent is absent from the intents registered in DiscordSocketClientAdapter (only Guilds | GuildMessageTyping | GuildMessages are set). Discord's gateway strips message content when this intent is not enabled, so msg.CleanContent always returns "" in production.

The test stubs message.CleanContent.Returns("clean content") and passes, but the actual production path silently returns an empty string. The test gives false confidence that CleanContent contains useful data.

Consider either:

1. Adding GatewayIntents.MessageContent to the registered intents in DiscordSocketClientAdapter, or
2. Adding a comment in the test noting that CleanContent is expected to be empty in production due to the missing intent.

[dnyw4l3n13]

Listing all 11 Arg.Any<>() matchers explicitly is brittle: if Discord.Net adds a parameter to SendMessageAsync in a future version, NSubstitute will silently stop matching the call (the setup won't fire, the method returns null, and the test throws NullReferenceException rather than a clear assertion failure).

ReturnsForAnyArgs is immune to parameter count changes:

channel.SendMessageAsync(default!, default, default, default, default, default, default, default!, default!, default, default)
    .ReturnsForAnyArgs(Task.FromResult(message));

[dnyw4l3n13]

ITextChannel extends IMessageChannel, so the channel substitute already satisfies the IMessageChannel type required by message.Channel. The extra messageChannel substitute and its Returns setup can be removed:

channel.Name.Returns("sent-channel");
message.Channel.Returns(channel);  // channel IS an IMessageChannel

This removes three lines and makes the data flow clearer: the channel the message is sent to is the same channel whose name appears in the result.

[dnyw4l3n13]

Code review round: fixed 4 findings — removed unnecessary IMessageChannel substitute, switched to ReturnsForAnyArgs for mock resilience, added Received(1) assertion to verify embed forwarding, and clarified CleanContent pass-through behaviour in test comment. Next cycle will continue with security review if CI is green.

[dnyw4l3n13]

msg.Channel.Name navigates through the returned IUserMessage to get the channel name, but the adapter already holds this._channel.Name directly. The indirection requires the test to stub message.Channel.Returns(channel) solely for this path — using this._channel.Name would be simpler and eliminate that extra stub.

[dnyw4l3n13]

Fixed in a37cac5 — switched to this._channel.Name directly and removed the now-unnecessary message.Channel.Returns(channel) stub from the test.

[dnyw4l3n13]

Discord.Net is already a transitive dependency of this test project via the ProjectReference to BuildBot.Discord.csproj. Adding an explicit PackageReference here creates a version-skew risk: if the production project bumps Discord.Net, the test project stays pinned to 3.20.1 and the two projects see different interface shapes at build time. No other test project in this repository re-declares transitive production dependencies — the test csproj should rely solely on the ProjectReference for Discord.Net types.

[dnyw4l3n13]

Fixed in 488d96c — removed the explicit Discord.Net PackageReference; the types are available transitively via the ProjectReference to BuildBot.Discord.csproj.

[dnyw4l3n13]

DiscordChannelAdapter was made public to enable direct instantiation in the test assembly, but the PR description itself mentions [assembly: InternalsVisibleTo("BuildBot.Discord.Tests")] as the intended approach — and that attribute was never added. The sibling DiscordSocketClientAdapter remains internal. Making this adapter public unnecessarily widens the library's API surface: external callers can now directly depend on the concrete type and its ITextChannel constructor parameter (a Discord.Net third-party interface), locking in an implementation detail that was previously encapsulated.

The correct fix is to revert to internal and add InternalsVisibleTo to the production project:

[assembly: InternalsVisibleTo("BuildBot.Discord.Tests")]

[dnyw4l3n13]

Reverted — FunFair.CodeAnalysis rule FFS0051 explicitly prohibits [assembly: InternalsVisibleTo] in this project, so making the class public is the correct approach given the project constraints. Finding withdrawn.

[dnyw4l3n13]

The constructor parameter channel has no null guard. Project convention in this service layer (e.g. BotService) guards all injected parameters with ?? throw new ArgumentNullException(nameof(...)). Since the class is now public, this gap is reachable from any caller, not just the internal factory that currently guards it before construction.

[dnyw4l3n13]

Fixed in e6bbfa6 — added null-guard to match project convention.

[dnyw4l3n13]

Line 48 ("This stub verifies the adapter correctly passes through whatever CleanContent the message returns.") describes what the test does rather than a non-obvious why, which the code-quality.instructions.md rule flags as an unacceptable comment. The non-obvious constraint that CleanContent is always "" in production (line 47) is the acceptable part, but line 48's description of the test's intent is inferable from the test name and structure and should be removed.

[dnyw4l3n13]

Fixed in ff19af1 — removed the second comment line.

[dnyw4l3n13]

Code review complete — 2 confirmed findings and 1 plausible finding posted as inline comments: (1) class should be internal + InternalsVisibleTo rather than public, (2) missing null-guard on constructor parameter, (3) test comment line 48 describes what the test does rather than a non-obvious why. Fixing now.

[dnyw4l3n13]

Code review round: fixed 2 findings — added null-guard on DiscordChannelAdapter constructor parameter (e6bbfa6), removed redundant test comment describing test intent (ff19af1). Note: the InternalsVisibleTo finding was withdrawn as FunFair.CodeAnalysis rule FFS0051 prohibits that pattern; making the class public is the correct approach for this project.

[dnyw4l3n13]

Code review clean — fixed 2 remaining inline findings (use this._channel.Name directly, remove redundant Discord.Net PackageReference) and advancing to security review.

[dnyw4l3n13]

Security review clean — no findings. Advancing to Human Review.

[dnyw4l3n13]

PR is ready for merge — marked ready for review, auto-merge enabled. Code review and security review both clean.

[credfeto]

Super-linter summary

Language	Validation result

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter