Add lifecycle and concurrency tests for OptimizelyFeatureProvider#158
Merged
Conversation
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.
Hardens `OptimizelyFeatureProvider` against state-machine and concurrency edge cases, and ships a production fix the tests surfaced. WireMock-backed — runs on every PR, no Docker required.
Re-targets the same work that was previously merged to the `feat/optimizely-it` integration branch (#157) at `main` so the production fix and the on-every-PR tests land on the default branch independently of the docker-compose IT suite.
Production fix in `OptimizelyFeatureProvider.decide()`
The lifecycle spec exposed a real bug: calling `optimizely.isValid()` after `shutdown()` re-enters the polling HTTP client (closed by then), and Apache HttpClient throws an `IOException` that escapes `decide()` and surfaces to callers as a raw exception instead of a clean `PROVIDER_NOT_READY` error.
Fix: `decide()` now checks the provider's own `stateRef` first (no SDK call needed for NOT_READY / ERROR states) and wraps the defensive `isValid` probe in `Try` so any SDK-internal throw degrades to `PROVIDER_NOT_READY`:
```scala
if (stateRef.get() != ProviderState.READY) Left("PROVIDER_NOT_READY")
else if (transformed.userId.isEmpty) Left("TARGETING_KEY_MISSING")
else if (!Try(optimizely.isValid).getOrElse(false)) Left("PROVIDER_NOT_READY")
else ...
```
Check order also changed: provider state takes precedence over targeting-key validation, since a non-ready provider makes the caller's context irrelevant. Existing tests still pass — the targeting spec's provider is READY, so the new state check falls through to the targeting-key check unchanged.
`OptimizelyProviderLifecycleSpec` (8 tests)
`OptimizelyProviderConcurrencySpec` (4 tests)
Test fixture
`optimizely/src/test/resources/test-datafile-with-flag.json` — minimal Optimizely v4 datafile with one boolean flag rolled out 100%, used by both new specs (the existing `test-datafile-v1.json` is empty and can't exercise actual evaluation paths).
Verification