feat: add flagd-api-testkit module#1742
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a dedicated testkit for the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new flagd-api-testkit module, which is a significant and well-designed addition for ensuring compliance of flagd-api Evaluator implementations. The use of a git submodule for test harnesses and bundling them into the JAR is a clever approach to simplify consumer setup. The SPI-based discovery of the evaluator factory is also a great design choice for minimizing boilerplate code for consumers.
My review focuses on improving the robustness and maintainability of the new testkit. I've identified a bug in context creation where value types are ignored, a point where brittle reflection can be replaced with public API usage, and a seemingly leftover file that causes a compilation error. Addressing these points will make the testkit more reliable and easier to maintain.
...-api-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/ContextSteps.java
Outdated
Show resolved
Hide resolved
...lagd-core/src/test/java/dev/openfeature/contrib/tools/flagd/core/e2e/FlagdCoreInitSteps.java
Outdated
Show resolved
Hide resolved
...i-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluationSteps.java
Outdated
Show resolved
Hide resolved
Adds a new Maven module tools/flagd-api-testkit that provides: - Reusable Cucumber step definitions (EvaluationSteps, ContextSteps) for testing any implementation of the flagd-api Evaluator interface - Gherkin feature files and flag configurations bundled into the release JAR via the test-harness git submodule (evaluation, fractional, semver, string, targeting, evaluator-refs, metadata features) - TestkitFlags utility to load bundled flag JSON from classpath - No testcontainers required — pure in-memory evaluation testing Consumers only need to provide a single @given("an evaluator") step that wires up their Evaluator implementation, plus a Cucumber suite runner. Integrates tools/flagd-core as the reference consumer: - FlagdCoreInitSteps provides the @given("an evaluator") step - FlagdCoreEvaluatorTest runs the 62 testkit Cucumber scenarios Build pipeline follows the flagd-core/schemas pattern: - exec-maven-plugin inits the test-harness submodule - maven-resources-plugin copies gherkin/flags into src/main/resources so they are packaged into the release JAR Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
…pattern Cucumber forbids extending step definition classes, so the abstract base class approach is replaced with a @FunctionalInterface factory: - Add EvaluatorFactory — @FunctionalInterface taking flagsJson String - Add EvaluatorInitSteps — concrete testkit class owning @given and @before - Add factory field to EvaluatorState with get/setFactory - Remove AbstractEvaluatorInitSteps Consumer setup is now two annotations and a lambda: public class MySetup { private final EvaluatorState state; public MySetup(EvaluatorState state) { this.state = state; } @before public void registerFactory() { state.setFactory(flagsJson -> { MyEvaluator e = new MyEvaluator(); e.setFlags(flagsJson); return e; }); } } Flags are loaded inside the testkit (EvaluatorInitSteps.anEvaluator), passed as a String to the factory so the consumer can call setFlags on their concrete evaluator type. FlagdCoreInitSteps updated to use the @before factory pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Replaces the @Before-hook factory registration pattern with Java SPI (ServiceLoader), eliminating all Cucumber knowledge from the consumer. Design: - EvaluatorFactory is the SPI interface (@FunctionalInterface) - EvaluatorInitSteps.anEvaluator() discovers the factory via ServiceLoader.load(EvaluatorFactory.class) — no state.getFactory() - AbstractEvaluatorTest implements EvaluatorFactory (abstract create()) and carries all @suite / Cucumber annotations; GLUE_PROPERTY_NAME only needs the testkit package - AbstractEvaluatorSetup removed — no longer needed Consumer setup is now two artifacts: 1. One class extending AbstractEvaluatorTest, implementing create(): public class MyEvaluatorTest extends AbstractEvaluatorTest { public Evaluator create(String flagsJson) throws Exception { MyEvaluator e = new MyEvaluator(); e.setFlags(flagsJson); return e; } } 2. META-INF/services/...EvaluatorFactory containing the class name No @before, no @ConfigurationParameter, no Cucumber imports in the consumer class. FlagdCoreEvaluatorTest updated as the reference implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
…ranch Points the testkit submodule to the feat/evaluator-testkut branch commit which adds evaluator/gherkin/ and evaluator/flags/ — the source files for the bundled testkit features and flag configuration. See: open-feature/flagd-testbed#344 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
…ile error - ContextSteps: use EvaluatorUtils.convert() for proper type conversion instead of always creating string values; fix MutableContext.add() compile error by using map constructor approach; add missing IOException and Map imports - EvaluationSteps: replace private field reflection with ImmutableMetadata.asUnmodifiableMap() - pom.xml: pin explicit versions for junit-platform-suite (1.14.3) and cucumber-junit-platform-engine (7.34.3) to fix compile-scope resolution - Remove FlagdCoreInitSteps.java (no longer needed with SPI pattern) Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…fixes) Bumps to ad47bad which fixes gherkin lint violations: - no-background-only-scenario in evaluation, evaluator-refs, string, targeting - no-homogenous-tags in no-default-variant Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new flagd-api-testkit module, which provides a compliance testkit for flagd-api Evaluator implementations. The design is well-thought-out, using a git submodule for test harnesses, Maven resource plugin to bundle test files, and Java SPI for easy integration by consumers. The changes also include updating flagd-core to use this new testkit as a reference implementation. The code is clean and the documentation is clear. I have a few minor suggestions to improve code quality and maintainability.
...i-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluationSteps.java
Outdated
Show resolved
Hide resolved
...i-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluationSteps.java
Outdated
Show resolved
Hide resolved
...i-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluationSteps.java
Outdated
Show resolved
Hide resolved
...pi-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluatorUtils.java
Outdated
Show resolved
Hide resolved
…ws, remove stale annotations - pom.xml: remove hardcoded versions for junit-platform-suite and cucumber-junit-platform-engine; versions are managed by junit-bom/cucumber-bom in parent; keep explicit compile scope for junit-platform-suite - EvaluationSteps: narrow throws Throwable -> throws IOException on givenAFlag and resolvedValueEquals - EvaluationSteps: remove unused @SuppressWarnings(unchecked) and throws ClassNotFoundException from resolvedMetadataContains - EvaluatorUtils: remove unused @SuppressWarnings(unchecked) from convert() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new flagd-api-testkit module, which is a compliance testkit for flagd-api Evaluator implementations. The changes are well-structured and the new module is a great addition for ensuring compliance. I've provided a few suggestions to improve maintainability and code clarity in the new module.
...-api-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/ContextSteps.java
Show resolved
Hide resolved
...pi-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluatorUtils.java
Outdated
Show resolved
Hide resolved
…fy Object conversion
- pom.xml: extract assertj and jackson-databind versions into properties for maintainability
- EvaluatorUtils: remove redundant value.equals("{}") check; Jackson handles "{}" correctly in the else branch
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new flagd-api-testkit Maven module, which serves as a compliance testkit for flagd-api Evaluator implementations. The design is solid, leveraging a git submodule for test harnesses and Java's SPI for extensibility, which allows for minimal setup by consumers. The new module is well-documented and includes a reference implementation update in flagd-core.
My review has identified a few critical issues in the pom.xml of the new module concerning invalid dependency and plugin versions that could cause build failures. Additionally, I've provided a suggestion to refactor a small portion of the code for better clarity and efficiency. Overall, this is a great addition to the project.
...-api-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/ContextSteps.java
Show resolved
Hide resolved
Bumps to 9a522b5 which removes the Background block from no-default-variant.feature to satisfy the no-background-only-scenario gherkin lint rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
...pi-testkit/src/main/java/dev/openfeature/contrib/tools/flagd/api/testkit/EvaluatorState.java
Outdated
Show resolved
Hide resolved
- Add FlagType enum (BOOLEAN, STRING, INTEGER, FLOAT, OBJECT) with Gherkin name mapping and fromString() factory - EvaluatorState.flagType is now FlagType instead of String - EvaluationSteps: parse incoming Gherkin type token via FlagType.fromString(), switch on enum constants instead of string literals - EvaluatorUtils: add FlagType overload of convert() delegating to String variant; String overload retained for metadata type resolution (resolvedMetadataContains) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
v3.1.0 of open-feature/flagd-testbed includes the evaluator testkit gherkin features and flag configuration (PR open-feature#344), so the submodule no longer needs to track a feature branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Setting branch = v3.1.0 in .gitmodules so Renovate tracks version tags for auto-updates rather than following main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
3 participants
Summary
Adds a new Maven module
tools/flagd-api-testkit— a compliance testkit for verifying implementations of theflagd-apiEvaluatorinterface.Design goals
open-feature/test-harnesssubmodule and packaged into the release JAR; consumers need no submodule of their ownConsumer setup
Extend
AbstractEvaluatorTestand implementcreate():Register via SPI in
src/test/resources/META-INF/services/dev.openfeature.contrib.tools.flagd.api.testkit.EvaluatorFactory:No
@Before, no@ConfigurationParameter, no Cucumber imports in the consumer class. TheEvaluatorFactoryis discovered viajava.util.ServiceLoaderinside the testkit's@Given("an evaluator")step.Feature files (10 files, split by concern)
evaluation.featurezero-values.featureno-default-variant.featuredefaultVariantedge caseserrors.featureFLAG_NOT_FOUND,TYPE_MISMATCHfractional.featuresemver.featurestring.featurestarts_with/ends_withtargeting.featureevaluator-refs.feature$refevaluator reusemetadata.featureReference implementation
tools/flagd-coreserves as the reference consumer — all 62 Cucumber scenarios pass.Checklist
tools/flagd-api-testkitwithversion.txt,CHANGELOG.md,README.mdpom.xmlandrelease-please-config.jsontools/flagd-api-testkit/test-harnessmaven-resources-pluginflagd-coreupdated to use testkit as reference consumer (62/62 scenarios pass)