Skip to content

Commit 79ef358

Browse files
finnegancarrollfinnegancarroll
committed
Refactor FeatureFlagTests according to refactor. Test cases sync on test only
feature flag to avoid race conditions/polluting other test environments. Add cases for test utils. Add cases for system properties. Signed-off-by: Finn Carroll <carrofin@amazon.com>
1 parent 2aa5f19 commit 79ef358

File tree

1 file changed

+115
-29
lines changed

1 file changed

+115
-29
lines changed

‎server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java‎

Lines changed: 115 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -8,54 +8,140 @@
88

99
package org.opensearch.common.util;
1010

11+
import org.opensearch.common.SuppressForbidden;
12+
import org.opensearch.common.settings.Setting;
1113
import org.opensearch.common.settings.Settings;
12-
import org.opensearch.test.FeatureFlagSetter;
1314
import org.opensearch.test.OpenSearchTestCase;
15+
import org.junit.After;
16+
import org.junit.AfterClass;
17+
import org.junit.BeforeClass;
18+
import org.junit.Rule;
19+
import org.junit.rules.TestRule;
20+
import org.junit.runners.model.Statement;
1421

15-
import static org.opensearch.common.util.FeatureFlags.DATETIME_FORMATTER_CACHING;
16-
import static org.opensearch.common.util.FeatureFlags.EXTENSIONS;
22+
import java.io.IOException;
1723

24+
import static org.opensearch.common.util.FeatureFlags.FEATURE_FLAG_PREFIX;
25+
26+
/**
27+
* It is easy for the state of feature flags to persist across tests running on multiple threads as:
28+
* - Feature flags are globally available
29+
* - Feature flags may be loaded from JVM system properties
30+
* Here we test underlying implementation (FeatureFlags.FeatureFlagsImpl) to avoid polluting global scope.
31+
* Other tests should use the set()/reset() helpers provided in FeatureFlags.TestUtils.
32+
*/
1833
public class FeatureFlagTests extends OpenSearchTestCase {
34+
private static final String TEST_FEATURE = FEATURE_FLAG_PREFIX + "testfeat.enabled";
35+
private static final Setting<Boolean> TEST_FEATURE_FLAG = Setting.boolSetting(TEST_FEATURE, false, Setting.Property.NodeScope);
36+
private static final FeatureFlags.FeatureFlagsImpl featureFlagsImpl = new FeatureFlags.FeatureFlagsImpl();
37+
38+
// Make featureFlagsImpl static to mock global feature flags as used by other test cases.
39+
// Synchronize on featureFlagsImpl to avoid race conditions when tests run in parallel.
40+
@Rule
41+
public TestRule synchronizationRule = (base, description) -> new Statement() {
42+
@Override
43+
public void evaluate() throws Throwable {
44+
synchronized (featureFlagsImpl) {
45+
base.evaluate();
46+
}
47+
}
48+
};
49+
50+
@BeforeClass
51+
public static void setupClass() {
52+
FeatureFlags.TestUtils.addFlag(TEST_FEATURE_FLAG);
53+
}
54+
55+
@AfterClass
56+
public static void teardownClass() {
57+
FeatureFlags.TestUtils.removeFlag(TEST_FEATURE_FLAG);
58+
}
59+
60+
@SuppressForbidden(reason = "Unit test on isolated feature flag")
61+
@After
62+
public void teardown() throws IOException {
63+
synchronized (TEST_FEATURE) {
64+
System.clearProperty(TEST_FEATURE);
65+
featureFlagsImpl.reset();
66+
}
67+
}
1968

20-
private final String FLAG_PREFIX = "opensearch.experimental.feature.";
69+
public void testFeatureFlagsNotInitialized() {
70+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
71+
}
72+
73+
public void testFeatureFlagsFromDefault() {
74+
featureFlagsImpl.initializeFeatureFlags();
75+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
76+
}
2177

22-
public void testFeatureFlagSet() {
23-
final String testFlag = FLAG_PREFIX + "testFlag";
24-
FeatureFlagSetter.set(testFlag);
25-
assertNotNull(System.getProperty(testFlag));
26-
assertTrue(FeatureFlags.isEnabled(testFlag));
78+
public void testFeatureFlagFromEmpty() {
79+
featureFlagsImpl.initializeFeatureFlags(Settings.EMPTY);
80+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
2781
}
2882

29-
public void testMissingFeatureFlag() {
30-
final String testFlag = FLAG_PREFIX + "testFlag";
31-
assertNull(System.getProperty(testFlag));
32-
assertFalse(FeatureFlags.isEnabled(testFlag));
83+
public void testFeatureFlagFromSettings() {
84+
// init from settings
85+
featureFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FEATURE, true).build());
86+
assertTrue(featureFlagsImpl.isEnabled(TEST_FEATURE));
87+
// overwrite with new settings
88+
featureFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FEATURE, false).build());
89+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
3390
}
3491

3592
public void testNonBooleanFeatureFlag() {
3693
String javaVersionProperty = "java.version";
3794
assertNotNull(System.getProperty(javaVersionProperty));
38-
assertFalse(FeatureFlags.isEnabled(javaVersionProperty));
95+
assertFalse(featureFlagsImpl.isEnabled(javaVersionProperty));
96+
}
97+
98+
public void testFeatureFlagsEmptySettings() {
99+
featureFlagsImpl.initializeFeatureFlags(Settings.EMPTY);
100+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
101+
}
102+
103+
public void testFeatureFlagsTestUtilsSetSingleFlag() {
104+
featureFlagsImpl.set(TEST_FEATURE, false);
105+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
106+
}
107+
108+
@SuppressForbidden(reason = "Unit test on isolated feature flag")
109+
public void testFeatureFlagFromSystemProperty() {
110+
System.setProperty(TEST_FEATURE, "true");
111+
featureFlagsImpl.initializeFeatureFlags();
112+
assertTrue(featureFlagsImpl.isEnabled(TEST_FEATURE));
39113
}
40114

41-
public void testBooleanFeatureFlagWithDefaultSetToFalse() {
42-
final String testFlag = EXTENSIONS;
43-
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
44-
assertNotNull(testFlag);
45-
assertFalse(FeatureFlags.isEnabled(testFlag));
115+
@SuppressForbidden(reason = "Unit test on isolated feature flag")
116+
public void testFeatureFlagSettingOverwritesSystemProperties() {
117+
System.setProperty(TEST_FEATURE, "true");
118+
featureFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FEATURE, false).build());
119+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
120+
System.setProperty(TEST_FEATURE, "true");
121+
featureFlagsImpl.initializeFeatureFlags();
122+
assertTrue(featureFlagsImpl.isEnabled(TEST_FEATURE));
123+
featureFlagsImpl.initializeFeatureFlags(Settings.builder().put(TEST_FEATURE, false).build());
124+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
46125
}
47126

48-
public void testBooleanFeatureFlagInitializedWithEmptySettingsAndDefaultSetToFalse() {
49-
final String testFlag = DATETIME_FORMATTER_CACHING;
50-
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
51-
assertFalse(FeatureFlags.isEnabled(testFlag));
127+
@SuppressForbidden(reason = "Unit test on isolated feature flag")
128+
public void testFeatureDoesNotExist() {
129+
final String DNE_FF = FEATURE_FLAG_PREFIX + "doesntexist";
130+
assertFalse(featureFlagsImpl.isEnabled(DNE_FF));
131+
System.setProperty(DNE_FF, "true");
132+
featureFlagsImpl.initializeFeatureFlags();
133+
assertFalse(featureFlagsImpl.isEnabled(DNE_FF));
134+
featureFlagsImpl.initializeFeatureFlags(Settings.builder().put(DNE_FF, true).build());
135+
assertFalse(featureFlagsImpl.isEnabled(DNE_FF));
52136
}
53137

54-
public void testInitializeFeatureFlagsWithExperimentalSettings() {
55-
FeatureFlags.initializeFeatureFlags(Settings.builder().put(EXTENSIONS, true).build());
56-
assertTrue(FeatureFlags.isEnabled(EXTENSIONS));
57-
assertFalse(FeatureFlags.isEnabled(DATETIME_FORMATTER_CACHING));
58-
// reset FeatureFlags to defaults
59-
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
138+
@SuppressForbidden(reason = "Unit test on isolated feature flag")
139+
public void testFeatureFlagsSysPropTestUtilsReset() {
140+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
141+
System.setProperty(TEST_FEATURE, "true");
142+
featureFlagsImpl.initializeFeatureFlags();
143+
assertTrue(featureFlagsImpl.isEnabled(TEST_FEATURE));
144+
featureFlagsImpl.reset();
145+
assertFalse(featureFlagsImpl.isEnabled(TEST_FEATURE));
60146
}
61147
}

0 commit comments

Comments
 (0)