test: Cover FDv2 request shapes in instance-id and tags tests#340
Merged
Conversation
CommonInstanceIDTests and CommonTagsTests previously asserted the X-LaunchDarkly-Instance-Id and X-LaunchDarkly-Tags headers only on a single streaming synchronizer, a single polling synchronizer, and the events endpoint. FDv2 introduces three new request shapes that the suite did not exercise: polling Initializers that run before the synchronizer, Secondary Synchronizers reached after the Primary is permanently removed, and the FDv1 Fallback Synchronizer reached via the server-directed FDv1 Fallback Directive. Add subtests in both files that stand up each new request shape and assert the relevant header is present. The new subtests are server-side only, since the FDv2 features they exercise are server-side today; the FDv1 directive subtest is additionally gated on CapabilityFDv1Fallback so SDKs that have not yet implemented the directive can opt out. The tags variant uses one representative tags config for the new subtests rather than iterating over makeValidTagsTestParams -- endpoint coverage and tag-value variation are orthogonal properties.
Each subtest in CommonInstanceIDTests previously asserted only that the header was non-empty. Two endpoints in the same subtest could return different non-empty values and the test would still pass, which would mask a regression where the SDK assigns a new instance-id per request shape (or, for example, uses a separate HTTP transport for events vs data and forgets to plumb the shared instance-id through). Replace the per-call non-empty check with a per-subtest checker that latches the first observed value and asserts every subsequent request carries the same value. The checker is scoped to one SDK client lifetime: each subtest constructs its own checker, since each subtest creates its own client and so legitimately receives a different instance-id. Where the SDK naturally contacts more than one endpoint in a subtest, check both: - event posts: the SDK hits the data source during init and the events endpoint on flush. - polling initializer requests: initializer runs before the synchronizer. - secondary synchronizer requests: primary receives the 401 before the SDK falls through to the secondary. - FDv1 fallback directive requests: FDv2 stream returns the 403 + directive before the SDK transitions to the FDv1 endpoint. Tags tests already verify consistency implicitly by comparing every endpoint against the same static expectedHeaderValue, so no change there.
The earlier consistency check guarantees that within one SDK client all requests carry the same instance-id. The complementary property -- that two distinct clients in the same process produce different instance-ids -- was not covered. Without that check, an SDK that seeded the instance-id from process-level state and reused it across client instances would still pass every existing assertion, even though the resulting telemetry would be unable to distinguish the clients. Add a subtest that stands up two independent SDK clients back to back, captures the instance-id from each, and asserts the values differ. Gated on !CapabilitySingleton since the test creates a second client while the first still exists.
…subtests
Multi-agent review caught a PROVEN bug in the tags FDv1 fallback
subtest. The configurer list was
withTagsConfig(fdv2TagParams.tags),
WithConfig({StartWaitTimeMS: 5000}),
WithStreamingSynchronizer(...),
WithFDv1Fallback(...),
WithConfig performs *configOut = config, which overwrites every field
previously set -- including Tags. The SDK would have been started
without tags configured, would not have sent X-LaunchDarkly-Tags on
any request, and the assertion against the expected header value
would have failed every run.
Replace WithConfig with WithWaitToStart, which mutates only
StartWaitTimeMS and InitCanFail. The same substitution is applied to
the instance-id FDv1 fallback subtest; the bug was latent there
(WithConfig was the first option in the chain), but the pattern is
the same fragility waiting for a future option to be prepended.
While in the area, bring tags FDv2 subtests up to instance-id parity
by checking both endpoints exercised in each flow:
- polling initializer: initializer + synchronizer
- secondary synchronizer: primary + secondary
- FDv1 fallback: FDv2 stream + FDv1
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
tanderson-ld
approved these changes
Jun 1, 2026
| // disambiguate them. Stand up two independent clients back to back and | ||
| // assert their instance-ids differ. Gated on !CapabilitySingleton since | ||
| // the test requires creating a second client while the first still exists. | ||
| if !t.Capabilities().Has(servicedef.CapabilitySingleton) { |
Contributor
There was a problem hiding this comment.
Not to be that guy, but an SDK could have the ability to make a singleton and the ability to make instances. I supposed we don't do that nor do we expect to do that?
Member
Author
There was a problem hiding this comment.
In that case, the SDK wouldn't specify a capability of singleton like this. It's one of the earliest capabilities, and just designates SDKs that can ONLY create a single instance at a time, not that it has the ability to create a singleton. It should have been CapabilityMultiClient or something instead.
| } | ||
| fdv2TagParams.expectedHeaderValue = c.makeExpectedTagsHeader(fdv2TagParams.tags) | ||
|
|
||
| if !c.isClientSide { |
Contributor
There was a problem hiding this comment.
Is there a way to positively identify the category instead of negatively? Then if another type is added, this doesn't start running accidentally.
Address Cursor Bugbot feedback on the new FDv2 header-coverage subtests: the secondary-synchronizer and FDv1-fallback subtests verified headers on both endpoints but never asserted the abandoned source stops receiving connections. An SDK that falls through to the secondary/FDv1 path but keeps retrying the removed primary/FDv2 stream in the background would have passed. Add RequireNoMoreConnections on the abandoned endpoint at the end of all four subtests, matching the pattern in DirectiveOnStreamingErrorEngagesFDv1.
Replace the negative `!c.isClientSide` guard on the FDv2 request-shape subtests with the positive `c.sdkKind.IsServerSide()`. Functionally identical today (IsClientSide is defined as !IsServerSide), but states intent directly and means a future SDK category must opt into these server-side-only shapes explicitly rather than inheriting them by default. Addresses review feedback from tanderson-ld and Cursor Bugbot. There is no FDv2/data-system capability to gate on; FDv2 support is assumed for all server-side SDKs, matching how doServerSideFDv2StreamTests is gated.
…/fdv2-header-coverage
feat/fdv2 (#325) reworked the mock data model: ServerSDKDataBuilder no longer carries IntentCode/IntentReason, and the streaming/polling mock services now require FDv2-format data instead of auto-converting plain ServerSDKData. After merging the updated base, port the new subtests: - Build the no-op synchronizer via FDv2SDKDataFromServerSDKData(..., "none", "up-to-date", "initial") instead of the removed builder methods. - Serve the secondary streaming synchronizer a valid FDv2 "xfer-full" basis so the SDK can initialize from it; raw ServerSDKData now makes the mock streamer reject the request ("cannot handle non-fdv2 sdk data") and init times out. Verified against go-server-sdk (v7) and ruby-server-sdk (main) contract test services: all instance-id and tags subtests pass.
The existing FDv1 fallback tests in common_tests_stream_fdv2.go pair WithFDv1Fallback with WithServiceEndpoints so the top-level streaming and polling endpoints also point at the mocks. The new FDv1 fallback directive subtests omitted it, so an SDK that resolves the FDv1 polling URL from ServiceEndpoints.Polling (rather than DataSystem.FDv1Fallback.BaseURI) would contact the real LaunchDarkly endpoint and time out. Add WithServiceEndpoints to match the established convention. Addresses Cursor Bugbot feedback. Re-verified against go-server-sdk (v7) and ruby-server-sdk (main): all instance-id and tags subtests pass.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5731505. Configure here.
The FDv1 fallback directive subtests are gated on CapabilityFDv1Fallback but
not on server-side, yet they hardcoded the server-side FDv1 polling body
({"flags":..,"segments":..}). Client-side SDKs use a flat map of evaluations,
so a client-side SDK claiming the capability would receive an unparseable
payload and fail to initialize.
Client-side support for the FDv1 Fallback Directive is in progress, so rather
than gating these subtests to server-side only, add a commonTestsBase
emptyFDv1FallbackBody helper that returns an empty payload in the format the
SDK kind expects, and serve that. Keeps the capability-only gate valid for both
kinds, matching the format-aware fdv1FallbackBody helper in
common_tests_stream_fdv2.go.
Addresses Cursor Bugbot feedback. Re-verified against go-server-sdk (v7) and
ruby-server-sdk (main): all instance-id and tags subtests pass.
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
2 participants
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
CommonInstanceIDTestsandCommonTagsTestspreviously verified theX-LaunchDarkly-Instance-IdandX-LaunchDarkly-Tagsheaders only on a single streaming synchronizer, a single polling synchronizer, and the events endpoint. FDv2 introduced three new request shapes that the suite did not exercise:DataSystemOptionPollingInitializer).CommonStreamingTests.FDv2).A regression that dropped either header from any of these new request shapes would not have been caught by the existing tests.
This PR adds three subtests to each of the two files, standing up each request shape and asserting the relevant header is present:
polling initializer requests-- builds a data system with a polling initializer plus a no-op (xfer-none/up-to-date) synchronizer; asserts on the initializer endpoint.secondary synchronizer requests after permanent fallback-- primary returns 401 (non-recoverable, immediate permanent removal); secondary serves an empty FDv2 stream; asserts on the secondary endpoint.FDv1 fallback directive requests-- FDv2 streaming returns403 + X-LD-FD-Fallback: true; FDv1 endpoint serves an empty{"flags":{},"segments":{}}body so init completes along the fallback path; asserts on the FDv1 endpoint.The new subtests are server-side only -- the FDv2 features they exercise are server-side today, matching the gating used in
server_side_stream_all.go. The FDv1 directive subtest is additionally gated onCapabilityFDv1Fallbackso SDKs that have not yet implemented the directive can opt out.The tags variant uses one representative tags config for the new subtests rather than iterating over
makeValidTagsTestParams-- endpoint coverage and tag-value variation are orthogonal properties, and the existing subtests already cover the latter.Note
Low Risk
Test-only changes to the SDK test harness; no production SDK or service behavior is modified.
Overview
Expands the SDK test harness so
X-LaunchDarkly-Instance-IdandX-LaunchDarkly-Tagsare verified on FDv2-related request paths that were previously untested (streaming/poll/events only).Instance ID: Replaces one-off header checks with
newInstanceIDChecker, which requires a non-empty header and the same value across every endpoint touched by one client. Adds a subtest that distinct clients get distinct instance IDs (skipped whenCapabilitySingleton). Adds server-side subtests for polling initializer, secondary synchronizer after permanent 401 fallback, and FDv1 fallback directive (gated onCapabilityFDv1Fallback), including assertions that abandoned endpoints stop receiving traffic.Tags: Mirrors the same three FDv2 request-shape subtests using one representative tags config (endpoint coverage vs tag permutations).
Shared: Adds
emptyFDv1FallbackBodyoncommonTestsBaseso FDv1 fallback mocks return server- vs client-side-shaped empty JSON for init-only scenarios.Reviewed by Cursor Bugbot for commit 48e0fd9. Bugbot is set up for automated code reviews on this repo. Configure here.