Skip to content

Commit 76911b8

Browse files
author
Christoph Büscher
committed
Fix test reproducability in AbstractBuilderTestCase setup (#32403)
Currently AbstractBuilderTestCase generates certain random values in its `beforeTest()` method annotated with @before only the first time that a test method in the suite is run while initializing the serviceHolder that we use for the rest of the test. This changes the values of subsequent random values and has the effect that when running single methods from a test suite with "-Dtests.method=*", the random values it sees are different from when the same test method is run as part of the whole test suite. This makes it hard to use the reproduction lines logged on failure. This change runs the inialization of the serviceHolder and the randomization connected to it using the test runners master seed, so reproduction by running just one method is possible again. Closes #32400
1 parent 7ac0f25 commit 76911b8

File tree

7 files changed

+41
-21
lines changed

7 files changed

+41
-21
lines changed

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
8888
}
8989

9090
@Override
91-
protected Settings indexSettings() {
91+
protected Settings createTestIndexSettings() {
9292
return Settings.builder()
93-
.put(super.indexSettings())
93+
.put(super.createTestIndexSettings())
9494
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
9595
.build();
9696
}

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
7373
}
7474

7575
@Override
76-
protected Settings indexSettings() {
76+
protected Settings createTestIndexSettings() {
7777
return Settings.builder()
78-
.put(super.indexSettings())
78+
.put(super.createTestIndexSettings())
7979
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
8080
.build();
8181
}

modules/parent-join/src/test/java/org/elasticsearch/join/query/LegacyHasChildQueryBuilderTests.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.elasticsearch.join.query;
2121

2222
import com.carrotsearch.randomizedtesting.generators.RandomPicks;
23+
2324
import org.apache.lucene.search.BooleanClause;
2425
import org.apache.lucene.search.BooleanQuery;
2526
import org.apache.lucene.search.ConstantScoreQuery;
@@ -111,9 +112,9 @@ protected void initializeAdditionalMappings(MapperService mapperService) throws
111112
}
112113

113114
@Override
114-
protected Settings indexSettings() {
115+
protected Settings createTestIndexSettings() {
115116
return Settings.builder()
116-
.put(super.indexSettings())
117+
.put(super.createTestIndexSettings())
117118
.put("index.version.created", Version.V_5_6_0) // multi type
118119
.build();
119120
}

modules/parent-join/src/test/java/org/elasticsearch/join/query/LegacyHasParentQueryBuilderTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
7070
}
7171

7272
@Override
73-
protected Settings indexSettings() {
73+
protected Settings createTestIndexSettings() {
7474
return Settings.builder()
75-
.put(super.indexSettings())
75+
.put(super.createTestIndexSettings())
7676
.put("index.version.created", Version.V_5_6_0) // legacy needs multi types
7777
.build();
7878
}

modules/parent-join/src/test/java/org/elasticsearch/join/query/ParentIdQueryBuilderTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
6262
}
6363

6464
@Override
65-
protected Settings indexSettings() {
65+
protected Settings createTestIndexSettings() {
6666
return Settings.builder()
67-
.put(super.indexSettings())
67+
.put(super.createTestIndexSettings())
6868
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
6969
.build();
7070
}

server/src/test/java/org/elasticsearch/index/query/SimpleQueryStringBuilderTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ public void testFieldsCannotBeSetToNull() {
234234

235235
public void testDefaultFieldParsing() throws IOException {
236236
assumeTrue("5.x behaves differently, so skip on non-6.x indices",
237-
indexVersionCreated.onOrAfter(Version.V_6_0_0_alpha1));
237+
indexSettings().getIndexVersionCreated().onOrAfter(Version.V_6_0_0_alpha1));
238238

239239
String query = randomAlphaOfLengthBetween(1, 10).toLowerCase(Locale.ROOT);
240240
String contentString = "{\n" +

test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919

2020
package org.elasticsearch.test;
2121

22+
import com.carrotsearch.randomizedtesting.RandomizedTest;
23+
import com.carrotsearch.randomizedtesting.SeedUtils;
24+
2225
import org.apache.lucene.index.IndexReader;
2326
import org.apache.lucene.util.Accountable;
2427
import org.elasticsearch.Version;
@@ -86,6 +89,7 @@
8689
import java.util.HashMap;
8790
import java.util.List;
8891
import java.util.Map;
92+
import java.util.concurrent.Callable;
8993
import java.util.concurrent.ExecutionException;
9094
import java.util.function.Function;
9195
import java.util.stream.Stream;
@@ -126,14 +130,13 @@ public abstract class AbstractBuilderTestCase extends ESTestCase {
126130
ALIAS_TO_CONCRETE_FIELD_NAME.put(GEO_POINT_ALIAS_FIELD_NAME, GEO_POINT_FIELD_NAME);
127131
}
128132

129-
protected static Version indexVersionCreated;
130-
131133
private static ServiceHolder serviceHolder;
132134
private static int queryNameId = 0;
133135
private static Settings nodeSettings;
134136
private static Index index;
135137
private static String[] currentTypes;
136138
protected static String[] randomTypes;
139+
private static long nowInMillis;
137140

138141
protected static Index getIndex() {
139142
return index;
@@ -162,6 +165,7 @@ public static void beforeClass() {
162165
.build();
163166

164167
index = new Index(randomAlphaOfLengthBetween(1, 10), "_na_");
168+
nowInMillis = randomNonNegativeLong();
165169

166170
// Set a single type in the index
167171
switch (random().nextInt(3)) {
@@ -211,15 +215,19 @@ protected static String createUniqueRandomName() {
211215
return queryName;
212216
}
213217

214-
protected Settings indexSettings() {
218+
protected Settings createTestIndexSettings() {
215219
// we have to prefer CURRENT since with the range of versions we support it's rather unlikely to get the current actually.
216-
indexVersionCreated = randomBoolean() ? Version.CURRENT
217-
: VersionUtils.randomVersionBetween(random(), null, Version.CURRENT);
220+
Version indexVersionCreated = randomBoolean() ? Version.CURRENT
221+
: VersionUtils.randomVersionBetween(random(), Version.V_6_0_0, Version.CURRENT);
218222
return Settings.builder()
219223
.put(IndexMetaData.SETTING_VERSION_CREATED, indexVersionCreated)
220224
.build();
221225
}
222226

227+
protected static IndexSettings indexSettings() {
228+
return serviceHolder.idxSettings;
229+
}
230+
223231
protected static String expectedFieldName(String builderFieldName) {
224232
if (currentTypes.length == 0 || !isSingleType()) {
225233
return builderFieldName;
@@ -234,10 +242,20 @@ public static void afterClass() throws Exception {
234242
}
235243

236244
@Before
237-
public void beforeTest() throws IOException {
245+
public void beforeTest() throws Exception {
238246
if (serviceHolder == null) {
239-
serviceHolder = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), this);
247+
// we initialize the serviceHolder and serviceHolderWithNoType just once, but need some
248+
// calls to the randomness source during its setup. In order to not mix these calls with
249+
// the randomness source that is later used in the test method, we use the master seed during
250+
// this setup
251+
long masterSeed = SeedUtils.parseSeed(RandomizedTest.getContext().getRunnerSeedAsString());
252+
RandomizedTest.getContext().runWithPrivateRandomness(masterSeed, (Callable<Void>) () -> {
253+
serviceHolder = new ServiceHolder(nodeSettings, createTestIndexSettings(), getPlugins(), nowInMillis,
254+
AbstractBuilderTestCase.this);
255+
return null;
256+
});
240257
}
258+
241259
serviceHolder.clientInvocationHandler.delegate = this;
242260
}
243261

@@ -333,11 +351,12 @@ private static class ServiceHolder implements Closeable {
333351
private final BitsetFilterCache bitsetFilterCache;
334352
private final ScriptService scriptService;
335353
private final Client client;
336-
private final long nowInMillis = randomNonNegativeLong();
354+
private final long nowInMillis;
337355

338-
ServiceHolder(Settings nodeSettings, Settings indexSettings,
339-
Collection<Class<? extends Plugin>> plugins, AbstractBuilderTestCase testCase) throws IOException {
356+
ServiceHolder(Settings nodeSettings, Settings indexSettings, Collection<Class<? extends Plugin>> plugins, long nowInMillis,
357+
AbstractBuilderTestCase testCase) throws IOException {
340358
Environment env = InternalSettingsPreparer.prepareEnvironment(nodeSettings, null);
359+
this.nowInMillis = nowInMillis;
341360
PluginsService pluginsService;
342361
pluginsService = new PluginsService(nodeSettings, null, env.modulesFile(), env.pluginsFile(), plugins);
343362

0 commit comments

Comments
 (0)