Skip to content

test: strengthen SDK regression coverage - #283

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

marandaneto merged 2 commits into
mainfrom
test-audit

Conversation

@marandaneto

@marandaneto marandaneto commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

💡 Motivation and Context

Several tests could pass without proving the behavior in their descriptions. Examples include a remote-config expectation with no matcher, a fork test that ignored the child's exit status, and a log export test that did not prove the background worker restarted after a fork. The JSON compatibility cases were also missing from normal test discovery.

This PR strengthens payload, retry, timing, flag, MCP, and Rails assertions. It replaces several scheduling sleeps with explicit synchronization and adds cleanup for worker threads. JSON compatibility tests now run in separate processes. An optional OpenTelemetry bundle and CI jobs run the real export and fork tests, which are explicitly pending without those dependencies.

The production MCP lifecycle fix, its regression tests, and its changeset are now exclusively in #284. The shared client-cleanup hook that required that fix was also removed, so this PR remains independently testable. This PR contains no production SDK changes or changeset.

💚 How did you test it?

  • After splitting out the fix, the default suite reports 1,241 examples, zero failures, and two pending optional integrations.
  • The OpenTelemetry bundle passes all 1,241 examples.
  • Line coverage is 93.74% and branch coverage is 80.58% for the same 62 loaded production files. The coverage comment includes the baseline and measurement limits.
  • RuboCop, the public API snapshot check, and git diff --check pass after the split.
  • Earlier validation included randomized full-suite runs, both gem builds, and six controlled defects that passed the old tests and failed the repaired tests. These checks were run before extracting the lifecycle fix.
  • Docker compliance previously executed all 47 cases in each mode. Async passed 46 and sync passed 45. The failures match the existing documented GeoIP default and synchronous batching limitations.
  • Local RSpec validation used Ruby 4.0.7. Docker compliance used Ruby 3.3. The updated GitHub Actions matrix still needs to run.
  • Autoreview of commit 4f09eff against origin/main reported no actionable findings.

Generated coverage data, audit logs, and gem archives remain local and are not included in this PR.

📝 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 changeset is needed for these test and CI changes. The production fix's changeset is in #284.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Pi performed the audit using file and shell tools, read-only subagent reviews, and the autoreview helper. The work focused on stronger assertions and reliable execution rather than deleting tests or changing unrelated SDK behavior. The production fix discovered during the audit was moved to #284 for separate review and release.

Some broader coverage suggestions and ambiguous contracts remain follow-ups. This PR does not claim complete coverage of every SDK behavior. Human review is required.

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

marandaneto commented Sep 26, 2026 •

Copy link
Copy Markdown
Member Author

Test coverage before and after

Updated after moving the production MCP lifecycle fix, its regression tests, and its changeset to #284. Compared baseline 185060ab46517ae2e944d3f27b3c89630fc09a4f with this PR at 4f09eff, using the same native Ruby coverage runner and file filters on Ruby 4.0.7.

Measurement Before After Change
Default bundle line coverage 4,631 / 4,953 (93.50%) 4,643 / 4,953 (93.74%) +12 lines, +0.24 percentage points
Default bundle branch coverage 1,821 / 2,266 (80.36%) 1,826 / 2,266 (80.58%) +5 branches, +0.22 percentage points
OpenTelemetry bundle line coverage 4,631 / 4,953 (93.50%) 4,643 / 4,953 (93.74%) +12 lines, +0.24 percentage points
OpenTelemetry bundle branch coverage 1,822 / 2,266 (80.41%) 1,826 / 2,266 (80.58%) +4 branches, +0.18 percentage points

Test results

Bundle Before After
Default 1,219 passed 1,239 passed, 2 pending
OpenTelemetry 1,221 passed 1,241 passed

Both suites were rerun after the split and have no RSpec failures. The two optional integrations were silently omitted from the default baseline. They now appear as pending without their dependencies and pass with the OpenTelemetry bundle. The increase also includes three existing JSON compatibility cases that now run in separate processes during normal discovery.

Before the split, six controlled defects passed the old tests and failed the repaired tests: returning nil for remote config, sending a partial batch too early, failing a child-process flush, skipping middleware insertion, overwriting an application-owned session header, and not restarting the OpenTelemetry background worker. Those repaired assertions remain in this PR, but the mutation checks were not rerun after the split.

Measurement limits

These figures cover 62 loaded production files in the parent process under lib/, posthog-rails/lib/, and sdk_compliance_adapter/. The line and branch denominators are unchanged. They do not include dependency code or merge coverage from forked children and subprocesses.

Version files loaded before coverage starts, the Rails generator and template, and the Rails entry wrapper are outside this measurement. These are not whole-repository coverage percentages. The updated supported-Ruby CI matrix still needs to run.

@github-actions

github-actions Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

posthog-ruby-sync Compliance Report

Date: 2026-09-28T13:09:05.083836+00:00
Duration: 94077ms

⚠️ Some Tests Failed

45/47 tests passed, 2 failed


Capture Tests

⚠️ 29/30 tests passed, 1 failed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 10ms
Format Validation.Event Has Uuid ✅ 6ms
Format Validation.Event Has Lib Properties ✅ 7ms
Format Validation.Distinct Id Is String ✅ 9ms
Format Validation.Token Is Present ✅ 6ms
Format Validation.Custom Properties Preserved ✅ 6ms
Format Validation.Event Has Timestamp ✅ 7ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 6ms
Retry Behavior.Retries On 503 ✅ 5313ms
Retry Behavior.Does Not Retry On 400 ✅ 2011ms
Retry Behavior.Does Not Retry On 401 ✅ 2011ms
Retry Behavior.Respects Retry After Header ✅ 8018ms
Retry Behavior.Implements Backoff ✅ 15297ms
Retry Behavior.Retries On 500 ✅ 5118ms
Retry Behavior.Retries On 502 ✅ 5116ms
Retry Behavior.Retries On 504 ✅ 5145ms
Retry Behavior.Max Retries Respected ✅ 15514ms
Deduplication.Generates Unique Uuids ✅ 19ms
Deduplication.Preserves Uuid On Retry ✅ 5154ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10254ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5152ms
Deduplication.No Duplicate Events In Batch ✅ 18ms
Deduplication.Different Events Have Different Uuids ✅ 9ms
Compression.Sends Gzip When Enabled ✅ 7ms
Batch Format.Uses Proper Batch Structure ✅ 6ms
Batch Format.Flush With No Events Sends Nothing ✅ 4ms
Batch Format.Multiple Events Batched Together ❌ 22ms
Error Handling.Does Not Retry On 403 ✅ 2010ms
Error Handling.Does Not Retry On 413 ✅ 2011ms
Error Handling.Retries On 408 ✅ 5116ms

Failures

batch_format.multiple_events_batched_together

Expected 1 requests, got 5

Feature_Flags Tests

⚠️ 16/17 tests passed, 1 failed

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

Failures

request_payload.disable_geoip_omitted_defaults_to_false

Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['distinct_id', 'groups', 'person_properties', 'group_properties', 'flag_keys_to_evaluate', 'token']

@github-actions

github-actions Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

posthog-ruby-async Compliance Report

Date: 2026-09-28T13:09:19.145917+00:00
Duration: 98094ms

⚠️ Some Tests Failed

46/47 tests passed, 1 failed


Capture Tests

✅ 30/30 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields ✅ 108ms
Format Validation.Event Has Uuid ✅ 105ms
Format Validation.Event Has Lib Properties ✅ 107ms
Format Validation.Distinct Id Is String ✅ 106ms
Format Validation.Token Is Present ✅ 106ms
Format Validation.Custom Properties Preserved ✅ 106ms
Format Validation.Event Has Timestamp ✅ 106ms
Format Validation.Non Utc Event Timestamp Is Converted To Utc ✅ 6ms
Retry Behavior.Retries On 503 ✅ 5309ms
Retry Behavior.Does Not Retry On 400 ✅ 2109ms
Retry Behavior.Does Not Retry On 401 ✅ 2108ms
Retry Behavior.Respects Retry After Header ✅ 8013ms
Retry Behavior.Implements Backoff ✅ 15722ms
Retry Behavior.Retries On 500 ✅ 5211ms
Retry Behavior.Retries On 502 ✅ 5210ms
Retry Behavior.Retries On 504 ✅ 5211ms
Retry Behavior.Max Retries Respected ✅ 15521ms
Deduplication.Generates Unique Uuids ✅ 111ms
Deduplication.Preserves Uuid On Retry ✅ 5208ms
Deduplication.Preserves Uuid And Timestamp On Retry ✅ 10315ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry ✅ 5208ms
Deduplication.No Duplicate Events In Batch ✅ 109ms
Deduplication.Different Events Have Different Uuids ✅ 106ms
Compression.Sends Gzip When Enabled ✅ 105ms
Batch Format.Uses Proper Batch Structure ✅ 105ms
Batch Format.Flush With No Events Sends Nothing ✅ 3ms
Batch Format.Multiple Events Batched Together ✅ 109ms
Error Handling.Does Not Retry On 403 ✅ 2107ms
Error Handling.Does Not Retry On 413 ✅ 2107ms
Error Handling.Retries On 408 ✅ 5211ms

Feature_Flags Tests

⚠️ 16/17 tests passed, 1 failed

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

Failures

request_payload.disable_geoip_omitted_defaults_to_false

Field 'geoip_disable' not found in /flags request body at path 'geoip_disable'. Available keys: ['distinct_id', 'groups', 'person_properties', 'group_properties', 'flag_keys_to_evaluate', 'token']

@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Fixes client lifecycle tracking for subclasses and strengthens test coverage.

The PR appears safe to merge, though the instance-count guard should be corrected to preserve duplicate-client warnings.

Reviews (1) · Last reviewed commit: "test: strengthen SDK regression coverage..."

Comment thread lib/posthog/client.rb Outdated
@marandaneto
marandaneto marked this pull request as ready for review September 28, 2026 13:02
@marandaneto
marandaneto requested a review from a team as a code owner September 28, 2026 13:02
@marandaneto marandaneto changed the title test: strengthen SDK regression coverage and fix MCP client lifecycle test: strengthen SDK regression coverage Sep 28, 2026
@marandaneto
marandaneto merged commit 8fe7fc0 into main Sep 29, 2026
24 of 25 checks passed
@marandaneto
marandaneto deleted the test-audit branch September 29, 2026 06:10
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