feat: Add dedicated annotations for boolean, String, int and double flags in Junit5 extension #1478
Conversation
| @Test | ||
| @BooleanFlag(name = FLAG, value = true) | ||
| @BooleanFlag(name = FLAG, value = false) | ||
| void duplicatedTypedFlagsSimple() { |
There was a problem hiding this comment.
Please use more descriptive names for your tests, maybe something like duplicateFlagKeyOverridesPreviousDeclaration
|
@ssharaev you will need to force push a commit that is signed off, otherwise the DCO check will not succeed |
…lags in Junit5 extension Signed-off-by: Sviatoslav Sharaev <[email protected]>
ssharaev
force-pushed
the
feat/new-test-flags-annotations
branch
from
d53dc31 to
6777d82
Compare
| } | ||
|
|
||
| @Test | ||
| @DoubleFlag(name = FLAG, value = 1.d) |
There was a problem hiding this comment.
| @DoubleFlag(name = FLAG, value = 1.d) | |
| @DoubleFlag(name = FLAG, value = FLAG_VALUE) |
also for the other tests
| if (domainFlagNames.contains(flagName)) { | ||
| throw new IllegalArgumentException("Flag with name " + flagName + " already exists. " | ||
| + "There shouldn't be @Flag and @" + value.getClass().getSimpleName() + "Flag with the same name!"); | ||
| } | ||
|
|
||
| if (domainFlags.containsKey(flagName)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
One of these checks is redundant. I think we could even remove the domainFlagNames set entirely, as it contains the same information as the domainFlags map.
| @BooleanFlag(name = FLAG, value = false) | ||
| void duplicatedTypedFlagDoesntOverridePrevious() { | ||
| Client client = OpenFeatureAPI.getInstance().getClient(); | ||
| assertThat(client.getBooleanValue(FLAG, false)).isTrue(); |
There was a problem hiding this comment.
Reading the annotations I would have expected the last definition of FLAG to win and resolve to false.
Is this the same behavior as with multiple @Flag annotations?
| @BooleanFlag(name = FLAG, value = true) | ||
| void existingSimpleTypedFlagIsRetrieved() { | ||
| Client client = OpenFeatureAPI.getInstance().getClient("testSpecific"); | ||
| assertThat(client.getBooleanValue(FLAG, false)).isTrue(); |
There was a problem hiding this comment.
What does this test do? FLAG is defined on Class and Method level, both with true.
| @Test | ||
| @Flag(name = FLAG, value = "true") | ||
| @BooleanFlag(name = FLAG, value = true) | ||
| void simpleConfigDuplicateFlags() { |
There was a problem hiding this comment.
Interesting, that defining FLAG twice as simple flag and type flag on the method level throws, an exception, but a simple flag FLAG on class level and a typed flag FLAG on method level does not throw.
See existingSimpleTypedFlagIsRetrieved() on L143
There was a problem hiding this comment.
Just saw, this test class is disabled. Is this tech debt?
4 participants
This PR
BooleanFlag,StringFlag,IntegerFlag, andDoubleFlagfor more specific and type-safe flag definitions in JUnit testsRelated Issues
Fixes #923
Notes
Not sure about tests, feedback on the test coverage is very welcome!