Skip to content

Commit 7515dfd

Browse files
DaveCTurnerDaveCTurner
authored
Move systemPropertiesOverrideTest to separate source set (#137492)
Today `:server:systemPropertiesOverrideTest` uses the same source set as `:server:test`, but runs a disjoint set of tests via includes & excludes. This forces every developer to choose in their IDE between the two tasks when running any unit test in `:server` even though there's only one appropriate task to run for any particular test. This commit moves the serverless-specific unit tests into their own source set to remove this friction in the development flow.
1 parent c9c50ea commit 7515dfd

File tree

6 files changed

+203
-117
lines changed

6 files changed

+203
-117
lines changed

‎qa/serverless-system-properties/build.gradle‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
apply plugin: 'elasticsearch.build'
11+
12+
dependencies {
13+
testImplementation project(':test:framework')
14+
testImplementation project(':server')
15+
}
16+
17+
tasks.named("test").configure {
18+
systemProperty 'es.stateless.allow.index.refresh_interval.override', 'true'
19+
systemProperty 'es.serverless_transport', 'true'
20+
}

‎server/src/test/java/org/elasticsearch/index/IndexSettingsOverrideTests.java‎ renamed to ‎qa/serverless-system-properties/src/test/java/org/elasticsearch/index/IndexSettingsOverrideTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ public static IndexMetadata newIndexMeta(String name, Settings indexSettings) {
2727
}
2828

2929
public void testStatelessMinRefreshIntervalOverride() {
30-
assumeTrue(
30+
assertTrue(
3131
"This test depends on system property configured in build.gradle",
3232
Booleans.parseBoolean(
3333
System.getProperty(IndexSettings.RefreshIntervalValidator.STATELESS_ALLOW_INDEX_REFRESH_INTERVAL_OVERRIDE, "false")

‎qa/serverless-system-properties/src/test/java/org/elasticsearch/transport/ServerlessTransportHandshakeTests.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.transport;
11+
12+
import org.apache.logging.log4j.Level;
13+
import org.elasticsearch.Build;
14+
import org.elasticsearch.TransportVersion;
15+
import org.elasticsearch.cluster.node.DiscoveryNodeUtils;
16+
import org.elasticsearch.cluster.node.VersionInformation;
17+
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
18+
import org.elasticsearch.common.network.NetworkService;
19+
import org.elasticsearch.common.settings.Settings;
20+
import org.elasticsearch.common.util.PageCacheRecycler;
21+
import org.elasticsearch.indices.breaker.NoneCircuitBreakerService;
22+
import org.elasticsearch.test.ESTestCase;
23+
import org.elasticsearch.test.MockLog;
24+
import org.elasticsearch.test.transport.MockTransportService;
25+
import org.elasticsearch.threadpool.TestThreadPool;
26+
import org.elasticsearch.threadpool.ThreadPool;
27+
import org.elasticsearch.transport.netty4.Netty4Transport;
28+
import org.elasticsearch.transport.netty4.SharedGroupFactory;
29+
import org.junit.After;
30+
import org.junit.AfterClass;
31+
import org.junit.BeforeClass;
32+
33+
import java.util.ArrayList;
34+
import java.util.Collections;
35+
import java.util.List;
36+
import java.util.concurrent.TimeUnit;
37+
38+
import static java.util.Collections.emptySet;
39+
import static org.elasticsearch.transport.AbstractSimpleTransportTestCase.IGNORE_DESERIALIZATION_ERRORS_SETTING;
40+
41+
public class ServerlessTransportHandshakeTests extends ESTestCase {
42+
43+
private static ThreadPool threadPool;
44+
45+
@BeforeClass
46+
public static void startThreadPool() {
47+
threadPool = new TestThreadPool(ServerlessTransportHandshakeTests.class.getSimpleName());
48+
}
49+
50+
private final List<TransportService> transportServices = new ArrayList<>();
51+
52+
private TransportService startServices(String nodeNameAndId, Settings settings, TransportInterceptor transportInterceptor) {
53+
TcpTransport transport = new Netty4Transport(
54+
settings,
55+
TransportVersion.current(),
56+
threadPool,
57+
new NetworkService(Collections.emptyList()),
58+
PageCacheRecycler.NON_RECYCLING_INSTANCE,
59+
new NamedWriteableRegistry(Collections.emptyList()),
60+
new NoneCircuitBreakerService(),
61+
new SharedGroupFactory(settings)
62+
);
63+
TransportService transportService = new MockTransportService(
64+
settings,
65+
transport,
66+
threadPool,
67+
transportInterceptor,
68+
(boundAddress) -> DiscoveryNodeUtils.builder(nodeNameAndId)
69+
.name(nodeNameAndId)
70+
.address(boundAddress.publishAddress())
71+
.roles(emptySet())
72+
.version(VersionInformation.CURRENT)
73+
.build(),
74+
null,
75+
Collections.emptySet(),
76+
nodeNameAndId
77+
);
78+
transportService.start();
79+
transportService.acceptIncomingRequests();
80+
transportServices.add(transportService);
81+
return transportService;
82+
}
83+
84+
@After
85+
public void tearDown() throws Exception {
86+
for (TransportService transportService : transportServices) {
87+
transportService.close();
88+
}
89+
super.tearDown();
90+
}
91+
92+
@AfterClass
93+
public static void terminateThreadPool() {
94+
ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS);
95+
// since static must set to null to be eligible for collection
96+
threadPool = null;
97+
}
98+
99+
public void testAcceptsMismatchedServerlessBuildHashWithoutWarning() {
100+
assumeTrue("Current build needs to be a snapshot", Build.current().isSnapshot());
101+
final var transportInterceptorA = new BuildHashModifyingTransportInterceptor();
102+
final var transportInterceptorB = new BuildHashModifyingTransportInterceptor();
103+
final Settings settings = Settings.builder()
104+
.put("cluster.name", "a")
105+
.put(IGNORE_DESERIALIZATION_ERRORS_SETTING.getKey(), true) // suppress assertions to test production error-handling
106+
.build();
107+
final TransportService transportServiceA = startServices("TS_A", settings, transportInterceptorA);
108+
final TransportService transportServiceB = startServices("TS_B", settings, transportInterceptorB);
109+
MockLog.assertThatLogger(() -> {
110+
AbstractSimpleTransportTestCase.connectToNode(transportServiceA, transportServiceB.getLocalNode(), TestProfiles.LIGHT_PROFILE);
111+
assertTrue(transportServiceA.nodeConnected(transportServiceB.getLocalNode()));
112+
},
113+
TransportService.class,
114+
new MockLog.UnseenEventExpectation("incompatible wire format log", TransportService.class.getCanonicalName(), Level.WARN, "*")
115+
);
116+
117+
}
118+
119+
}

‎server/build.gradle‎

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -151,30 +151,6 @@ if (buildParams.snapshotBuild == false) {
151151

152152
tasks.named("test").configure {
153153
systemProperty 'es.insecure_network_trace_enabled', 'true'
154-
filter {
155-
excludeTestsMatching("*.TransportServiceHandshakeTests.testAcceptsMismatchedServerlessBuildHash")
156-
}
157-
excludes << '**/IndexSettingsOverrideTests.class'
158-
}
159-
160-
// There are tests rely on system properties to be configured differently. They must run in a separate test job
161-
// since the default does not work for them and configuring the system properties inside the test class/method
162-
// is too late because fields based on the system properties are often initialized statically.
163-
TaskProvider<Test> systemPropertiesOverrideTest = tasks.register("systemPropertiesOverrideTest", Test) {
164-
include '**/IndexSettingsOverrideTests.class'
165-
include '**/TransportServiceHandshakeTests.class'
166-
filter {
167-
includeTestsMatching("*.TransportServiceHandshakeTests.testAcceptsMismatchedServerlessBuildHash")
168-
includeTestsMatching("*.IndexSettingsOverrideTests.*")
169-
}
170-
systemProperty 'es.stateless.allow.index.refresh_interval.override', 'true'
171-
systemProperty 'es.serverless_transport', 'true'
172-
classpath = sourceSets.test.runtimeClasspath
173-
testClassesDirs = sourceSets.test.output.classesDirs
174-
}
175-
176-
tasks.named("check").configure {
177-
dependsOn(systemPropertiesOverrideTest)
178154
}
179155

180156
tasks.named("thirdPartyAudit").configure {

‎server/src/test/java/org/elasticsearch/transport/TransportServiceHandshakeTests.java‎

Lines changed: 4 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
package org.elasticsearch.transport;
1111

1212
import org.apache.logging.log4j.Level;
13-
import org.elasticsearch.Build;
1413
import org.elasticsearch.TransportVersion;
1514
import org.elasticsearch.Version;
1615
import org.elasticsearch.cluster.node.DiscoveryNode;
@@ -41,7 +40,6 @@
4140
import java.util.ArrayList;
4241
import java.util.Collections;
4342
import java.util.List;
44-
import java.util.concurrent.Executor;
4543
import java.util.concurrent.TimeUnit;
4644

4745
import static java.util.Collections.emptySet;
@@ -323,10 +321,8 @@ public void testNodeConnectWithDifferentNodeId() {
323321
}
324322

325323
public void testRejectsMismatchedBuildHash() {
326-
final DisruptingTransportInterceptor transportInterceptorA = new DisruptingTransportInterceptor();
327-
final DisruptingTransportInterceptor transportInterceptorB = new DisruptingTransportInterceptor();
328-
transportInterceptorA.setModifyBuildHash(true);
329-
transportInterceptorB.setModifyBuildHash(true);
324+
final var transportInterceptorA = new BuildHashModifyingTransportInterceptor();
325+
final var transportInterceptorB = new BuildHashModifyingTransportInterceptor();
330326
final Settings settings = Settings.builder()
331327
.put("cluster.name", "a")
332328
.put(IGNORE_DESERIALIZATION_ERRORS_SETTING.getKey(), true) // suppress assertions to test production error-handling
@@ -375,39 +371,9 @@ public void testRejectsMismatchedBuildHash() {
375371
assertFalse(transportServiceA.nodeConnected(discoveryNode));
376372
}
377373

378-
public void testAcceptsMismatchedServerlessBuildHash() {
379-
assumeTrue("Current build needs to be a snapshot", Build.current().isSnapshot());
380-
final DisruptingTransportInterceptor transportInterceptorA = new DisruptingTransportInterceptor();
381-
final DisruptingTransportInterceptor transportInterceptorB = new DisruptingTransportInterceptor();
382-
transportInterceptorA.setModifyBuildHash(true);
383-
transportInterceptorB.setModifyBuildHash(true);
384-
final Settings settings = Settings.builder()
385-
.put("cluster.name", "a")
386-
.put(IGNORE_DESERIALIZATION_ERRORS_SETTING.getKey(), true) // suppress assertions to test production error-handling
387-
.build();
388-
final TransportService transportServiceA = startServices(
389-
"TS_A",
390-
settings,
391-
TransportVersion.current(),
392-
VersionInformation.CURRENT,
393-
transportInterceptorA
394-
);
395-
final TransportService transportServiceB = startServices(
396-
"TS_B",
397-
settings,
398-
TransportVersion.current(),
399-
VersionInformation.CURRENT,
400-
transportInterceptorB
401-
);
402-
AbstractSimpleTransportTestCase.connectToNode(transportServiceA, transportServiceB.getLocalNode(), TestProfiles.LIGHT_PROFILE);
403-
assertTrue(transportServiceA.nodeConnected(transportServiceB.getLocalNode()));
404-
}
405-
406374
public void testAcceptsMismatchedBuildHashFromDifferentVersion() {
407-
final DisruptingTransportInterceptor transportInterceptorA = new DisruptingTransportInterceptor();
408-
final DisruptingTransportInterceptor transportInterceptorB = new DisruptingTransportInterceptor();
409-
transportInterceptorA.setModifyBuildHash(true);
410-
transportInterceptorB.setModifyBuildHash(true);
375+
final var transportInterceptorA = new BuildHashModifyingTransportInterceptor();
376+
final var transportInterceptorB = new BuildHashModifyingTransportInterceptor();
411377
final TransportService transportServiceA = startServices(
412378
"TS_A",
413379
Settings.builder().put("cluster.name", "a").build(),
@@ -425,58 +391,4 @@ public void testAcceptsMismatchedBuildHashFromDifferentVersion() {
425391
AbstractSimpleTransportTestCase.connectToNode(transportServiceA, transportServiceB.getLocalNode(), TestProfiles.LIGHT_PROFILE);
426392
assertTrue(transportServiceA.nodeConnected(transportServiceB.getLocalNode()));
427393
}
428-
429-
private static class DisruptingTransportInterceptor implements TransportInterceptor {
430-
431-
private boolean modifyBuildHash;
432-
433-
public void setModifyBuildHash(boolean modifyBuildHash) {
434-
this.modifyBuildHash = modifyBuildHash;
435-
}
436-
437-
@Override
438-
public <T extends TransportRequest> TransportRequestHandler<T> interceptHandler(
439-
String action,
440-
Executor executor,
441-
boolean forceExecution,
442-
TransportRequestHandler<T> actualHandler
443-
) {
444-
445-
if (TransportService.HANDSHAKE_ACTION_NAME.equals(action)) {
446-
return (request, channel, task) -> actualHandler.messageReceived(request, new TransportChannel() {
447-
@Override
448-
public String getProfileName() {
449-
return channel.getProfileName();
450-
}
451-
452-
@Override
453-
public void sendResponse(TransportResponse response) {
454-
assertThat(response, instanceOf(TransportService.HandshakeResponse.class));
455-
if (modifyBuildHash) {
456-
final TransportService.HandshakeResponse handshakeResponse = (TransportService.HandshakeResponse) response;
457-
channel.sendResponse(
458-
new TransportService.HandshakeResponse(
459-
handshakeResponse.getVersion(),
460-
handshakeResponse.getBuildHash() + "-modified",
461-
handshakeResponse.getDiscoveryNode(),
462-
handshakeResponse.getClusterName()
463-
)
464-
);
465-
} else {
466-
channel.sendResponse(response);
467-
}
468-
}
469-
470-
@Override
471-
public void sendResponse(Exception exception) {
472-
channel.sendResponse(exception);
473-
474-
}
475-
}, task);
476-
} else {
477-
return actualHandler;
478-
}
479-
}
480-
}
481-
482394
}

‎test/framework/src/main/java/org/elasticsearch/transport/BuildHashModifyingTransportInterceptor.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
package org.elasticsearch.transport;
11+
12+
import org.elasticsearch.test.ESTestCase;
13+
14+
import java.util.concurrent.Executor;
15+
16+
import static org.hamcrest.Matchers.instanceOf;
17+
18+
class BuildHashModifyingTransportInterceptor implements TransportInterceptor {
19+
20+
@Override
21+
public <T extends TransportRequest> TransportRequestHandler<T> interceptHandler(
22+
String action,
23+
Executor executor,
24+
boolean forceExecution,
25+
TransportRequestHandler<T> actualHandler
26+
) {
27+
28+
if (TransportService.HANDSHAKE_ACTION_NAME.equals(action)) {
29+
return (request, channel, task) -> actualHandler.messageReceived(request, new TransportChannel() {
30+
@Override
31+
public String getProfileName() {
32+
return channel.getProfileName();
33+
}
34+
35+
@Override
36+
public void sendResponse(TransportResponse response) {
37+
ESTestCase.assertThat(response, instanceOf(TransportService.HandshakeResponse.class));
38+
final TransportService.HandshakeResponse handshakeResponse = (TransportService.HandshakeResponse) response;
39+
channel.sendResponse(
40+
new TransportService.HandshakeResponse(
41+
handshakeResponse.getVersion(),
42+
handshakeResponse.getBuildHash() + "-modified",
43+
handshakeResponse.getDiscoveryNode(),
44+
handshakeResponse.getClusterName()
45+
)
46+
);
47+
}
48+
49+
@Override
50+
public void sendResponse(Exception exception) {
51+
channel.sendResponse(exception);
52+
53+
}
54+
}, task);
55+
} else {
56+
return actualHandler;
57+
}
58+
}
59+
}

0 commit comments

Comments
 (0)