test: GatewayExtractor and PaymentGatewayFactory unit tests (#52)#54
Merged
teesofttech merged 6 commits intoJun 12, 2026
Merged
Conversation
- Add PeachPayments = 15 to PaymentGatewayType enum - Add PeachPaymentsConfig (EntityId, AccessToken, IsSandbox) - Implement PeachPaymentsGateway: checkout initiation, payment verification, refund - Register gateway in IServiceCollectionExtensions and PaymentGatewayFactory - Add ZAR/KES/NGN/BWP routing to PeachPayments in PaymentService - Add PEACH_ prefix detection for gateway resolution
… helpers (#51) - Add MockHttpMessageHandler: queue-based fake HTTP handler with assertion helpers (AssertRequestCount, AssertLastRequestPath, AssertLastMethod). Supports multi-step gateways via sequential queuing. - Add PaymentRequestFactory: builds valid PaymentRequest and RefundRequest with sensible defaults; supports configure lambda. - Add GatewayConfigFactory: one builder per gateway, sets only the minimum keys required to pass constructor validation. - Add IntegrationTestBase: abstract base that checks env vars and exposes SkipIfMissingEnvVars() for clean CI output. - Add paybridge.runsettings: supports dotnet test --filter Category=Unit and --filter Category=Integration with code coverage config. - Add TestInfrastructureTests: 20 unit tests covering all helpers. Closes #51
- ci-tests.yml: runs on every push/PR to master and feature/* branches - Filters to Category=Unit only (no network, no secrets needed) - Publishes TRX results via dorny/test-reporter as PR comment - Uploads coverage.cobertura.xml artifact - integration-tests.yml: manual trigger (workflow_dispatch) + nightly cron - Runs Category=Integration tests with all 15 gateway env vars injected - Uses a protected 'integration' GitHub environment for secret scoping - fail-on-error=false so missing sandbox keys skip rather than fail
- Remove double-dash from XML comments in paybridge.runsettings (invalid XML: 'An XML comment cannot contain --') - Remove --settings flag from both workflows (runsettings only needed for local coverage; was causing the runner to reject it) - Rename workflow title to 'CI - Unit Tests' (dash safe in YAML) - Add fail-on-empty: false to dorny/test-reporter so it does not error when the TRX file is absent due to an upstream failure
GitHub denies test-reporter from creating check runs on PRs unless the workflow explicitly grants 'checks: write' and 'pull-requests: write'. Added permissions block to both ci-tests.yml and integration-tests.yml.
GatewayExtractorTests (15 tests): - DetectGatewayFromWebhook: Paystack (event key), Flutterwave (flw_ref), Stripe (ExpandoObject with stripe. prefix), Checkout (_links), Korapay (KR_ prefix), unknown payload, non-dictionary input, null input, unrecognised reference prefix, priority ordering - ExtractReferenceFromWebhook: Korapay (ExpandoObject dynamic), null for unknown payload, null for bad input type, fallback common property names (4 cases via Theory) PaymentGatewayFactoryTests (14 tests): - Constructor: null serviceProvider, null config, null logger - CreateGateways (empty EnabledGateways): single gateway returned, skips unregistered DI gateway, empty dict when nothing registered, multiple gateways when multiple registered - CreateGateways (explicit EnabledGateways): only enabled gateways returned, Automatic enum value ignored, no throw when constructor throws - GatewayType correctness: correct enum value, implements IPaymentGateway Note: Stripe and Korapay tests use ExpandoObject (not Dictionary) because the source implementation uses dynamic property access (data.type) which only works when the object satisfies both IDictionary<string,object> cast and dynamic member resolution — ExpandoObject satisfies both. Closes #52
📝 WalkthroughWalkthroughThis PR adds test infrastructure (mock handlers, factories, CI workflows), validates the infrastructure with unit tests, and implements a complete PeachPayments payment gateway with configuration, factory wiring, and service routing integration. ChangesTest Infrastructure & PeachPayments Gateway
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds new SDK unit-test coverage and supporting test/CI infrastructure, and also introduces a new production payment gateway (PeachPayments) with corresponding DI wiring and gateway-selection updates.
Changes:
- Added unit tests for
GatewayExtractorandPaymentGatewayFactory, plus shared test helpers/runsettings for consistent filtering and coverage. - Added GitHub Actions workflows for unit tests (PR/branch CI) and scheduled/manual integration tests.
- Added
PeachPaymentsgateway support across SDK runtime code (new gateway implementation, enum/config/DI registration, and selection/reference routing).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| PayBridge.SDK/Services/PaymentService.cs | Updates currency-to-gateway preference and adds PEACH_ reference detection. |
| PayBridge.SDK/Gateways/PeachPaymentsGateway.cs | New PeachPayments gateway implementation (create/verify/refund). |
| PayBridge.SDK/Factories/PaymentGatewayFactory.cs | Adds PeachPayments to factory initialization and DI resolution switch. |
| PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs | Registers PeachPaymentsGateway in DI (all + explicitly enabled). |
| PayBridge.SDK/Enums/PaymentGatewayType.cs | Adds PeachPayments enum member. |
| PayBridge.SDK/Dtos/PaymentGatewayConfig.cs | Adds PeachPaymentsConfig configuration section. |
| PayBridge.SDK.Test/Unit/TestInfrastructureTests.cs | Adds unit tests validating new test helper infrastructure. |
| PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs | Adds unit tests for factory behavior and selection. |
| PayBridge.SDK.Test/Unit/GatewayExtractorTests.cs | Adds unit tests for webhook gateway detection and reference extraction. |
| PayBridge.SDK.Test/PayBridge.SDK.Test.csproj | Adds/updates test project metadata and test dependencies. |
| PayBridge.SDK.Test/paybridge.runsettings | Adds runsettings for unit/integration filtering and coverage scoping. |
| PayBridge.SDK.Test/Helpers/PaymentRequestFactory.cs | Adds request/config factories to simplify tests. |
| PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs | Adds a queued-response HTTP handler for networkless tests. |
| PayBridge.SDK.Test/Helpers/IntegrationTestBase.cs | Adds base class to skip integration tests when env vars are missing. |
| .github/workflows/integration-tests.yml | Adds scheduled/manual integration test workflow with secrets-based env vars. |
| .github/workflows/ci-tests.yml | Adds PR/push unit test workflow including coverage artifact upload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
261
to
+265
| case "CDF": | ||
| case "XOF": | ||
| case "XAF": | ||
| case "MWK": | ||
| return ChooseAvailableGateway(PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); | ||
| return ChooseAvailableGateway(PaymentGatewayType.PeachPayments, PaymentGatewayType.PawaPay, PaymentGatewayType.DpoGroup, PaymentGatewayType.Flutterwave, PaymentGatewayType.Paystack); |
Comment on lines
+14
to
+20
| /// <summary> | ||
| /// Peach Payments gateway (South Africa, Kenya, Nigeria, Botswana). | ||
| /// Currencies: ZAR, KES, NGN, BWP, USD. | ||
| /// Auth: Bearer AccessToken + EntityId. | ||
| /// Docs: https://developer.peachpayments.com | ||
| /// </summary> | ||
| public class PeachPaymentsGateway : IPaymentGateway |
Comment on lines
+86
to
+93
| var json = JsonDocument.Parse(body).RootElement; | ||
| var resultCode = json.TryGetProperty("result", out var resultEl) && | ||
| resultEl.TryGetProperty("code", out var codeEl) | ||
| ? codeEl.GetString() : null; | ||
|
|
||
| var checkoutId = json.TryGetProperty("id", out var idEl) ? idEl.GetString() : null; | ||
| bool success = resultCode != null && resultCode.StartsWith("000"); | ||
|
|
Comment on lines
+103
to
+104
| Message = resultEl.TryGetProperty("description", out var descEl) | ||
| ? descEl.GetString() ?? "Unknown" : "Unknown", |
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
Comment on lines
+221
to
+222
| Message = resultEl.TryGetProperty("description", out var descEl) | ||
| ? descEl.GetString() ?? "Unknown" : "Unknown" |
Comment on lines
+40
to
+42
| var missing = requiredEnvVars | ||
| .Where(v => string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(v))) | ||
| .ToList(); |
| /// <summary> | ||
| /// Tests for the shared test infrastructure: MockHttpMessageHandler, | ||
| /// PaymentRequestFactory, GatewayConfigFactory, and IntegrationTestBase. | ||
| /// Closes #51. |
Comment on lines
+25
to
+29
| /// <summary> | ||
| /// Builds a real DI ServiceProvider with only the specified gateway registered. | ||
| /// Gateways that need HttpClient use a MockHttpMessageHandler so the | ||
| /// constructor succeeds without touching the network. | ||
| /// </summary> |
Comment on lines
+82
to
+85
| /// <summary> | ||
| /// Peach Payments gateway (South Africa, Kenya, Nigeria) | ||
| /// </summary> | ||
| PeachPayments = 15 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
.github/workflows/integration-tests.yml (2)
28-96: ⚖️ Poor tradeoffAddress security warnings flagged by static analysis.
Static analysis (zizmor) identified the same security concerns as
ci-tests.yml:
- Unpinned actions (lines 29, 32, 83, 90): Actions use tags instead of commit SHAs.
- Credential persistence (line 28-29): Checkout action doesn't set
persist-credentials: false.These warnings are identical to those in the unit test workflow. If you choose to address them, apply the same hardening approach consistently across both workflows.
For integration tests that handle secrets (lines 45-73), the credential-persistence concern is slightly more relevant, though the secrets are injected as environment variables rather than persisted Git credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-tests.yml around lines 28 - 96, Update the workflow to harden GitHub Actions: set persist-credentials: false on the checkout step (actions/checkout@v4) and replace floating tags for actions/setup-dotnet@v4, actions/upload-artifact@v4 and dorny/test-reporter@v1 with specific commit SHAs (pin to the latest vetted commit SHAs) to eliminate unpinned-action warnings; apply the same changes here as were made in ci-tests.yml so both unit and integration workflows are consistent.Source: Linters/SAST tools
74-80: 💤 Low valueConsider explicitly referencing the runsettings file.
Similar to the unit test workflow, integration tests don't reference
paybridge.runsettings. While integration tests may not need coverage collection, explicitly using--settingsensures consistent test execution configuration (e.g.,MaxCpuCount,ResultsDirectory).♻️ Proposed consistency improvement
run: | dotnet test PayBridge.SDK.Test/PayBridge.SDK.Test.csproj \ --configuration Release \ --no-build \ --filter "Category=Integration" \ + --settings PayBridge.SDK.Test/paybridge.runsettings \ --logger "trx;LogFileName=integration-test-results.trx" \ --results-directory ./TestResults🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-tests.yml around lines 74 - 80, Add explicit use of the runsettings file to the integration test command: when running dotnet test for PayBridge.SDK.Test (the existing dotnet test invocation), append --settings paybridge.runsettings to ensure consistent test runner configuration (e.g., MaxCpuCount, ResultsDirectory) across unit and integration workflows while keeping the existing filters and logger flags intact..github/workflows/ci-tests.yml (2)
38-46: ⚡ Quick winConsider explicitly referencing the runsettings file.
The test project includes
paybridge.runsettingsto configure coverage collection, but the workflow doesn't reference it via--settings. Additionally, the command-line--collect:"XPlat Code Coverage"may conflict with thedatacollector://Microsoft/CodeCoverage/2.0configured in runsettings.Consider adding the
--settingsflag to ensure consistent coverage configuration:- name: Run unit tests run: | dotnet test PayBridge.SDK.Test/PayBridge.SDK.Test.csproj \ --configuration Release \ --no-build \ --filter "Category=Unit" \ + --settings PayBridge.SDK.Test/paybridge.runsettings \ --logger "trx;LogFileName=unit-test-results.trx" \ --results-directory ./TestResults \ --collect:"XPlat Code Coverage"Alternatively, if relying on command-line coverage collection, update
paybridge.runsettingsto remove the conflictingDataCollectionRunSettingssection or configure it for XPlat Code Coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-tests.yml around lines 38 - 46, The workflow's dotnet test step ("Run unit tests") doesn't pass the repository's paybridge.runsettings, causing a potential conflict between the runsettings DataCollectionRunSettings (datacollector://Microsoft/CodeCoverage/2.0) and the command-line --collect:"XPlat Code Coverage"; update the dotnet test invocation to include --settings paybridge.runsettings to enforce the runsettings configuration, or alternatively remove/adjust the DataCollectionRunSettings section inside paybridge.runsettings and rely solely on --collect if you prefer command-line XPlat coverage; modify the command that runs in the "Run unit tests" step to use the chosen approach so coverage configuration is consistent.
24-70: ⚖️ Poor tradeoffAddress security warnings flagged by static analysis.
Static analysis (zizmor) identified several security concerns:
- Unpinned actions (lines 25, 28, 49, 56, 63): Actions reference tags (
@v4) instead of commit SHAs, which exposes the workflow to supply-chain attacks if action repositories are compromised.- Credential persistence (line 24-25):
actions/checkout@v4does not setpersist-credentials: false, potentially exposing credentials through artifacts.These are common security best practices for high-security projects. If your project's threat model requires strict supply-chain controls, consider:
🔒 Proposed security hardening
Pin actions to commit SHAs (example):
- name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 + with: + persist-credentials: falseApply similar pinning to all other actions (setup-dotnet, upload-artifact, test-reporter).
If using tag-based references is acceptable for your project's security posture, you may acknowledge and skip this suggestion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-tests.yml around lines 24 - 70, Update the workflow to address supply-chain and credential persistence issues: replace tag-based action references used in "uses: actions/checkout@v4", "uses: actions/setup-dotnet@v4", both "uses: actions/upload-artifact@v4" and "uses: dorny/test-reporter@v1" with pinned commit SHAs (commit@{sha}) to lock versions, and add persist-credentials: false under the checkout step (the block containing "uses: actions/checkout@v4") so credentials are not persisted to the workspace or artifacts; ensure the names (Checkout repository, Setup .NET 8, Upload test results, Upload coverage report, Publish test results (PR comment)) remain intact and update any documentation or inputs if you change the action refs.Source: Linters/SAST tools
PayBridge.SDK.Test/paybridge.runsettings (1)
1-38: ⚡ Quick winThe runsettings file is not explicitly referenced by CI workflows.
While this file is part of the test infrastructure per the PR objectives, neither
ci-tests.ymlnorintegration-tests.ymluses the--settingsflag to reference it. This means:
- The
MaxCpuCountandResultsDirectorysettings may rely on .NET's auto-discovery behavior (checking for.runsettingsin the test project directory).- The coverage configuration (lines 20-37) conflicts with the command-line
--collect:"XPlat Code Coverage"inci-tests.ymlline 46. The command-line collector typically takes precedence, so thedatacollector://Microsoft/CodeCoverage/2.0configuration here may be ignored.Recommendation: Decide on a consistent coverage strategy:
- Option 1 (command-line driven): Remove the
<DataCollectionRunSettings>section from this file and rely solely on--collect:"XPlat Code Coverage"in the workflow.- Option 2 (runsettings driven): Add
--settings PayBridge.SDK.Test/paybridge.runsettingsto both workflows and remove--collect:"XPlat Code Coverage"from the command line, configuring XPlat Code Coverage in runsettings instead.See the earlier comments on
ci-tests.ymlandintegration-tests.ymlfor proposed fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PayBridge.SDK.Test/paybridge.runsettings` around lines 1 - 38, The runsettings file contains a DataCollectionRunSettings/DataCollector configuration that conflicts with the CI workflows' command-line collector (--collect:"XPlat Code Coverage") and is not explicitly passed to the workflows; either remove the <DataCollectionRunSettings> section from paybridge.runsettings and keep the workflow --collect usage (Option 1), or update both CI workflows (ci-tests.yml and integration-tests.yml) to add the --settings PayBridge.SDK.Test/paybridge.runsettings flag and remove the command-line --collect option so the runsettings-driven collector is used (Option 2); locate the <DataCollectionRunSettings> / <DataCollector uri="datacollector://Microsoft/CodeCoverage/2.0"> elements in the XML to remove or keep, and edit the test invocation lines in the workflows that include --collect:"XPlat Code Coverage" to either delete that flag or add the --settings flag accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration-tests.yml:
- Around line 6-10: The workflow defines an unused input named "gateway"; either
remove that input block or wire it into the test invocation so tests are
filtered by gateway. To fix, update the test step that currently uses the fixed
filter "Category=Integration" (look for the dotnet test / test command in the
workflow) to include the gateway input (e.g., build a filter that combines
Category=Integration and Gateway=<${{ inputs.gateway }}>, or conditionally
append Gateway filter only when inputs.gateway != 'all'), or delete the entire
"gateway" inputs: section if you prefer Option 2.
In `@PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs`:
- Around line 102-108: BuildClient in MockHttpMessageHandler currently
constructs new HttpClient(this) which gives ownership of the handler to each
HttpClient and can lead to ObjectDisposedException when one client is disposed;
change the construction to use the HttpClient(HttpMessageHandler, bool) overload
with disposeHandler: false so the handler is not disposed by clients (or
alternatively update docs/summary of BuildClient to state it may only be called
once per handler instance); update references to BuildClient to reflect new
behavior if needed and ensure the handler is disposed explicitly by test
teardown if you choose disposeHandler: false.
In `@PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs`:
- Around line 25-41: The XML summary for BuildServiceProviderWith is inaccurate
about registering a MockHttpMessageHandler; it currently only calls
services.AddHttpClient(). Update the XML comment on BuildServiceProviderWith to
state that the method registers HttpClient/IHttpClientFactory via
services.AddHttpClient() and does not configure any MockHttpMessageHandler (or
alternatively note that individual tests must register mocks if a gateway
constructor performs HTTP), referencing BuildServiceProviderWith and the
services.AddHttpClient() call so readers know where to change behavior if they
need mocked handlers.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs`:
- Around line 142-143: The registration currently adds only the concrete
PeachPaymentsGateway; change both places (the branch around
services.AddScoped<PeachPaymentsGateway>() and the similar code at lines
194-196) to register it as the IPaymentGateway service so it is discoverable by
PaymentService when resolving IEnumerable<IPaymentGateway> — replace the
concrete-only registration with a scoped registration mapping IPaymentGateway to
PeachPaymentsGateway in IServiceCollectionExtensions.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs`:
- Around line 192-198: The hardcoded refund currency "ZAR" causes incorrect
refunds for non-ZAR payments; update the dictionary entry that currently sets
["currency"] = "ZAR" to use the actual transaction currency (e.g., use
request.Currency if available) and fall back to a sensible default such as
_config.PeachPayments.DefaultCurrency or _config.PeachPayments.Currency if
request.Currency is null/empty; keep existing formatting for amount
(request.Amount.ToString("F2", CultureInfo.InvariantCulture)) and retain other
keys (_config.PeachPayments.EntityId, ["amount"], ["paymentType"]) unchanged.
- Around line 202-207: The refund handler currently calls JsonDocument.Parse on
the response body unconditionally which can throw on non-2xx responses; update
the code around the call to _httpClient.PostAsync (the response variable) to
first check response.IsSuccessStatusCode and handle non-success responses by
logging the status code and body (via _logger.LogError/LogWarning) and returning
or throwing an appropriate error before attempting JsonDocument.Parse; only
parse the body and access properties like "result" (resultEl) when the response
is successful to avoid hiding upstream errors.
- Around line 143-145: The current parsing in PeachPaymentsGateway uses
json.ValueKind == JsonValueKind.Array ? json[0] : json which will throw when the
array is empty; update the logic to explicitly check array length (e.g.,
json.GetArrayLength() > 0) before indexing and handle the empty-array case as a
normal "not found" path (for example return null/NotFound result or set payment
to a safe default) instead of allowing an exception—modify the code around the
json and payment variables in PeachPaymentsGateway to perform this guard and
follow the gateway's existing not-found handling.
---
Nitpick comments:
In @.github/workflows/ci-tests.yml:
- Around line 38-46: The workflow's dotnet test step ("Run unit tests") doesn't
pass the repository's paybridge.runsettings, causing a potential conflict
between the runsettings DataCollectionRunSettings
(datacollector://Microsoft/CodeCoverage/2.0) and the command-line
--collect:"XPlat Code Coverage"; update the dotnet test invocation to include
--settings paybridge.runsettings to enforce the runsettings configuration, or
alternatively remove/adjust the DataCollectionRunSettings section inside
paybridge.runsettings and rely solely on --collect if you prefer command-line
XPlat coverage; modify the command that runs in the "Run unit tests" step to use
the chosen approach so coverage configuration is consistent.
- Around line 24-70: Update the workflow to address supply-chain and credential
persistence issues: replace tag-based action references used in "uses:
actions/checkout@v4", "uses: actions/setup-dotnet@v4", both "uses:
actions/upload-artifact@v4" and "uses: dorny/test-reporter@v1" with pinned
commit SHAs (commit@{sha}) to lock versions, and add persist-credentials: false
under the checkout step (the block containing "uses: actions/checkout@v4") so
credentials are not persisted to the workspace or artifacts; ensure the names
(Checkout repository, Setup .NET 8, Upload test results, Upload coverage report,
Publish test results (PR comment)) remain intact and update any documentation or
inputs if you change the action refs.
In @.github/workflows/integration-tests.yml:
- Around line 28-96: Update the workflow to harden GitHub Actions: set
persist-credentials: false on the checkout step (actions/checkout@v4) and
replace floating tags for actions/setup-dotnet@v4, actions/upload-artifact@v4
and dorny/test-reporter@v1 with specific commit SHAs (pin to the latest vetted
commit SHAs) to eliminate unpinned-action warnings; apply the same changes here
as were made in ci-tests.yml so both unit and integration workflows are
consistent.
- Around line 74-80: Add explicit use of the runsettings file to the integration
test command: when running dotnet test for PayBridge.SDK.Test (the existing
dotnet test invocation), append --settings paybridge.runsettings to ensure
consistent test runner configuration (e.g., MaxCpuCount, ResultsDirectory)
across unit and integration workflows while keeping the existing filters and
logger flags intact.
In `@PayBridge.SDK.Test/paybridge.runsettings`:
- Around line 1-38: The runsettings file contains a
DataCollectionRunSettings/DataCollector configuration that conflicts with the CI
workflows' command-line collector (--collect:"XPlat Code Coverage") and is not
explicitly passed to the workflows; either remove the
<DataCollectionRunSettings> section from paybridge.runsettings and keep the
workflow --collect usage (Option 1), or update both CI workflows (ci-tests.yml
and integration-tests.yml) to add the --settings
PayBridge.SDK.Test/paybridge.runsettings flag and remove the command-line
--collect option so the runsettings-driven collector is used (Option 2); locate
the <DataCollectionRunSettings> / <DataCollector
uri="datacollector://Microsoft/CodeCoverage/2.0"> elements in the XML to remove
or keep, and edit the test invocation lines in the workflows that include
--collect:"XPlat Code Coverage" to either delete that flag or add the --settings
flag accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02347e75-438c-4d65-a424-f13ffb9e5674
📒 Files selected for processing (16)
.github/workflows/ci-tests.yml.github/workflows/integration-tests.ymlPayBridge.SDK.Test/Helpers/IntegrationTestBase.csPayBridge.SDK.Test/Helpers/MockHttpMessageHandler.csPayBridge.SDK.Test/Helpers/PaymentRequestFactory.csPayBridge.SDK.Test/PayBridge.SDK.Test.csprojPayBridge.SDK.Test/Unit/GatewayExtractorTests.csPayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.csPayBridge.SDK.Test/Unit/TestInfrastructureTests.csPayBridge.SDK.Test/paybridge.runsettingsPayBridge.SDK/Dtos/PaymentGatewayConfig.csPayBridge.SDK/Enums/PaymentGatewayType.csPayBridge.SDK/Externsions/IServiceCollectionExtensions.csPayBridge.SDK/Factories/PaymentGatewayFactory.csPayBridge.SDK/Gateways/PeachPaymentsGateway.csPayBridge.SDK/Services/PaymentService.cs
Comment on lines
+6
to
+10
| inputs: | ||
| gateway: | ||
| description: 'Gateway to test (e.g. paystack, flutterwave, all)' | ||
| required: false | ||
| default: 'all' |
There was a problem hiding this comment.
The gateway input is defined but never used.
The workflow declares a gateway input suggesting users can filter tests by gateway (e.g., "paystack", "flutterwave", "all"), but the input is never referenced in the workflow. The test command (line 78) uses a fixed filter Category=Integration that runs all integration tests regardless of the input value.
Either remove the unused input or implement gateway-specific filtering:
✨ Option 1: Implement gateway filtering
- name: Run integration tests
env:
# ... (all env vars)
+ TEST_GATEWAY: ${{ github.event.inputs.gateway || 'all' }}
run: |
+ # Build filter based on gateway input
+ if [ "$TEST_GATEWAY" = "all" ]; then
+ FILTER="Category=Integration"
+ else
+ FILTER="Category=Integration&Gateway=$TEST_GATEWAY"
+ fi
+
dotnet test PayBridge.SDK.Test/PayBridge.SDK.Test.csproj \
--configuration Release \
--no-build \
- --filter "Category=Integration" \
+ --filter "$FILTER" \
--logger "trx;LogFileName=integration-test-results.trx" \
--results-directory ./TestResultsNote: This requires integration tests to be tagged with [Trait("Gateway", "paystack")] etc.
♻️ Option 2: Remove the unused input
on:
# Run manually from the Actions tab
workflow_dispatch:
- inputs:
- gateway:
- description: 'Gateway to test (e.g. paystack, flutterwave, all)'
- required: false
- default: 'all'
# Also run nightly on master so regressions are caught early
schedule:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/integration-tests.yml around lines 6 - 10, The workflow
defines an unused input named "gateway"; either remove that input block or wire
it into the test invocation so tests are filtered by gateway. To fix, update the
test step that currently uses the fixed filter "Category=Integration" (look for
the dotnet test / test command in the workflow) to include the gateway input
(e.g., build a filter that combines Category=Integration and Gateway=<${{
inputs.gateway }}>, or conditionally append Gateway filter only when
inputs.gateway != 'all'), or delete the entire "gateway" inputs: section if you
prefer Option 2.
Comment on lines
+102
to
+108
| public HttpClient BuildClient(string? baseAddress = null) | ||
| { | ||
| var client = new HttpClient(this); | ||
| if (baseAddress != null) | ||
| client.BaseAddress = new Uri(baseAddress); | ||
| return client; | ||
| } |
There was a problem hiding this comment.
Potential resource management hazard when reusing the handler.
BuildClient() creates an HttpClient that takes ownership of this handler. When that HttpClient is disposed, it will dispose the handler. If BuildClient() is called multiple times on the same MockHttpMessageHandler instance, the first HttpClient's disposal will dispose the shared handler, causing subsequent HttpClient instances to fail with ObjectDisposedException.
Consider either:
- Documenting that
BuildClient()should only be called once per handler instance, or - Passing
disposeHandler: falseto preventHttpClientfrom disposing the handler (requires using theHttpClient(HttpMessageHandler, bool)constructor overload).
🔒 Proposed fix to prevent handler disposal
public HttpClient BuildClient(string? baseAddress = null)
{
- var client = new HttpClient(this);
+ var client = new HttpClient(this, disposeHandler: false);
if (baseAddress != null)
client.BaseAddress = new Uri(baseAddress);
return client;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public HttpClient BuildClient(string? baseAddress = null) | |
| { | |
| var client = new HttpClient(this); | |
| if (baseAddress != null) | |
| client.BaseAddress = new Uri(baseAddress); | |
| return client; | |
| } | |
| public HttpClient BuildClient(string? baseAddress = null) | |
| { | |
| var client = new HttpClient(this, disposeHandler: false); | |
| if (baseAddress != null) | |
| client.BaseAddress = new Uri(baseAddress); | |
| return client; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK.Test/Helpers/MockHttpMessageHandler.cs` around lines 102 - 108,
BuildClient in MockHttpMessageHandler currently constructs new HttpClient(this)
which gives ownership of the handler to each HttpClient and can lead to
ObjectDisposedException when one client is disposed; change the construction to
use the HttpClient(HttpMessageHandler, bool) overload with disposeHandler: false
so the handler is not disposed by clients (or alternatively update docs/summary
of BuildClient to state it may only be called once per handler instance); update
references to BuildClient to reflect new behavior if needed and ensure the
handler is disposed explicitly by test teardown if you choose disposeHandler:
false.
Comment on lines
+25
to
+41
| /// <summary> | ||
| /// Builds a real DI ServiceProvider with only the specified gateway registered. | ||
| /// Gateways that need HttpClient use a MockHttpMessageHandler so the | ||
| /// constructor succeeds without touching the network. | ||
| /// </summary> | ||
| private static IServiceProvider BuildServiceProviderWith( | ||
| PaymentGatewayConfig config, | ||
| params PaymentGatewayType[] gatewayTypes) | ||
| { | ||
| var services = new ServiceCollection(); | ||
| services.AddLogging(); | ||
|
|
||
| // Register a named HttpClient for every gateway that uses IHttpClientFactory | ||
| services.AddHttpClient(); | ||
|
|
||
| services.AddSingleton(config); | ||
| services.AddSingleton(Microsoft.Extensions.Options.Options.Create(config)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if gateway constructors use HttpClient or IHttpClientFactory
# Search for gateway class constructors that accept HttpClient or IHttpClientFactory
rg -n -A 10 'class (Paystack|Flutterwave|Stripe|Monnify|Squad|Korapay|PeachPayments)Gateway' --glob '*.cs' | \
rg -C 5 'public.*Gateway\s*\('Repository: teesofttech/PayBridge
Length of output: 5113
Fix misleading XML comment about MockHttpMessageHandler in BuildServiceProviderWith
The XML comment claims gateways “use a MockHttpMessageHandler” to avoid network access during construction, but PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs (lines 25-41) only registers services.AddHttpClient() and doesn’t show any MockHttpMessageHandler/handler configuration. Since multiple gateways depend on HttpClient/IHttpClientFactory (e.g., PeachPaymentsGateway and gateways like KorapayGateway/MonnifyGateway/FlutterwaveGateway/StripeGateway), either wire up a mocked message handler for constructor-time HTTP (if needed) or update the comment to match the actual DI setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs` around lines 25 - 41,
The XML summary for BuildServiceProviderWith is inaccurate about registering a
MockHttpMessageHandler; it currently only calls services.AddHttpClient(). Update
the XML comment on BuildServiceProviderWith to state that the method registers
HttpClient/IHttpClientFactory via services.AddHttpClient() and does not
configure any MockHttpMessageHandler (or alternatively note that individual
tests must register mocks if a gateway constructor performs HTTP), referencing
BuildServiceProviderWith and the services.AddHttpClient() call so readers know
where to change behavior if they need mocked handlers.
Comment on lines
+142
to
143
| services.AddScoped<PeachPaymentsGateway>(); | ||
| return; |
There was a problem hiding this comment.
Register PeachPaymentsGateway as IPaymentGateway in both branches.
Current registration adds only the concrete type. PaymentService resolves gateways via IEnumerable<IPaymentGateway>, so PeachPayments won’t be discoverable for selection/routing.
Suggested fix
- services.AddScoped<PeachPaymentsGateway>();
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
...
case PaymentGatewayType.PeachPayments:
- services.AddScoped<PeachPaymentsGateway>();
+ AddGatewayRegistration<PeachPaymentsGateway>(services);
break;Also applies to: 194-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Externsions/IServiceCollectionExtensions.cs` around lines 142 -
143, The registration currently adds only the concrete PeachPaymentsGateway;
change both places (the branch around services.AddScoped<PeachPaymentsGateway>()
and the similar code at lines 194-196) to register it as the IPaymentGateway
service so it is discoverable by PaymentService when resolving
IEnumerable<IPaymentGateway> — replace the concrete-only registration with a
scoped registration mapping IPaymentGateway to PeachPaymentsGateway in
IServiceCollectionExtensions.
Comment on lines
+143
to
+145
| var json = JsonDocument.Parse(body).RootElement; | ||
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | ||
|
|
There was a problem hiding this comment.
Guard empty verify arrays before indexing.
When verification returns an empty array, direct json[0] access throws and escalates a normal “not found” case into an exception path.
Suggested fix
- var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json;
+ if (json.ValueKind == JsonValueKind.Array && json.GetArrayLength() == 0)
+ {
+ return new VerificationResponse
+ {
+ Success = false,
+ TransactionReference = transactionReference,
+ Status = PaymentStatus.Failed,
+ Message = "PeachPayments verification returned no matching transaction"
+ };
+ }
+ var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var json = JsonDocument.Parse(body).RootElement; | |
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; | |
| var json = JsonDocument.Parse(body).RootElement; | |
| if (json.ValueKind == JsonValueKind.Array && json.GetArrayLength() == 0) | |
| { | |
| return new VerificationResponse | |
| { | |
| Success = false, | |
| TransactionReference = transactionReference, | |
| Status = PaymentStatus.Failed, | |
| Message = "PeachPayments verification returned no matching transaction" | |
| }; | |
| } | |
| var payment = json.ValueKind == JsonValueKind.Array ? json[0] : json; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 143 - 145, The
current parsing in PeachPaymentsGateway uses json.ValueKind ==
JsonValueKind.Array ? json[0] : json which will throw when the array is empty;
update the logic to explicitly check array length (e.g., json.GetArrayLength() >
0) before indexing and handle the empty-array case as a normal "not found" path
(for example return null/NotFound result or set payment to a safe default)
instead of allowing an exception—modify the code around the json and payment
variables in PeachPaymentsGateway to perform this guard and follow the gateway's
existing not-found handling.
Comment on lines
+192
to
+198
| var formData = new Dictionary<string, string> | ||
| { | ||
| ["entityId"] = _config.PeachPayments.EntityId, | ||
| ["amount"] = request.Amount.ToString("F2", System.Globalization.CultureInfo.InvariantCulture), | ||
| ["currency"] = "ZAR", | ||
| ["paymentType"] = "RF", | ||
| }; |
There was a problem hiding this comment.
Refund currency is hardcoded to ZAR, which breaks non-ZAR Peach transactions.
This gateway is selected for additional currencies, but refund requests always send ZAR. That can cause rejected refunds or incorrect settlement behavior for non-ZAR payments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 192 - 198, The
hardcoded refund currency "ZAR" causes incorrect refunds for non-ZAR payments;
update the dictionary entry that currently sets ["currency"] = "ZAR" to use the
actual transaction currency (e.g., use request.Currency if available) and fall
back to a sensible default such as _config.PeachPayments.DefaultCurrency or
_config.PeachPayments.Currency if request.Currency is null/empty; keep existing
formatting for amount (request.Amount.ToString("F2",
CultureInfo.InvariantCulture)) and retain other keys
(_config.PeachPayments.EntityId, ["amount"], ["paymentType"]) unchanged.
Comment on lines
+202
to
+207
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | ||
| var body = await response.Content.ReadAsStringAsync(); | ||
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | ||
|
|
||
| var json = JsonDocument.Parse(body).RootElement; | ||
| var resultCode = json.TryGetProperty("result", out var resultEl) && |
There was a problem hiding this comment.
Handle non-success refund HTTP responses before JSON parsing.
Refund currently parses the body as JSON unconditionally. On non-2xx responses, this can throw and hide the real upstream error.
Suggested fix
var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content);
var body = await response.Content.ReadAsStringAsync();
_logger.LogDebug("PeachPayments refund response: {Body}", body);
+
+ if (!response.IsSuccessStatusCode)
+ {
+ return new RefundResponse
+ {
+ Success = false,
+ TransactionReference = request.TransactionReference,
+ Amount = request.Amount,
+ Status = PaymentStatus.Failed,
+ Message = $"PeachPayments refund failed: {response.StatusCode}"
+ };
+ }
var json = JsonDocument.Parse(body).RootElement;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | |
| var body = await response.Content.ReadAsStringAsync(); | |
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | |
| var json = JsonDocument.Parse(body).RootElement; | |
| var resultCode = json.TryGetProperty("result", out var resultEl) && | |
| var response = await _httpClient.PostAsync($"{BaseUrl}/payments/{request.TransactionReference}", content); | |
| var body = await response.Content.ReadAsStringAsync(); | |
| _logger.LogDebug("PeachPayments refund response: {Body}", body); | |
| if (!response.IsSuccessStatusCode) | |
| { | |
| return new RefundResponse | |
| { | |
| Success = false, | |
| TransactionReference = request.TransactionReference, | |
| Amount = request.Amount, | |
| Status = PaymentStatus.Failed, | |
| Message = $"PeachPayments refund failed: {response.StatusCode}" | |
| }; | |
| } | |
| var json = JsonDocument.Parse(body).RootElement; | |
| var resultCode = json.TryGetProperty("result", out var resultEl) && |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PayBridge.SDK/Gateways/PeachPaymentsGateway.cs` around lines 202 - 207, The
refund handler currently calls JsonDocument.Parse on the response body
unconditionally which can throw on non-2xx responses; update the code around the
call to _httpClient.PostAsync (the response variable) to first check
response.IsSuccessStatusCode and handle non-success responses by logging the
status code and body (via _logger.LogError/LogWarning) and returning or throwing
an appropriate error before attempting JsonDocument.Parse; only parse the body
and access properties like "result" (resultEl) when the response is successful
to avoid hiding upstream errors.
This was referenced Jul 17, 2026
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.
Summary
Closes #52
Files Added
PayBridge.SDK.Test/Unit/GatewayExtractorTests.cs— 15 testsPayBridge.SDK.Test/Unit/PaymentGatewayFactoryTests.cs— 14 testsGatewayExtractorTests (15 tests)
DetectGatewayFromWebhook
eventkeyflw_refkeytypestarting withstripe._linkskeyreferencestarts withKR_eventandtypekeysExtractReferenceFromWebhook
Key finding: Stripe and Korapay tests use
ExpandoObjectbecause the source does dynamic property access (data.type) — plainDictionary<string,object>satisfies theIDictionarycast but not dynamic member resolution.PaymentGatewayFactoryTests (14 tests)
Test Results
Summary by CodeRabbit
New Features
Tests