Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test reproducability in AbstractBuilderTestCase setup #32403

Merged
merged 7 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
}

@Override
protected Settings indexSettings() {
protected Settings createTestIndexSettings() {
return Settings.builder()
.put(super.indexSettings())
.put(super.createTestIndexSettings())
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
}

@Override
protected Settings indexSettings() {
protected Settings createTestIndexSettings() {
return Settings.builder()
.put(super.indexSettings())
.put(super.createTestIndexSettings())
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
}

@Override
protected Settings indexSettings() {
protected Settings createTestIndexSettings() {
return Settings.builder()
.put(super.indexSettings())
.put(super.createTestIndexSettings())
.put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ public void testFieldsCannotBeSetToNull() {

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

String query = randomAlphaOfLengthBetween(1, 10).toLowerCase(Locale.ROOT);
String contentString = "{\n" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,12 @@ public abstract class AbstractBuilderTestCase extends ESTestCase {
ALIAS_TO_CONCRETE_FIELD_NAME.put(GEO_POINT_ALIAS_FIELD_NAME, GEO_POINT_FIELD_NAME);
}

protected static Version indexVersionCreated;

private static ServiceHolder serviceHolder;
private static ServiceHolder serviceHolderWithNoType;
private static int queryNameId = 0;
private static Settings nodeSettings;
private static Index index;
private static Index indexWithNoType;
private static long nowInMillis;

protected static Index getIndex() {
return index;
Expand All @@ -152,7 +150,7 @@ public static void beforeClass() {
.build();

index = new Index(randomAlphaOfLengthBetween(1, 10), randomAlphaOfLength(10));
indexWithNoType = new Index(randomAlphaOfLengthBetween(1, 10), randomAlphaOfLength(10));
nowInMillis = randomNonNegativeLong();
}

@Override
Expand All @@ -173,15 +171,19 @@ protected static String createUniqueRandomName() {
return queryName;
}

protected Settings indexSettings() {
protected Settings createTestIndexSettings() {
// we have to prefer CURRENT since with the range of versions we support it's rather unlikely to get the current actually.
indexVersionCreated = randomBoolean() ? Version.CURRENT
Version indexVersionCreated = randomBoolean() ? Version.CURRENT
: VersionUtils.randomVersionBetween(random(), Version.V_6_0_0, Version.CURRENT);
return Settings.builder()
.put(IndexMetaData.SETTING_VERSION_CREATED, indexVersionCreated)
.build();
}

protected IndexSettings indexSettings() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be static?

return serviceHolder.idxSettings;
}

protected static String expectedFieldName(String builderFieldName) {
return ALIAS_TO_CONCRETE_FIELD_NAME.getOrDefault(builderFieldName, builderFieldName);
}
Expand All @@ -196,12 +198,17 @@ public static void afterClass() throws Exception {

@Before
public void beforeTest() throws IOException {
// the two following ranomized parameters are drawn every run, even though we use them in
// creating ServiceHolder just once. The other times are necessary so that further random
// generation stays the same for all test that is run, otherwise reproducability breaks
Settings indexSettings = createTestIndexSettings();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed fix is ok, but I wonder if the following wouldn't be better:

    @Before
    public void beforeTest() throws Exception {
        if (serviceHolder == null) {
            assert serviceHolderWithNoType == null;
            long masterSeed = SeedUtils.parseSeed(RandomizedTest.getContext().getRunnerSeedAsString());
            RandomizedTest.getContext().runWithPrivateRandomness(masterSeed, (Callable<Void>) () -> {
                serviceHolder = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), AbstractBuilderTestCase.this, true);
                serviceHolderWithNoType = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), AbstractBuilderTestCase.this, false);
                return null;
            });
        }
        serviceHolder.clientInvocationHandler.delegate = this;
        serviceHolderWithNoType.clientInvocationHandler.delegate = this;
    }

The idea is to execute this conditional initialization step under the suite randomness rather than the method randomness. If we don't want to parse the suite seed back from a string we could also generate our own suite seed, the important bit is that we generate it in the beforeClass which is executed under the suite randomness (master seed). I find that this way what we are doing is clearer. The proposed fix is good but can easily be reverted by mistake by just moving lines around.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion, I added this.


if (serviceHolder == null) {
serviceHolder = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), this, true);
serviceHolder = new ServiceHolder(nodeSettings, indexSettings, getPlugins(), nowInMillis, this, true);
}
serviceHolder.clientInvocationHandler.delegate = this;
if (serviceHolderWithNoType == null) {
serviceHolderWithNoType = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), this, false);
serviceHolderWithNoType = new ServiceHolder(nodeSettings, indexSettings, getPlugins(), nowInMillis, this, false);
}
serviceHolderWithNoType.clientInvocationHandler.delegate = this;
}
Expand Down Expand Up @@ -305,13 +312,15 @@ private static class ServiceHolder implements Closeable {
private final BitsetFilterCache bitsetFilterCache;
private final ScriptService scriptService;
private final Client client;
private final long nowInMillis = randomNonNegativeLong();
private final long nowInMillis;

ServiceHolder(Settings nodeSettings,
Settings indexSettings,
Collection<Class<? extends Plugin>> plugins,
long nowInMillis,
AbstractBuilderTestCase testCase,
boolean registerType) throws IOException {
this.nowInMillis = nowInMillis;
Environment env = InternalSettingsPreparer.prepareEnvironment(nodeSettings);
PluginsService pluginsService;
pluginsService = new PluginsService(nodeSettings, null, env.modulesFile(), env.pluginsFile(), plugins);
Expand Down