feat: add integration tests for Java generator#220
feat: add integration tests for Java generator#220vikasrao23 wants to merge 2 commits intoopen-feature:mainfrom
Conversation
Implements integration test coverage for the Java generator following the standardized Dagger-based testing pattern. Changes: - test/integration/cmd/java/run.go - Dagger test runner for Java - test/java-integration/pom.xml - Maven configuration with OpenFeature SDK - test/java-integration/src/main/java/dev/openfeature/Main.java - Test program - test/integration/cmd/run.go - Added Java test to main runner - Makefile - Added test-integration-java target The test: 1. Builds the CLI from source 2. Generates Java feature flag client code 3. Compiles and executes the generated code with Maven 4. Validates all flag types (boolean, string, int, double, object) 5. Runs as part of CI suite via test-integration target Acceptance criteria met: ✅ Java generator covered by integration test ✅ Generated Java code compiles and passes execution ✅ Test integrated into CI suite ✅ Follows documented integration testing structure Closes open-feature#115 Signed-off-by: vikasrao23 <vikasrao23@users.noreply.github.com>
Summary of ChangesHello @vikasrao23, 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 significantly enhances the testing infrastructure by introducing comprehensive integration tests for the Java code generator. It ensures that the generated Java code correctly compiles, runs, and accurately evaluates various flag types, aligning with the existing Dagger-based testing framework used for other language generators. This addition improves the reliability and maintainability of the Java generator. 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 adds integration tests for the Java generator, which is a great addition for ensuring code quality and correctness. The changes follow the existing patterns for integration tests in the repository. I've made a few suggestions to improve code clarity, robustness, and consistency in the new test files. My feedback includes using a more reliable method for determining the current directory in the Go test runner, using modern Java features for map initialization, and aligning the test build process with repository standards by including version information.
| WithWorkdir("/src"). | ||
| WithExec([]string{"go", "mod", "tidy"}). | ||
| WithExec([]string{"go", "mod", "download"}). | ||
| WithExec([]string{"go", "build", "-o", "cli", "./cmd/openfeature"}) |
There was a problem hiding this comment.
The build command for the CLI is missing the ldflags to inject version information. This is inconsistent with the build target in the Makefile and general repository rules about including commit hashes in builds.
While this seems to be a pre-existing pattern in other integration tests, it would be a good improvement to start aligning them. You could pass the commit hash from the host (e.g., from an environment variable) and use it to construct the ldflags for the build.
For example:
commit := os.Getenv("COMMIT_SHA") // assuming it's passed from the calling script
ldflags := fmt.Sprintf("-s -w -X 'main.commit=%s'", commit)
// ...
cli := client.Container().
// ...
WithExec([]string{"go", "build", "-ldflags", ldflags, "-o", "cli", "./cmd/openfeature"})This would require changes in how the test is invoked to pass the commit hash.
References
- Use the full commit hash for release builds and the short commit hash for development builds to differentiate them at a glance.
test/integration/cmd/java/run.go
Outdated
| projectDir, err := filepath.Abs(os.Getenv("PWD")) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to get project dir: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| // Get test directory | ||
| testDir, err := filepath.Abs(filepath.Join(projectDir, "test/java-integration")) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to get test dir: %v\n", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
For better robustness and clarity, you can simplify the directory path resolution. os.Getwd() is generally more reliable than os.Getenv("PWD") for getting the current working directory. You can also simplify the test directory path construction as os.Getwd() returns an absolute path.
| projectDir, err := filepath.Abs(os.Getenv("PWD")) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to get project dir: %v\n", err) | |
| os.Exit(1) | |
| } | |
| // Get test directory | |
| testDir, err := filepath.Abs(filepath.Join(projectDir, "test/java-integration")) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to get test dir: %v\n", err) | |
| os.Exit(1) | |
| } | |
| projectDir, err := os.Getwd() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Failed to get project dir: %v\n", err) | |
| os.Exit(1) | |
| } | |
| // Get test directory | |
| testDir := filepath.Join(projectDir, "test/java-integration") |
| Map<String, Flag<?>> flags = new HashMap<>(); | ||
|
|
||
| flags.put("discountPercentage", Flag.builder() | ||
| .variant("default", 0.15) | ||
| .defaultVariant("default") | ||
| .build()); | ||
|
|
||
| flags.put("enableFeatureA", Flag.builder() | ||
| .variant("default", false) | ||
| .defaultVariant("default") | ||
| .build()); | ||
|
|
||
| flags.put("greetingMessage", Flag.builder() | ||
| .variant("default", "Hello there!") | ||
| .defaultVariant("default") | ||
| .build()); | ||
|
|
||
| flags.put("usernameMaxLength", Flag.builder() | ||
| .variant("default", 50) | ||
| .defaultVariant("default") | ||
| .build()); | ||
|
|
||
| Map<String, Object> themeConfig = new HashMap<>(); | ||
| themeConfig.put("primaryColor", "#007bff"); | ||
| themeConfig.put("secondaryColor", "#6c757d"); | ||
|
|
||
| flags.put("themeCustomization", Flag.builder() | ||
| .variant("default", new Value(themeConfig)) | ||
| .defaultVariant("default") | ||
| .build()); |
There was a problem hiding this comment.
Since you are using Java 11, you can use Map.of() to create immutable maps for flags and themeConfig. This makes the code more concise and clearly expresses the intent that these maps are not modified after initialization.
Map<String, Object> themeConfig = Map.of(
"primaryColor", "#007bff",
"secondaryColor", "#6c757d"
);
Map<String, Flag<?>> flags = Map.of(
"discountPercentage", Flag.builder()
.variant("default", 0.15)
.defaultVariant("default")
.build(),
"enableFeatureA", Flag.builder()
.variant("default", false)
.defaultVariant("default")
.build(),
"greetingMessage", Flag.builder()
.variant("default", "Hello there!")
.defaultVariant("default")
.build(),
"usernameMaxLength", Flag.builder()
.variant("default", 50)
.defaultVariant("default")
.build(),
"themeCustomization", Flag.builder()
.variant("default", new Value(themeConfig))
.defaultVariant("default")
.build()
);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for the Java generator, enhancing code quality and ensuring the generated Java code compiles and executes correctly. The changes align with the existing Dagger-based testing pattern, which is a good practice for consistency and reproducibility. The new Makefile targets make it easy to run these tests. Overall, the changes are well-structured and contribute positively to the project's test coverage.
| // Build the CLI | ||
| cli := client.Container(). | ||
| From("golang:1.24-alpine"). | ||
| WithExec([]string{"apk", "add", "--no-cache", "git"}). |
There was a problem hiding this comment.
It's generally recommended to specify a full version tag for base images in production or CI environments to ensure reproducibility and prevent unexpected changes if golang:1.24-alpine is updated to a newer minor version that introduces breaking changes. For example, golang:1.24.2-alpine.
|
|
||
| // Test Java compilation with the generated files | ||
| javaContainer := client.Container(). | ||
| From("maven:3.9-eclipse-temurin-21-alpine"). |
There was a problem hiding this comment.
Similar to the Go base image, it's good practice to pin the Maven image to a specific patch version (e.g., maven:3.9.6-eclipse-temurin-21-alpine) to avoid unexpected behavior from minor version updates.
test/java-integration/pom.xml
Outdated
| <dependency> | ||
| <groupId>dev.openfeature</groupId> | ||
| <artifactId>sdk</artifactId> | ||
| <version>1.14.0</version> |
There was a problem hiding this comment.
It's a good practice to use a property for the SDK version (e.g., <openfeature.sdk.version>1.14.0</openfeature.sdk.version>) and then reference it as ${openfeature.sdk.version}. This centralizes version management and makes it easier to update dependencies.
test/java-integration/pom.xml
Outdated
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.11.0</version> |
There was a problem hiding this comment.
Similar to the SDK version, it's beneficial to use a property for the maven-compiler-plugin version to keep all plugin versions consistent and easily manageable.
test/java-integration/pom.xml
Outdated
| <source>11</source> | ||
| <target>11</target> | ||
| </configuration> |
There was a problem hiding this comment.
The source and target versions are already defined in the <properties> section. You can remove these redundant configurations here to avoid duplication and ensure a single source of truth for the Java version.
| <source>11</source> | |
| <target>11</target> | |
| </configuration> | |
| <configuration> | |
| </configuration> |
| if (enableFeatureADetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating boolean flag"); | ||
| } |
There was a problem hiding this comment.
The error message "Error evaluating boolean flag" is generic. It would be more helpful to include the flag key in the error message to quickly identify which flag caused the issue, especially when dealing with multiple flags.
| if (enableFeatureADetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating boolean flag"); | |
| } | |
| if (enableFeatureADetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating boolean flag: " + enableFeatureADetails.getFlagKey()); | |
| } |
| if (discountDetails.getErrorCode() != null) { | ||
| throw new Exception("Failed to get discount"); | ||
| } |
There was a problem hiding this comment.
Similar to the boolean flag, including the flag key in the error message "Failed to get discount" would improve debugging by pinpointing the exact flag that failed.
| if (discountDetails.getErrorCode() != null) { | |
| throw new Exception("Failed to get discount"); | |
| } | |
| if (discountDetails.getErrorCode() != null) { | |
| throw new Exception("Failed to get discount for flag: " + discountDetails.getFlagKey()); | |
| } |
| if (greetingDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating string flag"); | ||
| } |
There was a problem hiding this comment.
Adding the flag key to the error message "Error evaluating string flag" will make it easier to diagnose issues when multiple string flags are being evaluated.
| if (greetingDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating string flag"); | |
| } | |
| if (greetingDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating string flag: " + greetingDetails.getFlagKey()); | |
| } |
| if (usernameDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating int flag"); | ||
| } |
There was a problem hiding this comment.
Including the flag key in the error message "Error evaluating int flag" would provide more specific context for debugging integer flag evaluation failures.
| if (usernameDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating int flag"); | |
| } | |
| if (usernameDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating int flag: " + usernameDetails.getFlagKey()); | |
| } |
| if (themeDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating object flag"); | ||
| } |
There was a problem hiding this comment.
For object flags, a more descriptive error message including the flag key would be beneficial for troubleshooting, e.g., "Error evaluating object flag: themeCustomization".
| if (themeDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating object flag"); | |
| } | |
| if (themeDetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating object flag: " + themeDetails.getFlagKey()); | |
| } |
|
Thanks for the review! Working on addressing all the feedback now. |
- Use os.Getwd() for better portability and simplify path construction - Update Java version from 11 to 21 to match Docker image - Use Map.of() for immutable maps (Java 9+ best practice) - Add properties for SDK and plugin versions for centralized management - Remove redundant source/target from maven-compiler-plugin configuration - Include flag keys in error messages for better debugging - Remove unused HashMap import
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds valuable integration test coverage for the Java generator, following the established Dagger-based pattern. The implementation is solid and enhances the project's test suite. I've identified a few opportunities for improvement, mainly concerning code duplication in the test runner, a discrepancy in the specified Java version, and repeated logic in the Java test code. Addressing these points will improve the maintainability and clarity of the new test infrastructure.
| javaCmd := exec.Command("go", "run", "github.com/open-feature/cli/test/integration/cmd/java") | ||
| javaCmd.Stdout = os.Stdout | ||
| javaCmd.Stderr = os.Stderr | ||
| if err := javaCmd.Run(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error running Java integration test: %v\n", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
While this works, the pattern for running each integration test is duplicated multiple times in this main function. This makes the file harder to maintain and more error-prone when adding new tests. Consider refactoring to use a loop to execute the tests.
For example, the main function could be simplified to:
func main() {
fmt.Println("=== Running all integration tests ===")
tests := []string{
"csharp",
"go",
"nodejs",
"angular",
"java",
}
for _, test := range tests {
cmd := exec.Command("go", "run", "github.com/open-feature/cli/test/integration/cmd/"+test)
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
fmt.Fprintf(os.Stderr, "Error running %s integration test: %v\n", test, err)
os.Exit(1)
}
}
fmt.Println("=== All integration tests passed successfully ===")
}| <maven.compiler.source>21</maven.compiler.source> | ||
| <maven.compiler.target>21</maven.compiler.target> |
There was a problem hiding this comment.
The PR description states that the minimum Java version is 11, but this pom.xml is configured for Java 21. To align with the documentation and ensure the generated code is compatible with the stated minimum version, please update the compiler source and target versions to 11.
| <maven.compiler.source>21</maven.compiler.source> | |
| <maven.compiler.target>21</maven.compiler.target> | |
| <maven.compiler.source>11</maven.compiler.source> | |
| <maven.compiler.target>11</maven.compiler.target> |
| FlagEvaluationDetails<Boolean> enableFeatureADetails = EnableFeatureA.valueWithDetails(client, evalContext); | ||
| if (enableFeatureADetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating boolean flag: " + enableFeatureADetails.getFlagKey()); | ||
| } |
There was a problem hiding this comment.
This error-checking logic is repeated for each flag type evaluation. To improve code readability and maintainability, you could extract this into a private helper method.
For example, you could add this method to the Main class:
private static <T> void checkDetails(FlagEvaluationDetails<T> details, String flagType) throws Exception {
if (details.getErrorCode() != null) {
throw new Exception("Error evaluating " + flagType + " flag: " + details.getFlagKey());
}
}Then you can replace this block and similar ones with a single call to this new helper method.
| FlagEvaluationDetails<Boolean> enableFeatureADetails = EnableFeatureA.valueWithDetails(client, evalContext); | |
| if (enableFeatureADetails.getErrorCode() != null) { | |
| throw new Exception("Error evaluating boolean flag: " + enableFeatureADetails.getFlagKey()); | |
| } | |
| checkDetails(EnableFeatureA.valueWithDetails(client, evalContext), "boolean"); |
|
✅ All feedback from Gemini code review has been addressed:
Ready for re-review. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds integration tests for the Java generator, which is a great addition for ensuring the quality of the generated code. The changes follow the existing Dagger-based testing pattern. My review includes a few suggestions to improve consistency with the stated goals (targeting Java 11) and to enhance the maintainability of the new test code.
|
|
||
| // Test Java compilation with the generated files | ||
| javaContainer := client.Container(). | ||
| From("maven:3.9-eclipse-temurin-21-alpine"). |
There was a problem hiding this comment.
To align with the goal of supporting Java 11 as the minimum version (as mentioned in the PR description and the suggested change in pom.xml), the Docker image used for the test should also be a Java 11-based image.
| From("maven:3.9-eclipse-temurin-21-alpine"). | |
| From("maven:3.9-eclipse-temurin-11-alpine"). |
| <maven.compiler.source>21</maven.compiler.source> | ||
| <maven.compiler.target>21</maven.compiler.target> |
There was a problem hiding this comment.
The PR description mentions that the Java generator targets a minimum version of Java 11. However, the pom.xml is configured for Java 21. To ensure the integration test validates compatibility with the intended minimum version, you should update the compiler source and target to 11. You will also need to update the Docker image in test/integration/cmd/java/run.go to use a Java 11 image.
| <maven.compiler.source>21</maven.compiler.source> | |
| <maven.compiler.target>21</maven.compiler.target> | |
| <maven.compiler.source>11</maven.compiler.source> | |
| <maven.compiler.target>11</maven.compiler.target> |
| Boolean enableFeatureA = EnableFeatureA.value(client, evalContext); | ||
| System.out.println("enableFeatureA: " + enableFeatureA); | ||
| FlagEvaluationDetails<Boolean> enableFeatureADetails = EnableFeatureA.valueWithDetails(client, evalContext); | ||
| if (enableFeatureADetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating boolean flag: " + enableFeatureADetails.getFlagKey()); | ||
| } | ||
|
|
||
| Double discount = DiscountPercentage.value(client, evalContext); | ||
| System.out.printf("Discount Percentage: %.2f%n", discount); | ||
| FlagEvaluationDetails<Double> discountDetails = DiscountPercentage.valueWithDetails(client, evalContext); | ||
| if (discountDetails.getErrorCode() != null) { | ||
| throw new Exception("Failed to get discount for flag: " + discountDetails.getFlagKey()); | ||
| } | ||
|
|
||
| String greetingMessage = GreetingMessage.value(client, evalContext); | ||
| System.out.println("greetingMessage: " + greetingMessage); | ||
| FlagEvaluationDetails<String> greetingDetails = GreetingMessage.valueWithDetails(client, evalContext); | ||
| if (greetingDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating string flag: " + greetingDetails.getFlagKey()); | ||
| } | ||
|
|
||
| Integer usernameMaxLength = UsernameMaxLength.value(client, evalContext); | ||
| System.out.println("usernameMaxLength: " + usernameMaxLength); | ||
| FlagEvaluationDetails<Integer> usernameDetails = UsernameMaxLength.valueWithDetails(client, evalContext); | ||
| if (usernameDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating int flag: " + usernameDetails.getFlagKey()); | ||
| } | ||
|
|
||
| Value themeCustomization = ThemeCustomization.value(client, evalContext); | ||
| FlagEvaluationDetails<Value> themeDetails = ThemeCustomization.valueWithDetails(client, evalContext); | ||
| if (themeDetails.getErrorCode() != null) { | ||
| throw new Exception("Error evaluating object flag: " + themeDetails.getFlagKey()); | ||
| } | ||
| System.out.println("themeCustomization: " + themeCustomization); |
There was a problem hiding this comment.
The error checking logic for FlagEvaluationDetails is duplicated for each flag type. This can be refactored into a helper method to reduce code duplication and improve maintainability.
You can add the following helper method to the Main class:
private static <T> void checkDetails(FlagEvaluationDetails<T> details, String flagType) throws Exception {
if (details.getErrorCode() != null) {
throw new Exception("Error evaluating " + flagType + " flag: " + details.getFlagKey());
}
}Then, you can simplify the evaluation checks as shown in the suggestion.
Boolean enableFeatureA = EnableFeatureA.value(client, evalContext);
System.out.println("enableFeatureA: " + enableFeatureA);
checkDetails(EnableFeatureA.valueWithDetails(client, evalContext), "boolean");
Double discount = DiscountPercentage.value(client, evalContext);
System.out.printf("Discount Percentage: %.2f%n", discount);
checkDetails(DiscountPercentage.valueWithDetails(client, evalContext), "double");
String greetingMessage = GreetingMessage.value(client, evalContext);
System.out.println("greetingMessage: " + greetingMessage);
checkDetails(GreetingMessage.valueWithDetails(client, evalContext), "string");
Integer usernameMaxLength = UsernameMaxLength.value(client, evalContext);
System.out.println("usernameMaxLength: " + usernameMaxLength);
checkDetails(UsernameMaxLength.valueWithDetails(client, evalContext), "int");
Value themeCustomization = ThemeCustomization.value(client, evalContext);
checkDetails(ThemeCustomization.valueWithDetails(client, evalContext), "object");
System.out.println("themeCustomization: " + themeCustomization);2 participants
Summary
Fixes #115
Implements integration test coverage for the Java generator following the standardized Dagger-based testing pattern documented in test/README.md.
Changes
New Files
test/integration/cmd/java/run.go- Dagger test runner for Javatest/java-integration/pom.xml- Maven project configurationtest/java-integration/src/main/java/dev/openfeature/Main.java- Test program.value()and.valueWithDetails()methods.getKey()method for all flagsModified Files
test/integration/cmd/run.go- Added Java test to main integration runnerMakefile- Addedtest-integration-javatargetHow It Works
The test follows the same pattern as existing integration tests (C#, Go, Node.js, Angular):
cli generate javawith the sample manifestUses Dagger for reproducibility - runs identically locally and in CI.
Testing
Run the Java integration test:
Run all integration tests (including Java):
Example Output
Acceptance Criteria
Additional Notes
Signed-off-by: vikasrao23 vikasrao23@users.noreply.github.com