feat: Impression events #25
Conversation
| assertThat(config.proxyUrl).isEmpty() | ||
| assertThat(config.clientKey).isEmpty() |
There was a problem hiding this comment.
Proxy url and clientKey are required but we don't need them for testing (when both polling and metrics are disabled), therefore I think it's easier to keep them as empty as in production they should have a value
| while (!ready) { | ||
| println("Waiting for unleash to be ready") | ||
| waits ++ | ||
| Thread.sleep(50) | ||
| if (waits > 100) { | ||
| throw IllegalStateException("Unleash did not become ready") | ||
| } |
There was a problem hiding this comment.
https://medium.com/androiddevelopers/alternatives-to-idling-resources-in-compose-tests-8ae71f9fc473
Seems to only relate to compose though. But feels waitUntil(ready, Duration.ofSeconds(1)) would make for a much more readable test.
We could add https://github.com/awaitility/awaitility as a testDependency, that would give you an await api for anything async.
gastonfournier
force-pushed
the
delayed-init
branch
2 times, most recently
from
3e2bddb to
abe670c
Compare
… therefore I think it's easier to keep them as empty as in production they should have a value
gastonfournier
force-pushed
the
impression-events
branch
from
c825b99 to
b13c465
Compare
| val enabled = toggle?.enabled ?: defaultValue | ||
| val impressionData = toggle?.impressionData ?: unleashConfig.forceImpressionData | ||
| if (impressionData) { | ||
| emit(ImpressionEvent(toggleName, enabled, unleashContextState.value)) |
There was a problem hiding this comment.
@chriswk @sighphyre there's a chance that context might change from checking isEnabled to obtaining the context in this line. Is this something we should be worried about? Maybe the js proxy SDK won't suffer from this issue because there's a single thread and I'm not sure this method can be interrupted by other thread setting the context:
https://github.com/Unleash/unleash-proxy-client-js/blob/d13ad709213cad7bedd92c8a4c1bc9ee09d4a5c7/src/index.ts#L252-L269
Server side SDKs don't suffer from this problem because they need the context to evaluate isEnabled, therefore it's guaranteed that the context used for the evaluation is the same one sent in the impression event.
There is a solution for this... the cache should also store the current context and should return it when we get a toggle. It'd be something like: val (toggle, context) = cache.get(toggleName)
No description provided.