-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: In-memory provider for e2e testing and minimal usage #546
feat: In-memory provider for e2e testing and minimal usage #546
Conversation
Signed-off-by: liran2000 <liran2000@gmail.com>
private Flags.State state; | ||
private Map<String, Object> variants; | ||
private String defaultVariant; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose adding a context evaluator backed by a user-provided callback
See go-sdk implementation for example - https://github.com/open-feature/go-sdk/blob/main/pkg/openfeature/memprovider/in_memory_provider.go#L167-L175
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand what is the purpose and value here with enhancing with additional testing capabilities. The in-memory provider is for testing purposes, how is adding a context evaluator helping with testing actual flow ? it will only test the testing provider ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation resides as a test
source. This mismatch with the requirement of the specification.
When we agreed on the in-memory provider through OFEP and added it to the specification 1, the agreement was to make the in-memory an extra provider built into the SDK itself. And we agreed to support evaluation contexts through lambda/callback.
So the packaging of the implementation should be corrected first. I am proposing to move it to a package named memprovider
or any better-named package.
Regarding the purpose, the main benefit of having evaluation context support is testing SDK. It allows us to write end-to-end tests (or to migrate existing ones based on flagd) for context evaluations and verify SDK correctness. Besides, end users can use the provider to prototype OpenFeature features.
Footnotes
Overall great work. I would love to get this to mergeable status. Please check comments from my initial review and let me know if you have any concerns |
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@@ -16,20 +16,6 @@ If you think we might be out of date with the spec, you can check that by invoki | |||
|
|||
If you're adding tests to cover something in the spec, use the `@Specification` annotation like you see throughout the test suites. | |||
|
|||
## End-to-End Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than removing, IMO we should mention that the e2e tests use InMemoryProvider
. Consider the GO SDK readme for reference - https://github.com/open-feature/go-sdk/blob/main/e2e/README.md
File file = new File(classLoader.getResource("features/testing-flags.json").getFile()); | ||
Path resPath = file.toPath(); | ||
String conf = new String(java.nio.file.Files.readAllBytes(resPath), "UTF8"); | ||
Flags flags = Flags.builder().setConfigurationJson(conf).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused and unnecessary 🤔 Was the intention here to fail fast with flag configuration validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover, will remove it
Some observations and suggestions for improvement In the current implementation, flag configurations are loaded from a string. I think this is not ideal for general usage and diverge from the specification 1
I am suggesting changing the constructor to accept a known map of flags. The end user may use any mechanism to parse their flags and initialize the provider. Further, this should allow us to simplify Footnotes |
- move to non-test package - add ContextEvaluator - update Flags structure - implement events provider - enrich tests - refactor Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
Signed-off-by: liran2000 <liran2000@gmail.com>
@@ -494,62 +486,6 @@ | |||
</plugins> | |||
</build> | |||
</profile> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to remove this profile. It is still needed and works normally
Changes you made at StepDefinisions.java [1] is sufficient to fulfill the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this section, we do not initialize the test-harness submodule. See the build result - https://github.com/open-feature/java-sdk/actions/runs/5824793560/job/15795407914?pr=546
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this comment it can be removed, if you still think it should remain update here and I can return it.
<!-- this profile handles running the flagd e2e tests -->
<!-- TODO: this profile can likely be removed with TODO: this section should be updated with https://github.com/open-feature/java-sdk/issues/523 -->
<!-- TODO: we should pull the submodule and run these tests unconditionall once flagd isn't required -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we still need them. Tests are now independent from flagd but we need this profile to intialize git submodule and run gherkin tetsts
See the GitHub action - https://github.com/open-feature/java-sdk/blob/main/.github/workflows/pullrequest.yml#L42-L43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we could just always pull the submodule and copy the gherkin tests, since now they don't require the external flagd process. I dont really mind if we keep these integration tests on a separate profile or not.
I think if we keep the profile though, we should add back the little bit of documentation in the contributing guide about it.
.build(); | ||
} | ||
|
||
private static Value objectToValue(Object object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be exposed in Value
class itself. This will help other provider implementations
* @param map map of objects | ||
* @return a Structure object in the SDK format | ||
*/ | ||
public static Structure mapToStructure(Map<String, Object> map) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be exposed from MutableStructure
class so that it can be commonly used where needed
@@ -0,0 +1,184 @@ | |||
package dev.openfeature.sdk.providers.memory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good package name and differentiate from contrib provider implemenations (dev.openfeature.contrib.providers.*
)
provider.updateFlags(flags); | ||
} | ||
|
||
public static Map<String, Flag<?>> buildFlags() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the public accessor status & multiple usages, I think this fits well into testutils - https://github.com/open-feature/java-sdk/tree/main/src/test/java/dev/openfeature/sdk/testutils
Probably with a class named TestFlags
import dev.openfeature.sdk.Reason; | ||
import dev.openfeature.sdk.Structure; | ||
import dev.openfeature.sdk.Value; | ||
import dev.openfeature.sdk.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider avoiding wildcard imports (check other places too)
Signed-off-by: liran2000 <liran2000@gmail.com>
|
||
/** | ||
* Building flags for testing purposes. | ||
* @return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggesting the removal as return is not defined
|
||
@SneakyThrows | ||
@Test | ||
void mapToStructureTest() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to StructureTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the latest reviews, I do not see any other concerns. Approval from my end :)
Signed-off-by: liran2000 <liran2000@gmail.com>
src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java
Show resolved
Hide resolved
src/main/java/dev/openfeature/sdk/providers/memory/InMemoryProvider.java
Outdated
Show resolved
Hide resolved
src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java
Outdated
Show resolved
Hide resolved
* Building flags for testing purposes. | ||
* @return map of flags | ||
*/ | ||
public static Map<String, Flag<?>> buildFlags() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this API looks pretty solid. It's what I expected. Interested in @justinabrahms @thisthat 's takes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks really good, thanks @liran2000 !!
I have a few small comments, in particular this one (unless I'm missing something). Also watch out for the Javadoc stuff I mentioned to make sure people don't accidentally think some of these classes are for general SDK usage.
I think with a few small changes this can be merged!
Signed-off-by: liran2000 <liran2000@gmail.com>
src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java
Outdated
Show resolved
Hide resolved
@liran2000 don't worry about the codecov... IMHO your coverage of new code is fine, but there's an issue with one test. |
Signed-off-by: liran2000 <liran2000@gmail.com>
verify emit event called on the spied provider as it verifies the config changed event was emitted, which is the in-memory provider implementation. The event itself is a functionality of the events framework, thus not needed to be tested here, Signed-off-by: liran2000 <liran2000@gmail.com>
…o feat/523-in-memory-provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good to me, and tests look stable now. Thanks again @liran2000
Approving. I will wait until EOD before merging to give others a chance to review.
Kudos, SonarCloud Quality Gate passed!
|
@liran2000 I opened a small PR to satisfy codecov: #561, and made some other minor changes I didn't notice here. |
This PR
Issue 523 - In-memory provider for e2e testing and minimal SDK usage