Update to support UFC configuration files via using the shared common SDK #70
Conversation
| @@ -1,5 +1,7 @@ | |||
| src/test/resources/shared | |||
There was a problem hiding this comment.
putting the shared test files in the same place the shared common repository does for consistency and to make it easier to reuse testing tools
| testImplementation platform('org.junit:junit-bom:5.10.2') | ||
| testImplementation 'org.junit.jupiter:junit-jupiter' | ||
| testImplementation 'com.github.tomakehurst:wiremock-jre8:2.35.2' | ||
| testImplementation 'org.mockito:mockito-core:4.11.0' | ||
| testImplementation 'com.squareup.okhttp3:okhttp:4.9.1' |
There was a problem hiding this comment.
Needed to mock stuff regarding Reponse
| import java.util.*; | ||
| import java.util.stream.Collectors; | ||
| import org.ehcache.Cache; | ||
| import cloud.eppo.BaseEppoClient; |
There was a problem hiding this comment.
It's not worth comparing the diff here, just look at the new (green) code to see what's going on. I'll add comments to help guide.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class EppoClient { | ||
| public class EppoClient extends BaseEppoClient { |
There was a problem hiding this comment.
The way things worked out, I opting for inheritance. My first pass did composition but then getInstance() was returning BaseEppoClient and there were some protected things (like loadConfigurations() I wanted to be able to leverage but not exposed outside of this class.
It may seem like an odd pattern choice (for now), as the only thing really overridden is the constructor. However it works well and I think it will be more useful for android when we start dealing with thing like caching.
| } | ||
|
|
||
| /** This function is used to get assignment Value */ | ||
| protected Optional<EppoValue> getAssignmentValue( |
There was a problem hiding this comment.
All getAssignment()-like methods now live in BaseEppoClient
| InputValidator.validateNotBlank(subjectKey, "Invalid argument: subjectKey cannot be blank"); | ||
| InputValidator.validateNotBlank(flagKey, "Invalid argument: flagKey cannot be blank"); | ||
| private static EppoClient instance; | ||
| private static Timer pollTimer; |
There was a problem hiding this comment.
The poller made static so that it is a singleton as well, and we don't have renegade timers everywhere. Also so it can easily be stopped (useful for tests)
| subjectCategoricalAttributes, | ||
| actionNumericAttributes, | ||
| actionCategoricalAttributes)); | ||
| public Builder forceReinitialize(boolean forceReinitialize) { |
There was a problem hiding this comment.
Latest and greatest from our JavaScript repository 📈
| * @param actionAttributes attributes associated with the given action | ||
| * @return null if no exception was encountered by logging; otherwise, the encountered exception | ||
| */ | ||
| public Exception logNonBanditAction( |
There was a problem hiding this comment.
We decided to ax this functionality 🪓 The juice was not worth the squeeze
| @@ -7,38 +7,31 @@ | |||
|
|
|||
| public class FetchConfigurationsTask extends TimerTask { | |||
| private static final Logger log = LoggerFactory.getLogger(FetchConfigurationsTask.class); | |||
| private final ConfigurationStore configurationStore; | |||
| private final Runnable runnable; | |||
There was a problem hiding this comment.
By passing in a runable we can pass in a lambda which is nice for, among other things, having this leverage a protected BaseEppoClient method.
|
|
||
| @BeforeAll | ||
| public static void initMockServer() { | ||
| mockServer = new WireMockServer(TEST_PORT); |
There was a problem hiding this comment.
Unlike the shared common repository, these test cases use a mock local server, which makes them much faster. 🐎
| @@ -1,34 +1,33 @@ | |||
| package com.eppo.sdk; | |||
|
|
|||
| import static cloud.eppo.helpers.AssignmentTestCase.parseTestCaseFile; | |||
There was a problem hiding this comment.
This is another file where I'd ignore the diff and just look at the new (green) code
| Stream.of("option1", "option2", "option3").collect(Collectors.toSet()); | ||
| new EppoClient.Builder() | ||
| .apiKey(DUMMY_FLAG_API_KEY) | ||
| .pollingIntervalMs(20) |
There was a problem hiding this comment.
With Java, it was much easier to just use very small timeouts than to mock time out (or mock the Timer) like we do in JavaScript
| @@ -34,11 +34,12 @@ dependencies { | |||
| // Logback classic 1.3.x is compatible with java 8 | |||
| implementation 'ch.qos.logback:logback-classic:1.3.14' | |||
|
|
|||
| testImplementation 'org.projectlombok:lombok:1.18.24' | |||
| testImplementation 'cloud.eppo:sdk-common-jvm:3.0.0-SNAPSHOT:tests' | |||
There was a problem hiding this comment.
interesting, didn't know about this syntax
build.gradle
Outdated
| @@ -25,7 +25,7 @@ repositories { | |||
| } | |||
|
|
|||
| dependencies { | |||
| implementation 'cloud.eppo:sdk-common-jvm:2.0.0-SNAPSHOT' | |||
| implementation 'cloud.eppo:sdk-common-jvm:3.0.0-SNAPSHOT' | |||
There was a problem hiding this comment.
we need to remember to publish sdk-common-jvm v3.0.0 and bump it here to the release version before releasing this lib
| @@ -6,7 +6,7 @@ plugins { | |||
| } | |||
|
|
|||
| group = 'cloud.eppo' | |||
| version = '2.5.0-SNAPSHOT' | |||
| version = '3.0.0-SNAPSHOT' | |||
| @@ -7,38 +7,31 @@ | |||
|
|
|||
| public class FetchConfigurationsTask extends TimerTask { | |||
There was a problem hiding this comment.
public class JitteryTimerTask?
Eppo Internal:
🎟️ Ticket: FF-2842 - Java SDK UFC and Bandits Tests Passing
👯♀️ Related PR:
sdk-common-jdk #29This updates the Java SDK to leverage the shared common SDK (
sdk-common-jdk), which upgrades it to using the Universal Flag Configuration (UFC) files.The tests for
EppoClienthave been updated to cover:sdk-test-data)sdk-test-data)Note: this won't successfully run integration tests until the common SDK changes have been merged and published. However, using the updated common repository published to a local maven repository, the tests pass:
