Test: SignalR hub and realtime integration tests (#706)#751
Conversation
AuthController constructor added IUserContext parameter in a prior PR but the unit tests were not updated. Pass a mock IUserContext to fix the compilation error.
Provides CreateBoardsHubConnection helper using WebApplicationFactory's in-memory test server, and an insertion-ordered EventCollector for deterministic event assertion without race conditions.
Covers presence lifecycle (join, leave, editing state, abrupt disconnect), authentication enforcement (unauthenticated, invalid token, no board access, join-before-edit guard), multi-user presence, same-user multi-connection aggregation, board switching, Guid.Empty sanitization, timestamp validity, and group isolation (no cross-board event leakage). Uses real SignalR pipeline via WebApplicationFactory test server. Closes #706
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Adversarial Self-ReviewPotential issues found
Assessment
RecommendationItems 2 (missing status assertions on grant access) and 1 (unused import) should be fixed. Items 4-5 (mutation/tool events) could be tracked as follow-up. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive integration tests for the BoardsHub SignalR hub, covering authentication, presence lifecycle, and authorization enforcement. It also updates AuthController tests to include a mocked IUserContext and provides a new SignalRTestHelper utility to facilitate hub testing. The review feedback focuses on improving test reliability and code quality by asserting specific exception types instead of the base Exception, utilizing 'await using' for guaranteed connection disposal, avoiding potentially flaky Task.Delay calls, and maintaining consistency with the project's preference for DateTimeOffset.
| await using var connection = SignalRTestHelper.CreateBoardsHubConnection(_factory, accessToken: null); | ||
|
|
||
| var act = () => connection.StartAsync(); | ||
| await act.Should().ThrowAsync<Exception>(); |
There was a problem hiding this comment.
Catching the base Exception type is too broad for an authentication test. It could mask other issues like configuration errors or null references. It is better to assert that a specific exception related to authentication failure is thrown (e.g., HttpRequestException or a specific SignalR exception wrapping the 401 response).
| await using var connection = SignalRTestHelper.CreateBoardsHubConnection(_factory, "not-a-valid-jwt"); | ||
|
|
||
| var act = () => connection.StartAsync(); | ||
| await act.Should().ThrowAsync<Exception>(); |
There was a problem hiding this comment.
Similar to the unauthenticated connection test, catching a generic Exception here is discouraged. Specifying the expected exception type ensures the test fails if the connection fails for an unexpected reason.
| await SignalRTestHelper.WaitForEventsAsync(events1, 1); | ||
|
|
||
| // User2 connects and joins | ||
| var conn2 = SignalRTestHelper.CreateBoardsHubConnection(_factory, user2.Token); |
There was a problem hiding this comment.
Using await using for the connection is safer. If an assertion fails before the manual DisposeAsync() call at line 237, the connection might not be disposed promptly. HubConnection.DisposeAsync() is idempotent, so calling it manually to simulate a disconnect is still valid even with await using.
await using var conn2 = SignalRTestHelper.CreateBoardsHubConnection(_factory, user2.Token);| await conn1.InvokeAsync("JoinBoard", board.Id); | ||
| await SignalRTestHelper.WaitForEventsAsync(events1, 1); | ||
|
|
||
| var conn2 = SignalRTestHelper.CreateBoardsHubConnection(_factory, user.Token); |
There was a problem hiding this comment.
Using await using for the connection is safer here as well, ensuring disposal if an assertion fails before the manual DisposeAsync() call at line 417.
await using var conn2 = SignalRTestHelper.CreateBoardsHubConnection(_factory, user.Token);| await conn2.InvokeAsync("JoinBoard", boardB.Id); | ||
|
|
||
| // Give events time to arrive (they shouldn't) | ||
| await Task.Delay(500); |
There was a problem hiding this comment.
Using a fixed Task.Delay to assert that an event is not received can lead to flaky tests, especially in CI environments where execution might be slower. While testing the absence of an event is challenging, consider if there's a more deterministic way to verify isolation, or at least use a named constant for the delay to clarify its purpose.
| using System.Collections.Concurrent; | ||
| using Microsoft.AspNetCore.SignalR.Client; |
There was a problem hiding this comment.
The System.Collections.Concurrent namespace is not used in this file. Removing unused directives keeps the code clean.
using Microsoft.AspNetCore.SignalR.Client;| var deadline = DateTime.UtcNow + (timeout ?? TimeSpan.FromSeconds(5)); | ||
| while (collector.Count < expectedCount && DateTime.UtcNow < deadline) |
There was a problem hiding this comment.
The project generally prefers DateTimeOffset.UtcNow for absolute timestamps. Using DateTime.UtcNow here is inconsistent with other parts of the codebase and can lead to issues when comparing mixed types.
var deadline = DateTimeOffset.UtcNow + (timeout ?? TimeSpan.FromSeconds(5));
while (collector.Count < expectedCount && DateTimeOffset.UtcNow < deadline)There was a problem hiding this comment.
Pull request overview
Adds backend integration coverage for the BoardsHub SignalR realtime presence flow using the in-memory WebApplicationFactory pipeline, and updates an existing auth controller edge-case test to match the current controller constructor.
Changes:
- Introduces
BoardsHubIntegrationTestsexercising hub authentication, presence lifecycle, authorization failures, and several edge cases. - Adds
SignalRTestHelper+EventCollector<T>to build in-memoryHubConnections and collect events for deterministic assertions. - Fixes
AuthControllerEdgeCaseTestscompilation by supplying the newly-requiredIUserContextconstructor dependency.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| backend/tests/Taskdeck.Api.Tests/Support/SignalRTestHelper.cs | New helper for creating hub connections against the test server and collecting hub events. |
| backend/tests/Taskdeck.Api.Tests/BoardsHubIntegrationTests.cs | New suite of integration tests covering BoardsHub presence/auth/authorization and edge cases. |
| backend/tests/Taskdeck.Api.Tests/AuthControllerEdgeCaseTests.cs | Updates controller construction to include IUserContext, fixing a pre-existing build break. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,83 @@ | |||
| using System.Collections.Concurrent; | |||
There was a problem hiding this comment.
Remove the unused using System.Collections.Concurrent; (it isn't referenced in this file), to avoid analyzer/warning noise in the test project.
| using System.Collections.Concurrent; |
| var deadline = DateTime.UtcNow + (timeout ?? TimeSpan.FromSeconds(5)); | ||
| while (collector.Count < expectedCount && DateTime.UtcNow < deadline) | ||
| { | ||
| await Task.Delay(50); | ||
| } | ||
|
|
||
| return collector.ToList(); |
There was a problem hiding this comment.
WaitForEventsAsync returns the collected events even when the timeout expires without reaching expectedCount, which makes timeouts harder to diagnose and can lead to unclear failures downstream. Consider throwing a TimeoutException (including expected vs actual counts) when the deadline is reached without enough events.
| var deadline = DateTime.UtcNow + (timeout ?? TimeSpan.FromSeconds(5)); | |
| while (collector.Count < expectedCount && DateTime.UtcNow < deadline) | |
| { | |
| await Task.Delay(50); | |
| } | |
| return collector.ToList(); | |
| var effectiveTimeout = timeout ?? TimeSpan.FromSeconds(5); | |
| var deadline = DateTime.UtcNow + effectiveTimeout; | |
| while (collector.Count < expectedCount && DateTime.UtcNow < deadline) | |
| { | |
| await Task.Delay(50); | |
| } | |
| var events = collector.ToList(); | |
| if (events.Count < expectedCount) | |
| { | |
| throw new System.TimeoutException( | |
| $"Timed out waiting for {expectedCount} event(s). Received {events.Count} event(s) within {effectiveTimeout}."); | |
| } | |
| return events; |
|
|
||
| /// <summary> | ||
| /// Integration tests for the BoardsHub SignalR hub covering presence lifecycle, | ||
| /// authentication enforcement, board mutation event delivery, and edge cases. |
There was a problem hiding this comment.
The XML summary says these tests cover "board mutation event delivery", but this test class only subscribes/asserts on boardPresence snapshots. Either add mutation-delivery tests (e.g., card/column events) or adjust the comment to match the actual coverage.
| /// authentication enforcement, board mutation event delivery, and edge cases. | |
| /// authentication enforcement, boardPresence snapshot delivery, and edge cases. |
| await using var connection = SignalRTestHelper.CreateBoardsHubConnection(_factory, accessToken: null); | ||
|
|
||
| var act = () => connection.StartAsync(); | ||
| await act.Should().ThrowAsync<Exception>(); |
There was a problem hiding this comment.
This assertion will pass for any exception type/cause (e.g., server error, routing misconfig), not specifically an authentication rejection. Tighten the assertion to validate the failure is due to auth (e.g., assert the negotiate/start exception indicates 401/Unauthorized).
| await act.Should().ThrowAsync<Exception>(); | |
| var exception = await act.Should().ThrowAsync<Exception>(); | |
| exception.Which.ToString().Should().MatchRegex("(?i)(401|Unauthorized)"); |
| public async Task InvalidToken_Connection_ShouldFailToStart() | ||
| { | ||
| await using var connection = SignalRTestHelper.CreateBoardsHubConnection(_factory, "not-a-valid-jwt"); | ||
|
|
||
| var act = () => connection.StartAsync(); | ||
| await act.Should().ThrowAsync<Exception>(); | ||
| } |
There was a problem hiding this comment.
Like the unauthenticated case, asserting a generic Exception here can mask failures unrelated to token validation. Consider asserting the error indicates an auth failure (401/Unauthorized) so the test doesn't pass on unrelated connection-start errors.
| await client1.PostAsJsonAsync( | ||
| $"/api/boards/{board.Id}/access", | ||
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); |
There was a problem hiding this comment.
The response from the access grant request isn't validated. Add a status-code assertion (or EnsureSuccessStatusCode) so authorization setup failures are caught at the right place.
| await client1.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/access", | |
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); | |
| var grantAccessResponse = await client1.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/access", | |
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); | |
| grantAccessResponse.EnsureSuccessStatusCode(); |
| await client1.PostAsJsonAsync( | ||
| $"/api/boards/{board.Id}/access", | ||
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); |
There was a problem hiding this comment.
The response from the access grant request isn't checked. Consider asserting success here to avoid confusing failures later when the second user tries to join the board.
| await client1.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/access", | |
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); | |
| using var grantAccessResponse = await client1.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/access", | |
| new GrantAccessDto(board.Id, user2.UserId, UserRole.Editor)); | |
| grantAccessResponse.EnsureSuccessStatusCode(); |
| new CreateColumnDto(board.Id, "Backlog", null, null)); | ||
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | ||
| var cardResponse = await client.PostAsJsonAsync( | ||
| $"/api/boards/{board.Id}/cards", | ||
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); |
There was a problem hiding this comment.
colResponse isn't validated before reading/deserializing the body. Add a status-code assertion (e.g., Created) or EnsureSuccessStatusCode() to make failures easier to diagnose.
| new CreateColumnDto(board.Id, "Backlog", null, null)); | |
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | |
| var cardResponse = await client.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/cards", | |
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); | |
| new CreateColumnDto(board.Id, "Backlog", null, null)); | |
| colResponse.StatusCode.Should().Be(HttpStatusCode.Created); | |
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | |
| var cardResponse = await client.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/cards", | |
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); | |
| cardResponse.StatusCode.Should().Be(HttpStatusCode.Created); |
| new CreateColumnDto(board.Id, "Backlog", null, null)); | ||
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | ||
| var cardResponse = await client.PostAsJsonAsync( | ||
| $"/api/boards/{board.Id}/cards", | ||
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); |
There was a problem hiding this comment.
cardResponse isn't validated before deserializing. As with other tests in this file, assert the expected status code (e.g., Created) before using the response content.
| new CreateColumnDto(board.Id, "Backlog", null, null)); | |
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | |
| var cardResponse = await client.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/cards", | |
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); | |
| new CreateColumnDto(board.Id, "Backlog", null, null)); | |
| colResponse.StatusCode.Should().Be(HttpStatusCode.Created); | |
| var column = await colResponse.Content.ReadFromJsonAsync<ColumnDto>(); | |
| var cardResponse = await client.PostAsJsonAsync( | |
| $"/api/boards/{board.Id}/cards", | |
| new CreateCardDto(board.Id, column!.Id, "Test Card", null, null, null)); | |
| cardResponse.StatusCode.Should().Be(HttpStatusCode.Created); |
| // Give events time to arrive (they shouldn't) | ||
| await Task.Delay(500); | ||
|
|
||
| // Only the initial join event from board A should be present | ||
| var collected = events.ToList(); | ||
| collected.Should().HaveCount(1); | ||
| collected.Single().BoardId.Should().Be(boardA.Id); | ||
| } |
There was a problem hiding this comment.
This test relies on a fixed Task.Delay(500) and then checks the current list, which can create false positives (an unexpected event could arrive just after the snapshot is taken). Prefer an assertion helper that actively fails if any additional event arrives within a defined window.
| // Give events time to arrive (they shouldn't) | |
| await Task.Delay(500); | |
| // Only the initial join event from board A should be present | |
| var collected = events.ToList(); | |
| collected.Should().HaveCount(1); | |
| collected.Single().BoardId.Should().Be(boardA.Id); | |
| } | |
| // Assert that no additional events arrive within the observation window. | |
| var collected = await AssertNoAdditionalEventsAsync(events, expectedCount: 1, TimeSpan.FromMilliseconds(500)); | |
| // Only the initial join event from board A should be present | |
| collected.Should().HaveCount(1); | |
| collected.Single().BoardId.Should().Be(boardA.Id); | |
| } | |
| private static async Task<IReadOnlyList<BoardPresenceSnapshot>> AssertNoAdditionalEventsAsync( | |
| EventCollector<BoardPresenceSnapshot> events, | |
| int expectedCount, | |
| TimeSpan observationWindow) | |
| { | |
| var deadline = DateTimeOffset.UtcNow + observationWindow; | |
| while (DateTimeOffset.UtcNow < deadline) | |
| { | |
| var snapshot = events.ToList(); | |
| snapshot.Should().HaveCount(expectedCount, "no additional boardPresence events should arrive during the observation window"); | |
| await Task.Delay(50); | |
| } | |
| var finalSnapshot = events.ToList(); | |
| finalSnapshot.Should().HaveCount(expectedCount, "no additional boardPresence events should arrive during the observation window"); | |
| return finalSnapshot; | |
| } |
… import - Add StatusCode assertions on all GrantAccessDto calls so test setup failures surface clearly instead of causing confusing Forbidden errors - Remove unused System.Collections.Concurrent import from SignalRTestHelper
Review fixes applied (304af98)Fixed items from adversarial review:
Remaining known gaps (not blocking, can be follow-up):
|
- Replace generic Exception assertions with HttpRequestException + 401 check in auth tests (prevents false positives from config errors) - WaitForEventsAsync now throws TimeoutException when expected events do not arrive, making timeout failures immediately diagnosable - Use DateTimeOffset.UtcNow instead of DateTime.UtcNow for consistency - Add missing status code assertions on column/card creation in ClearEditingCard test - Fix XML doc to not claim boardMutation coverage that does not exist - Use await using for conn2 in DisconnectOne test to prevent resource leak on assertion failure
Second-Pass Adversarial ReviewIssues found and fixed (474fefd)1. False-positive auth tests (HIGH) 2. Silent timeout in WaitForEventsAsync (HIGH) 3. DateTime.UtcNow instead of DateTimeOffset.UtcNow (LOW) 4. Missing precondition assertions in ClearEditingCard (MEDIUM) 5. Misleading XML doc comment (LOW) 6. Resource leak risk in DisconnectOne test (LOW) Items reviewed but not changed
|
Summary
EventCollector<T>helper provides insertion-ordered, thread-safe event collection to avoid race conditions in assertionsAuthControllerEdgeCaseTests(missingIUserContextparameter)Test coverage
Closes #706
Test plan
dotnet test --filter "FullyQualifiedName~BoardsHubIntegrationTests"InMemoryBoardPresenceTrackerTestsstill pass