From 0158b59a5a6257b69d61a4f1dd1732cd79fbe8d7 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 14 Aug 2018 11:35:09 -0700 Subject: [PATCH] Test: Fix forbidden uses in test framework (#32824) This commit fixes existing uses of forbidden apis in the test framework and re-enables the forbidden apis check. It was previously completely disabled and had missed a rename of the forbidden apis signatures files. closes #32772 --- test/build.gradle | 9 --------- test/fixtures/build.gradle | 5 +++++ test/framework/build.gradle | 5 +++-- .../indices/analysis/AnalysisFactoryTestCase.java | 2 +- .../test/AbstractBuilderTestCase.java | 6 +++--- .../java/org/elasticsearch/test/ESTestCase.java | 15 ++++++++++----- .../test/fixture/AbstractHttpFixture.java | 6 ++++-- .../junit/listeners/ReproduceInfoPrinter.java | 8 +++++++- .../AbstractSimpleTransportTestCase.java | 12 +++++++++--- .../elasticsearch/transport/MockTcpTransport.java | 2 ++ 10 files changed, 44 insertions(+), 26 deletions(-) diff --git a/test/build.gradle b/test/build.gradle index 2055896bda4..d0a3065e7c8 100644 --- a/test/build.gradle +++ b/test/build.gradle @@ -28,19 +28,10 @@ subprojects { apply plugin: 'nebula.maven-base-publish' apply plugin: 'nebula.maven-scm' - - // the main files are actually test files, so use the appropriate forbidden api sigs - forbiddenApisMain { - signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt'), - PrecommitTasks.getResource('/forbidden/es-signatures.txt'), - PrecommitTasks.getResource('/forbidden/es-test-signatures.txt')] - } - // TODO: should we have licenses for our test deps? dependencyLicenses.enabled = false dependenciesInfo.enabled = false // TODO: why is the test framework pulled in... - forbiddenApisMain.enabled = false jarHell.enabled = false } diff --git a/test/fixtures/build.gradle b/test/fixtures/build.gradle index e69de29bb2d..153124e84b0 100644 --- a/test/fixtures/build.gradle +++ b/test/fixtures/build.gradle @@ -0,0 +1,5 @@ + +subprojects { + // fixtures are mostly external and by default we don't want to check forbidden apis + forbiddenApisMain.enabled = false +} diff --git a/test/framework/build.gradle b/test/framework/build.gradle index 5f1bc524da5..ab513a1b0bb 100644 --- a/test/framework/build.gradle +++ b/test/framework/build.gradle @@ -41,8 +41,9 @@ compileTestJava.options.compilerArgs << '-Xlint:-rawtypes' // the main files are actually test files, so use the appropriate forbidden api sigs forbiddenApisMain { - signaturesURLs = [PrecommitTasks.getResource('/forbidden/all-signatures.txt'), - PrecommitTasks.getResource('/forbidden/test-signatures.txt')] + signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt'), + PrecommitTasks.getResource('/forbidden/es-all-signatures.txt'), + PrecommitTasks.getResource('/forbidden/es-test-signatures.txt')] } // TODO: should we have licenses for our test deps? diff --git a/test/framework/src/main/java/org/elasticsearch/indices/analysis/AnalysisFactoryTestCase.java b/test/framework/src/main/java/org/elasticsearch/indices/analysis/AnalysisFactoryTestCase.java index 3fded43d858..5298c3995ce 100644 --- a/test/framework/src/main/java/org/elasticsearch/indices/analysis/AnalysisFactoryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/indices/analysis/AnalysisFactoryTestCase.java @@ -67,7 +67,7 @@ public abstract class AnalysisFactoryTestCase extends ESTestCase { Matcher m = UNDERSCORE_THEN_ANYTHING.matcher(s); StringBuffer sb = new StringBuffer(); while (m.find()) { - m.appendReplacement(sb, m.group(1).toUpperCase()); + m.appendReplacement(sb, m.group(1).toUpperCase(Locale.ROOT)); } m.appendTail(sb); sb.setCharAt(0, Character.toUpperCase(sb.charAt(0))); diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java index 0a11325311d..58593cbe2fd 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java @@ -21,7 +21,6 @@ package org.elasticsearch.test; import com.carrotsearch.randomizedtesting.RandomizedTest; import com.carrotsearch.randomizedtesting.SeedUtils; - import org.apache.lucene.index.IndexReader; import org.apache.lucene.util.Accountable; import org.elasticsearch.Version; @@ -42,6 +41,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsModule; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.index.Index; @@ -194,8 +194,8 @@ public abstract class AbstractBuilderTestCase extends ESTestCase { @AfterClass public static void afterClass() throws Exception { - org.apache.lucene.util.IOUtils.close(serviceHolder); - org.apache.lucene.util.IOUtils.close(serviceHolderWithNoType); + IOUtils.close(serviceHolder); + IOUtils.close(serviceHolderWithNoType); serviceHolder = null; serviceHolderWithNoType = null; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index 1498a69cf6a..922a6e0d276 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -54,6 +54,7 @@ import org.elasticsearch.cluster.ClusterModule; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.CheckedBiFunction; import org.elasticsearch.common.CheckedRunnable; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.io.PathUtilsForTesting; @@ -198,13 +199,9 @@ public abstract class ESTestCase extends LuceneTestCase { } static { - System.setProperty("log4j.shutdownHookEnabled", "false"); - System.setProperty("log4j2.disable.jmx", "true"); - + setTestSysProps(); LogConfigurator.loadLog4jPlugins(); - // Enable Netty leak detection and monitor logger for logged leak errors - System.setProperty("io.netty.leakDetection.level", "paranoid"); String leakLoggerName = "io.netty.util.ResourceLeakDetector"; Logger leakLogger = LogManager.getLogger(leakLoggerName); Appender leakAppender = new AbstractAppender(leakLoggerName, null, @@ -243,6 +240,14 @@ public abstract class ESTestCase extends LuceneTestCase { Collections.sort(javaZoneIds); JAVA_ZONE_IDS = Collections.unmodifiableList(javaZoneIds); } + @SuppressForbidden(reason = "force log4j and netty sysprops") + private static void setTestSysProps() { + System.setProperty("log4j.shutdownHookEnabled", "false"); + System.setProperty("log4j2.disable.jmx", "true"); + + // Enable Netty leak detection and monitor logger for logged leak errors + System.setProperty("io.netty.leakDetection.level", "paranoid"); + } protected final Logger logger = Loggers.getLogger(getClass()); protected final DeprecationLogger deprecationLogger = new DeprecationLogger(logger); diff --git a/test/framework/src/main/java/org/elasticsearch/test/fixture/AbstractHttpFixture.java b/test/framework/src/main/java/org/elasticsearch/test/fixture/AbstractHttpFixture.java index daa70298224..7fb4e7c55ff 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/fixture/AbstractHttpFixture.java +++ b/test/framework/src/main/java/org/elasticsearch/test/fixture/AbstractHttpFixture.java @@ -20,6 +20,8 @@ package org.elasticsearch.test.fixture; import com.sun.net.httpserver.HttpServer; +import org.elasticsearch.common.SuppressForbidden; +import org.elasticsearch.common.io.PathUtils; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -32,7 +34,6 @@ import java.net.SocketAddress; import java.net.URI; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.util.HashMap; import java.util.List; @@ -48,6 +49,7 @@ import static java.util.Collections.singletonMap; /** * Base class for test fixtures that requires a {@link HttpServer} to work. */ +@SuppressForbidden(reason = "uses httpserver by design") public abstract class AbstractHttpFixture { protected static final Map TEXT_PLAIN_CONTENT_TYPE = contentType("text/plain; charset=utf-8"); @@ -62,7 +64,7 @@ public abstract class AbstractHttpFixture { private final Path workingDirectory; protected AbstractHttpFixture(final String workingDir) { - this.workingDirectory = Paths.get(Objects.requireNonNull(workingDir)); + this.workingDirectory = PathUtils.get(Objects.requireNonNull(workingDir)); } /** diff --git a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java index aeffd26e214..cddcca59e6c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java +++ b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java @@ -22,6 +22,7 @@ import com.carrotsearch.randomizedtesting.ReproduceErrorMessageBuilder; import org.apache.logging.log4j.Logger; import org.apache.lucene.util.Constants; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESTestCase; @@ -86,7 +87,12 @@ public class ReproduceInfoPrinter extends RunListener { gradleMessageBuilder.appendClientYamlSuiteProperties(); } - System.err.println(b.toString()); + printToErr(b.toString()); + } + + @SuppressForbidden(reason = "printing repro info") + private static void printToErr(String s) { + System.err.println(s); } protected static class GradleMessageBuilder extends ReproduceErrorMessageBuilder { diff --git a/test/framework/src/main/java/org/elasticsearch/transport/AbstractSimpleTransportTestCase.java b/test/framework/src/main/java/org/elasticsearch/transport/AbstractSimpleTransportTestCase.java index 50b7b8ce575..20cce99477c 100644 --- a/test/framework/src/main/java/org/elasticsearch/transport/AbstractSimpleTransportTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/transport/AbstractSimpleTransportTestCase.java @@ -29,6 +29,7 @@ import org.elasticsearch.Version; import org.elasticsearch.action.ActionListenerResponseHandler; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.cluster.node.DiscoveryNode; +import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.StreamInput; @@ -64,6 +65,7 @@ import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.ServerSocket; import java.net.Socket; +import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -1894,7 +1896,7 @@ public abstract class AbstractSimpleTransportTestCase extends ESTestCase { // means that once we received an ACK from the client we just drop the packet on the floor (which is what we want) and we run // into a connection timeout quickly. Yet other implementations can for instance can terminate the connection within the 3 way // handshake which I haven't tested yet. - socket.bind(new InetSocketAddress(InetAddress.getLocalHost(), 0), 1); + socket.bind(getLocalEphemeral(), 1); socket.setReuseAddress(true); DiscoveryNode first = new DiscoveryNode("TEST", new TransportAddress(socket.getInetAddress(), socket.getLocalPort()), emptyMap(), @@ -2008,7 +2010,7 @@ public abstract class AbstractSimpleTransportTestCase extends ESTestCase { public void testTcpHandshakeTimeout() throws IOException { try (ServerSocket socket = new MockServerSocket()) { - socket.bind(new InetSocketAddress(InetAddress.getLocalHost(), 0), 1); + socket.bind(getLocalEphemeral(), 1); socket.setReuseAddress(true); DiscoveryNode dummy = new DiscoveryNode("TEST", new TransportAddress(socket.getInetAddress(), socket.getLocalPort()), emptyMap(), @@ -2029,7 +2031,7 @@ public abstract class AbstractSimpleTransportTestCase extends ESTestCase { public void testTcpHandshakeConnectionReset() throws IOException, InterruptedException { try (ServerSocket socket = new MockServerSocket()) { - socket.bind(new InetSocketAddress(InetAddress.getLocalHost(), 0), 1); + socket.bind(getLocalEphemeral(), 1); socket.setReuseAddress(true); DiscoveryNode dummy = new DiscoveryNode("TEST", new TransportAddress(socket.getInetAddress(), socket.getLocalPort()), emptyMap(), @@ -2665,4 +2667,8 @@ public abstract class AbstractSimpleTransportTestCase extends ESTestCase { protected abstract void closeConnectionChannel(Transport transport, Transport.Connection connection) throws IOException; + @SuppressForbidden(reason = "need local ephemeral port") + private InetSocketAddress getLocalEphemeral() throws UnknownHostException { + return new InetSocketAddress(InetAddress.getLocalHost(), 0); + } } diff --git a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java index bbff340c860..438151b51b9 100644 --- a/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java +++ b/test/framework/src/main/java/org/elasticsearch/transport/MockTcpTransport.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.transport; +import org.elasticsearch.cli.SuppressForbidden; import org.elasticsearch.core.internal.io.IOUtils; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; @@ -160,6 +161,7 @@ public class MockTcpTransport extends TcpTransport { } @Override + @SuppressForbidden(reason = "real socket for mocking remote connections") protected MockChannel initiateChannel(InetSocketAddress address, ActionListener connectListener) throws IOException { final MockSocket socket = new MockSocket(); final MockChannel channel = new MockChannel(socket, address, "none");