Skip to content

test: strengthen assertions and remove timing assumptions - #337

Merged
marandaneto merged 2 commits into
mainfrom
test-audit
Sep 29, 2026
Merged

marandaneto merged 2 commits into
mainfrom
test-audit

Conversation

@marandaneto

Copy link
Copy Markdown
Member

💡 Motivation and Context

Several tests passed without exercising the behavior in their names. The flag-event suppression test never evaluated a flag, the missing-evaluation-time test supplied a timestamp, and some serialization assertions checked the wrong property or only checked for a non-null result. Fixed sleeps also made concurrency and polling tests depend on scheduling.

This test-only change strengthens assertions across the core SDK, AI integration, and ASP.NET Core integration:

  • Check exact payloads, cache reuse, flag-event suppression, and context property precedence.
  • Use barriers for overlapping scopes and wait for published local flags instead of sleeping for a fixed duration.
  • Verify that disposal waits for an in-flight poll and that exception-capture fallback tests actually reach their intended failure.
  • Consume AI streams outside their original scope without buffering them first, then check privacy, identity, and response bytes.
  • Restrict intermediate fake-clock retry assertions to the target where the injected clock controls delays. Both targets retain final retry-count and response assertions.

No production code, public API, dependency, or coverage exclusions changed. No tests were deleted. The two existing skipped cancellation tests remain unchanged. Broader product concerns found during the audit are not included in this PR.

💚 How did you test it?

  • Ran locked restore, Release build, bin/fmt --check, and the full solution tests after syncing with main at b5440d7.
  • The final run passed 2,505 test executions with four existing skip executions across the two core targets.
  • Measured Coverlet line and branch coverage on the updated base and this branch with the same command. The comparison is posted in a PR comment, including the baseline retry-test failure and runtime limitations.
  • During the audit, seven temporary production mutations escaped the original focused assertions and failed the repaired tests. A controlled scheduling pause also reproduced the legacy retry-test race before its repair. All mutations were removed.
  • Autoreview against origin/main passed for cb2b4bebe366b140c72d31f72af04080053e2aa4 with no actionable findings.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran pnpm changeset to generate a changeset file

No release is needed for this test-only change.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Pi performed the audit using read-only delegate reviews, file and shell tools, the .NET test runner, Coverlet, and the isolated autoreview helper. The parent agent applied and validated all test changes. The work stayed in a dedicated worktree, and raw logs and audit reports were kept out of the PR. No shareable session link was generated.

The review checked the relevant SDK specifications for flag tracking, definition loading, callback ordering, and AI privacy. The changes preserve existing SDK behavior rather than addressing unrelated implementation differences. Human review is required.

@marandaneto marandaneto self-assigned this Sep 26, 2026
@marandaneto

Copy link
Copy Markdown
Member Author

Test coverage comparison

This comparison was rerun after syncing with the latest base. Before: b5440d78280409221d467d98ab52bfa75014ad69. After: cb2b4bebe366b140c72d31f72af04080053e2aa4.

SDK assembly / test target Lines before Lines after Branches before Branches after
PostHog / net8.0 84.67% 85.13% 77.67% 78.76%
PostHog / netcoreapp3.1 81.96% 82.40% 75.82% 76.94%
PostHog.AI / net8.0 81.01% 82.62% 71.91% 73.45%
PostHog.AspNetCore / net8.0 83.51% 83.51% 74.16% 74.16%

Each row uses the collector's package-level coverage from that assembly's own test project. TestLibrary is excluded from this table. Results are not summed across target frameworks, and no coverage filters or exclusions changed. Stronger assertions can improve regression protection without increasing coverage.

Test results and limitations

  • Before: 2,500 passed, one failed, four skipped. The failure was RetriesOnGatewayHttpStatusCodeThenSucceeds(GatewayTimeout) on the legacy target. The test expected the intermediate request count to remain one, but the real 1 ms retry had already completed. This PR fixes that timing assumption.
  • After: 2,505 passed, zero failed, four skipped. Two new date-format parameter rows add four executions across the two core targets. The four skips are the same two existing cancellation tests compiled for both targets.
  • Both measurements used macOS arm64, .NET SDK 9.0.121, and runtime 9.0.20. Repository RollForward=Major ran both target builds on .NET 9. These results do not establish native .NET 3.1 or .NET 8 runtime coverage.
  • The baseline includes the failed test described above. Its failure is disclosed rather than treating the baseline as a fully passing run.

Both runs used:

dotnet restore --locked-mode
dotnet test -c Release --no-restore --nologo \
  --collect:'XPlat Code Coverage' \
  --results-directory <before-or-after-directory> --logger trx

Release build and formatting checks also passed. Raw Cobertura and TRX reports are retained locally in the audit evidence directory.

@github-actions

github-actions Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

posthog-dotnet Compliance Report

Date: 2026-09-27T14:30:53.121589+00:00
Duration: 124920ms

✅ All Tests Passed!

47/47 tests passed


Capture Tests

✅ 30/30 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 250ms
Format Validation.Event Has Uuid ✅ 125ms
Format Validation.Event Has Lib Properties ✅ 109ms
Format Validation.Distinct Id Is String ✅ 112ms
Format Validation.Token Is Present ✅ 109ms
Format Validation.Custom Properties Preserved ✅ 115ms
Format Validation.Event Has Timestamp ✅ 109ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 117ms
Retry Behavior.Retries On 503 ✅ 8119ms
Retry Behavior.Does Not Retry On 400 ✅ 2117ms
Retry Behavior.Does Not Retry On 401 ✅ 2112ms
Retry Behavior.Respects Retry After Header ✅ 8119ms
Retry Behavior.Implements Backoff ✅ 22128ms
Retry Behavior.Retries On 500 ✅ 6118ms
Retry Behavior.Retries On 502 ✅ 6118ms
Retry Behavior.Retries On 504 ✅ 6114ms
Retry Behavior.Max Retries Respected ✅ 22133ms
Deduplication.Generates Unique Uuids ✅ 115ms
Deduplication.Preserves Uuid On Retry ✅ 6117ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 13129ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 6120ms
Deduplication.No Duplicate Events In Batch ✅ 116ms
Deduplication.Different Events Have Different Uuids ✅ 109ms
Compression.Sends Gzip When Enabled ✅ 109ms
Batch Format.Uses Proper Batch Structure ✅ 107ms
Batch Format.Flush With No Events Sends Nothing ✅ 106ms
Batch Format.Multiple Events Batched Together ✅ 114ms
Error Handling.Does Not Retry On 403 ✅ 2110ms
Error Handling.Does Not Retry On 413 ✅ 2111ms
Error Handling.Retries On 408 ✅ 6121ms

Feature_Flags Tests

✅ 17/17 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id ✅ 146ms
Request Payload.Flags Request Uses V2 Query Param ✅ 109ms
Request Payload.Flags Request Hits Flags Path Not Decide ✅ 107ms
Request Payload.Flags Request Omits Authorization Header ✅ 108ms
Request Payload.Token In Flags Body Matches Init ✅ 108ms
Request Payload.Groups Round Trip ✅ 112ms
Request Payload.Groups Default To Empty Object ✅ 110ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False ✅ 109ms
Request Payload.Disable Geoip Omitted Defaults To False ✅ 108ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key ✅ 109ms
Request Lifecycle.No Flags Request On Init Alone ✅ 3ms
Request Lifecycle.No Flags Request On Normal Capture ✅ 108ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests ✅ 212ms
Request Lifecycle.Mock Response Value Is Returned To Caller ✅ 111ms
Retry Behavior.Retries Flags On 502 ✅ 1114ms
Retry Behavior.Retries Flags On 504 ✅ 1110ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event ✅ 210ms

@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Low risk] Test suite improvements and timing fixes.

The PR appears safe to merge; no outstanding blocking finding or new actionable issue was identified.

Reviews (2) · Last reviewed commit: "test: wait for retry timer registration ..."

Comment thread tests/UnitTests/Library/HttpClientExtensionsTests.cs
@marandaneto
marandaneto marked this pull request as ready for review September 28, 2026 13:01
@marandaneto
marandaneto requested a review from a team as a code owner September 28, 2026 13:01
@marandaneto
marandaneto merged commit ae4d019 into main Sep 29, 2026
21 checks passed
@marandaneto
marandaneto deleted the test-audit branch September 29, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants