chore: adds DataSystem and FDv2 contract test service functionality#341
chore: adds DataSystem and FDv2 contract test service functionality#341tanderson-ld wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
"events/disabling" was skipped, but it is supported and has been for some time. I have updated it to not skip this.
| -s https://raw.githubusercontent.com/launchdarkly/sdk-test-harness/main/downloader/run.sh \ | ||
| | VERSION=v2 PARAMS="-url http://localhost:8001 -host 10.0.2.2 -skip-from testharness-suppressions.txt -debug $(TEST_HARNESS_PARAMS)" sh | ||
| -s https://raw.githubusercontent.com/launchdarkly/sdk-test-harness/v2/downloader/run.sh \ | ||
| | VERSION=v2 PARAMS="-url http://localhost:8001 -host 10.0.2.2 -debug -stop-service-at-end -skip-from $(SUPPRESSION_FILE) $(TEST_HARNESS_PARAMS_V2)" sh |
There was a problem hiding this comment.
For reviewers: The above block will be deleted and replaced with the below block after the below block is uncommented in a future PR.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 7b4efaf. Configure here.
| SdkConfigModeDefinition topLevel = new SdkConfigModeDefinition(); | ||
| topLevel.initializers = dataSystem.initializers; | ||
| topLevel.synchronizers = dataSystem.synchronizers; | ||
| dsBuilder.customizeConnectionMode(ConnectionMode.STREAMING, buildConnectionModeBuilder(topLevel)); |
There was a problem hiding this comment.
Top-level pipelines hardcoded to STREAMING ignoring foreground mode
Medium Severity
When connectionModeConfig.initialConnectionMode is set to a non-STREAMING value (e.g., "polling") but customConnectionModes is absent, the top-level initializers/synchronizers are hardcoded onto ConnectionMode.STREAMING via customizeConnectionMode. Meanwhile, foregroundConnectionMode was already changed to the requested mode. This means the custom pipelines are applied to an inactive mode while the actual foreground mode uses its default pipeline, silently ignoring the test harness's intended configuration.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7b4efaf. Configure here.
There was a problem hiding this comment.
I think this is right, looking at the code. Do you want to use connectionModeConfig.initialConnectionMode as the first parameter, but fall back to STREAMING if the initial connection mode is empty?
There was a problem hiding this comment.
And should this issue be triggered by a contract test that exercises the POLLING mode?
| case "background": return ConnectionMode.BACKGROUND; | ||
| default: throw new IllegalArgumentException("Unknown connection mode: " + name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this function something that could be put into the ConnectionMode class?
Each ConnectionMode has a defined name string. I see that ConnectionMode currently has toString(), maybe we could add fromString()?
|
|
||
| public static class SdkConfigDataInitializer { | ||
| SdkConfigPollParams polling; | ||
| } |
There was a problem hiding this comment.
For my current cache initializer work, I'll want to add a new SdkConfigCacheParams class and wire it up here?
aaron-zeisler
left a comment
aaron-zeisler
left a comment
There was a problem hiding this comment.
Overall it looks good to me. I have a couple curiosity questions, and I think the issue Bugbot pointed out is real.
2 participants
Requirements
I have added test coverage for new or changed functionality
N/A, this is the test coverage.
I have followed the repository's pull request submission guidelines
I have validated my changes against all supported platform versions
Related issues
SDK-1823
Describe the solution you've provided
Adds DataSystem control to the contract test service (this is what the SDK test harness connects to).
Note
Medium Risk
Touches client initialization/configuration paths used by contract tests and adjusts FDv2 changeset translation, so misconfiguration could alter how flags are fetched/updated during tests and FDv2 operation. Changes are scoped but affect core data-loading behavior.
Overview
Adds DataSystem controls to the Android contract-test service. The harness can now send a
dataSystemconfig that selects an initialConnectionModeand defines custom initializer/synchronizer pipelines (streaming/polling), with automatic mode switching disabled to keep tests deterministic.Updates FDv2 changeset translation.
FDv2ChangeSetTranslatornow forces theFlagkey to come from the FDv2 envelope (not the inner JSON), preventing missing/incorrect keys.Contract test harness runner adjustments. The
Makefilenow pins tosdk-test-harnessv2, standardizes params/suppression files, and introduces an FDv2 suppression list with a placeholder commentedv3runner.Reviewed by Cursor Bugbot for commit 7b4efaf. Bugbot is set up for automated code reviews on this repo. Configure here.