Skip to content

[PM-34546] Add integration tests for Notifications POST /send endpoint - #8171

Merged
justindbaur merged 5 commits into
mainfrom
add-notifications-post-send-tests
Aug 10, 2026
Merged

[PM-34546] Add integration tests for Notifications POST /send endpoint#8171
justindbaur merged 5 commits into
mainfrom
add-notifications-post-send-tests

Conversation

@justindbaur

@justindbaur justindbaur commented Aug 7, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-34546

📔 Objective

Adds integration tests for the POST /send endpoint on the Notifications service.

The tests form a three-part contract:

  1. PushAsync_ProducesASupportedPayload — the real NotificationsApiPushEngine must produce a payload matching a format in SupportedPayloads, catching wire-format changes at CI time.
  2. PostSend_RoutesPayloadToCorrectHubGroup — every format in SupportedPayloads is accepted by the live /send endpoint and routed to the correct SignalR hub group.
  3. PushAsync_Notification_RoutesToClientTypeGroup — a PushAsync call with a specific ClientType in the notification payload routes end-to-end to the correct client-type-scoped SignalR group.

SupportedPayloads also serves as the rolling-upgrade compatibility list: old formats are kept for at least one release so that senders on the previous version still work after Notifications is deployed.

Adds PostSendEndpointTests and NotificationsApplicationFactory to
test/Notifications.Test. The tests form a three-part contract:

1. NotificationsApiPushEngine.PushAsync produces a payload that matches
   a format in SupportedPayloads, catching wire-format changes at CI time.
2. Every format in SupportedPayloads is accepted by the live /send
   endpoint and routed to the correct SignalR hub group.
3. PushAsync calls with a specific ClientType in the notification payload
   route to the correct client-type-scoped group.

SupportedPayloads also serves as the rolling-upgrade compatibility list:
old formats are kept for at least one release so that senders on the
previous version still work after Notifications is updated.
@justindbaur justindbaur changed the title Add integration tests for Notifications POST /send endpoint [PM-34546] test: add integration tests for Notifications POST /send endpoint Aug 7, 2026
@justindbaur justindbaur added the t:tech-debt Change Type - Tech debt label Aug 7, 2026
@justindbaur justindbaur changed the title [PM-34546] test: add integration tests for Notifications POST /send endpoint [PM-34546] Add integration tests for Notifications POST /send endpoint Aug 7, 2026
Comment thread test/Notifications.Test/NotificationsApplicationFactory.cs Dismissed
Comment thread test/Notifications.Test/NotificationsApplicationFactory.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Fixed
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.66%. Comparing base (1c0cf67) to head (d15fb49).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8171      +/-   ##
==========================================
+ Coverage   63.38%   63.66%   +0.28%     
==========================================
  Files        2337     2338       +1     
  Lines      101456   101524      +68     
  Branches     9167     9179      +12     
==========================================
+ Hits        64303    64637     +334     
+ Misses      34942    34672     -270     
- Partials     2211     2215       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justindbaur
justindbaur requested a review from a team August 10, 2026 16:47
@justindbaur
justindbaur marked this pull request as ready for review August 10, 2026 16:47
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Test-only change adding integration coverage for POST /send on the Notifications service, plus a NotificationsApplicationFactory that pairs an in-memory Notifications host with a real in-memory Identity server for token issuance. Both findings from the previous review round were addressed in d15fb49: RoutingCases is now the single source of truth with SupportedPayloads derived from it, and the captured payload is read from the actual request body via ReadAsStringAsync, so the recorded literals are now the real camelCase wire format JsonContent.Create emits. Routing expectations for all nine cases were re-traced against HubHelpers and match the production switch, including the client-type-scoped group behavior and the installation-before-user precedence for PushType.Notification.

Code Review Details

No findings at or above the reporting threshold.

Minor, not worth an inline comment: NotificationsApplicationFactory.CreateClient() remains unused — a test asserting POST /send rejects an unauthenticated request would put it to work and cover the Internal policy on this internal ingress endpoint. Also note the XML <summary> carrying the rolling-upgrade guidance now sits on the RoutingCase record declaration rather than the RoutingCases array a maintainer would edit.

Dependency Changes

Package Change Ecosystem
Microsoft.AspNetCore.Mvc.Testing New to this project (10.0.8), already used in IntegrationTestCommon and five other test projects at the same pinned version NuGet

Lock file (test/Notifications.Test/packages.lock.json) was regenerated alongside the manifest change.

Comment thread test/Notifications.Test/PostSendEndpointTests.cs Outdated
Comment thread test/Notifications.Test/PostSendEndpointTests.cs Outdated
- Capture actual wire bytes via ReadAsStringAsync instead of re-serializing
  with default JsonSerializerOptions (which produced PascalCase rather than
  the camelCase JsonContent.Create emits)
- Update SupportedPayloads literals to camelCase to match the real wire format
- Make RoutingCases the single source of truth; derive SupportedPayloads from
  it so the two arrays can never desynchronize
@withinfocus withinfocus added the ai-review Request a Claude code review label Aug 10, 2026

@withinfocus withinfocus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, but see the AI review comment for some suggestions that it didn't bubble up into dedicated suggestions.

@justindbaur
justindbaur merged commit ef8b7a9 into main Aug 10, 2026
60 checks passed
@justindbaur
justindbaur deleted the add-notifications-post-send-tests branch August 10, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants