test: Consolidate test providers #1661
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1661 +/- ##
============================================
+ Coverage 91.69% 93.20% +1.50%
- Complexity 517 520 +3
============================================
Files 51 51
Lines 1264 1265 +1
Branches 112 112
============================================
+ Hits 1159 1179 +20
+ Misses 68 50 -18
+ Partials 37 36 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chrfwow
force-pushed
the
consolidate-test-providers
branch
from
3001b95 to
15363e0
Compare
Signed-off-by: christian.lutnik <[email protected]>
Signed-off-by: christian.lutnik <[email protected]>
… consolidate-test-providers
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates multiple test providers into a single TestProvider class with a comprehensive builder pattern for different testing scenarios. The change simplifies test infrastructure by replacing several specialized provider classes with one configurable provider.
Key Changes
- Introduces a new unified
TestProviderwith a fluent builder pattern that supports various test configurations - Removes several specialized test provider classes (
TestEventsProvider,DoSomethingProvider,AlwaysBrokenWithExceptionProvider, etc.) - Updates the
Flagclass to include immutable fields and flag metadata support
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/test/java/dev/openfeature/sdk/testutils/testProvider/TestProvider.java |
New consolidated test provider with builder pattern supporting various test scenarios |
src/test/java/dev/openfeature/sdk/testutils/testProvider/FlagEvaluation.java |
Helper class to track flag evaluations for testing |
src/test/java/dev/openfeature/sdk/e2e/Flag.java |
Enhanced Flag class with immutable fields and metadata support |
| Multiple test files | Updated to use the new TestProvider instead of removed provider classes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for (int i = 0; i < hooks.length; i++) { | ||
| this.hooks.add(hooks[i]); |
There was a problem hiding this comment.
Use enhanced for-loop instead of traditional for-loop for better readability and performance. Replace with for (Hook hook : hooks) { this.hooks.add(hook); }
| for (int i = 0; i < hooks.length; i++) { | |
| this.hooks.add(hooks[i]); | |
| for (Hook hook : hooks) { | |
| this.hooks.add(hook); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
better performance? 🤔
src/test/java/dev/openfeature/sdk/testutils/testProvider/TestProvider.java
Show resolved
Hide resolved
src/test/java/dev/openfeature/sdk/testutils/testProvider/TestProvider.java
Show resolved
Hide resolved
![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 significant improvement to the test suite by consolidating various test providers into a single, highly configurable TestProvider using a builder pattern. This refactoring greatly enhances the readability, maintainability, and flexibility of the tests. The changes are well-implemented across the codebase. My review includes a suggestion to align the behavior of the new TestProvider with the OpenFeature specification regarding the resolution reason when returning a default value.
| return builder.errorMessage(errorMessage).errorCode(errorCode).build(); | ||
| } | ||
| if (allowAnyFlag) { | ||
| return builder.reason(Reason.STATIC.name()) |
There was a problem hiding this comment.
When allowAnyFlag is true, the provider returns the defaultValue from the evaluation call. According to the OpenFeature specification, the reason for this should be DEFAULT, as the provider is not resolving a value from its own configuration but is falling back to the default. The STATIC reason is for values that are hard-coded in the provider.
| return builder.reason(Reason.STATIC.name()) | |
| return builder.reason(Reason.DEFAULT.name()) |
There was a problem hiding this comment.
I'm not sure about this... What reason is better here?
| assertThat(evaluationDetails).isNotNull(); | ||
| assertThat(evaluationDetails.getErrorCode()).isNull(); | ||
| assertThat(evaluationDetails.getReason()).isEqualTo("DEFAULT"); | ||
| assertThat(evaluationDetails.getReason()).isEqualTo(Reason.STATIC.name()); |
There was a problem hiding this comment.
This assertion was changed to STATIC to match the new TestProvider's behavior. With the suggested change in TestProvider to return DEFAULT reason when allowUnknownFlags is true, this test should assert for DEFAULT reason.
| assertThat(evaluationDetails.getReason()).isEqualTo(Reason.STATIC.name()); | |
| assertThat(evaluationDetails.getReason()).isEqualTo(Reason.DEFAULT.name()); |
|
Signed-off-by: christian.lutnik <[email protected]>
1 participant
Consolidates all of our test providers into one, which covers all their functionality. Different behaviors can be selected through a builder pattern, that guides the user through the different options