API contract tests#2255
Merged
Merged
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
niemyjski
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds contract-style test coverage around API surface metadata and behavior (OpenAPI + controller endpoint manifests), middleware edge cases, and legacy v1 compatibility paths, while enabling targeted WebSocket broker testing via internal access.
Changes:
- Added/expanded integration tests for OpenAPI output, controller endpoint manifests, and ProblemDetails validation response shape.
- Added middleware tests for throttling route exemptions and overage enforcement scenarios.
- Updated WebSocket broker + connection mapping behavior to support reliable logout/socket cleanup testing and safer concurrent access.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Exceptionless.Tests/Utility/Handlers/ThrottlingMiddlewareTests.cs | Adds unthrottled-route coverage for v1 project config and WebSocket push endpoints. |
| tests/Exceptionless.Tests/Utility/Handlers/OverageMiddlewareTests.cs | New integration tests covering overage middleware behaviors (suspension, size, missing length, usage limit, global disable). |
| tests/Exceptionless.Tests/Hubs/WebSocketLogoutTests.cs | New tests asserting auth-token revocation closes sockets and clears user-id connection mappings. |
| tests/Exceptionless.Tests/Hubs/WebSocketConnectionManagerTests.cs | Refactors tests to use shared TestWebSocket helper (removes nested class). |
| tests/Exceptionless.Tests/Hubs/TestWebSocket.cs | Introduces reusable test WebSocket implementation for hub/broker tests. |
| tests/Exceptionless.Tests/Controllers/ValidationSnapshotTests.cs | New snapshot-style tests asserting ProblemDetails JSON shape/casing across endpoints. |
| tests/Exceptionless.Tests/Controllers/OrganizationControllerTests.cs | Adds test documenting PATCH {} short-circuit behavior (no-op update). |
| tests/Exceptionless.Tests/Controllers/OpenApiControllerTests.cs | Adds structural assertions for selected OpenAPI paths/schemas/security schemes. |
| tests/Exceptionless.Tests/Controllers/EventControllerTests.cs | Adds legacy v1 PATCH casing-compatibility test for user description mapping. |
| tests/Exceptionless.Tests/Controllers/Data/controller-manifest.json | Adds regenerated controller endpoint manifest baseline snapshot. |
| tests/Exceptionless.Tests/Controllers/ControllerManifestTests.cs | Adds controller endpoint manifest generator + baseline comparison test. |
| src/Exceptionless.Web/Properties/AssemblyInfo.cs | Exposes web internals to Exceptionless.Tests for broker testing. |
| src/Exceptionless.Web/Hubs/MessageBusBroker.cs | Makes entity-change handler internal and adds auth-token removal logout cleanup (close sockets + clear mapping). |
| src/Exceptionless.Core/Utility/IConnectionMapping.cs | Improves thread-safety by locking around HashSet mutations and returning copies from reads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
niemyjski
force-pushed
the
niemyjski/api-contract-tests
branch
from
2d6d5e6 to
6853bee
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Exceptionless.Core/Utility/IConnectionMapping.cs:26
- AddAsync updates the per-key HashSet without synchronization (
hs.Add(connectionId)). Since other code now enumerates or checks Count on the same HashSet, HashSet must be protected by a shared lock for all reads/writes. Consider locking onhsinside the AddOrUpdate update delegate before callingAdd.
_connections.AddOrUpdate(key, [connectionId], (_, hs) =>
{
hs.Add(connectionId);
return hs;
});
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y RemoveAsync, blank lines Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…fields before save Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ithLoggingBase Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This update significantly enhances the `ConnectionMapping` utility by introducing a `ConnectionSet` wrapper. This wrapper provides per-key locking and a "detached" flag, which are crucial for: - Routinely removing keys from the `ConcurrentDictionary` when all associated connections are gone, preventing memory leaks and improving efficiency. - Robustly handling concurrent additions and removals, ensuring data integrity and preventing race conditions during map updates. Additional tests have been added to validate the new concurrent behaviors and key cleanup.
niemyjski
force-pushed
the
niemyjski/api-contract-tests
branch
from
dfc65fc to
33d68f2
Compare
Comment on lines
+80
to
+81
| var mapping = Assert.IsType<ConnectionMapping>(_connectionMapping); | ||
| Assert.Equal(1, mapping.TrackedKeyCount); |
Comment on lines
+217
to
+224
| private static DefaultHttpContext CreateEventPostContext(ClaimsPrincipal user, string organizationId, string projectId, long? contentLength = null) | ||
| { | ||
| var context = CreateContext(HttpMethods.Post, "/api/v2/events"); | ||
| context.User = user; | ||
| if (contentLength.HasValue) | ||
| context.Request.Headers.ContentLength = contentLength.Value; | ||
|
|
||
| return context; |
Previously, both missing (null) and explicit `Content-Length: 0` headers were treated as invalid, returning a `411 Length Required` status. This update modifies the `OverageMiddleware` to allow requests with a *missing* `Content-Length` header to pass through, differentiating them from requests explicitly specifying `Content-Length: 0`. Requests with an explicit `Content-Length: 0` will continue to be blocked with `411 Length Required`, as this still indicates a potentially malformed request where a body is expected. Additionally, the `PostsSize` diagnostic metric will now only record positive content lengths.
Simplifies `CreateEventPostContext` in `OverageMiddlewareTests` by removing redundant `organizationId` and `projectId` parameters, which are now derived from the `ClaimsPrincipal`. Also removes obsolete `ConnectionMapping` internal assertions from `WebSocketTests` that became irrelevant after recent `ConnectionMapping` refactorings.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Test coverage gaps. There were no tests for controller endpoint metadata, ProblemDetails response shape, middleware behavior, or the legacy v1 patch API. Existing tests also had architectural issues (wrong base classes, incorrect assertions, broken namespace filters).
Controller manifest
ControllerManifestTests: namespace filter was silently excludingSavedViewController,TokenController, andWebHookController(different namespace). Fixed to includeExceptionless.App.Controllers.*.controller-manifest.json: regenerated baseline now covers all 13 controllers.Overage middleware
OverageMiddlewareTests: changed base class toIntegrationTestsBase+AppWebHostFactory(requires Elasticsearch); removed staleEnsureSampleDataAsync()calls in favor of theResetDataAsyncoverride.Validation shape
ValidationSnapshotTests: 3 cross-controller tests verifying ProblemDetails JSON shape (camelCase keys, required RFC 7807 fields) for project create, org patch, and user-description endpoints. Auth-specific tests removed -- already covered inAuthControllerTests.Legacy v1 patch API
EventControllerTests: addedLegacyPatchAsync_WithPascalCaseLegacyFields_MapsToUserDescription. The v1 endpoint checksUnknownProperties["UserEmail"](PascalCase) -- snake-case keys silently produce no mapping. This test documents and guards the required key casing.OrganizationControllerTests: addedPatchAsync_EmptyJsonBody_ReturnsOriginalOrganizationUnchanged(empty delta short-circuits beforeSaveAsync).PatchCompatibilityTests.cs: deleted -- was a standalone file with wrong assumptions that tested behavior that doesn't exist or belongs in the respective controller test files.Notes
MessageBusBroker.OnEntityChangedAsyncwas madeinternal+InternalsVisibleTo("Exceptionless.Tests")added toExceptionless.Web. This is the cleanest way to test broker behavior without going through the message bus and without requiringEnableWebSockets: truein the test host.ValidationSnapshotTestsaccepts eitherapplication/jsonorapplication/problem+jsonas the error response content type. Controllers declare[Produces("application/json", "application/problem+json")]withapplication/jsonfirst, so content negotiation picks that type without an explicitAccept: application/problem+jsonheader from the test client. The JSON structure assertions (camelCase, RFC 7807 fields) are what matter here.