[FEATURE] Refactor ExtensionsRunner test code out of production source tree

[dbwiddis]

Is your feature request related to a problem?

We shouldn't have exclusively test-related code in the main source tree, e.g.,

opensearch-sdk-java/src/main/java/org/opensearch/sdk/ExtensionsRunner.java

Lines 117 to 123 in 8fbcc6e

	/**
	* Instantiates a new Extensions Runner using test settings.
	*
	* @throws IOException if the runner failed to read settings or API.
	*/
	public ExtensionsRunner() throws IOException {
	ExtensionSettings extensionSettings = readExtensionSettings();

which calls here, only by tests:

opensearch-sdk-java/src/main/java/org/opensearch/sdk/ExtensionsRunner.java

Lines 159 to 163 in 8fbcc6e

	private static ExtensionSettings readExtensionSettings() throws IOException {
	File file = new File(ExtensionSettings.EXTENSION_DESCRIPTOR);
	ObjectMapper objectMapper = new ObjectMapper(new YAMLFactory());
	return objectMapper.readValue(file, ExtensionSettings.class);
	}

which uses this constant pointing to the test source tree:

opensearch-sdk-java/src/main/java/org/opensearch/sdk/ExtensionSettings.java

Lines 19 to 22 in 8fbcc6e

	/**
	* Path to extension.yml file for testing. Internal use only.
	*/
	static final String EXTENSION_DESCRIPTOR = "src/test/resources/extension.yml";

What solution would you like?

1. Remove the no-arg constructor above from ExtensionsRunner
2. Make the constructor with an Extension argument either package-private (probably) or protected (maybe) so it's accessible by tests
3. Create a new subclass of ExtensionsRunner in the src/test/java tree to be used as an ExtensionsRunner object when needed for tests, such as the linked comment in the context section below. Move the above code snippets there with modifications as noted below.
4. Update all tests which use the existing no-arg constructor to use the new class.

One would think TestExtensionsRunner would be a good name but it's already in use for our test class. Picking a new name will probably be the hardest part. It can extend ExtensionsRunner like so:

public class ExtensionsRunnerForTesting extends ExtensionsRunner {

    public ExtensionsRunnerForTesting() throws IOException {
        super(new Extension () {

            @Override
            public ExtensionSettings getExtensionSettings() {
                // Move the result of current readExtensionSettings() method of ExtensionsRunner here
                // except get rid of yaml file reading, and just instantiate the object with the values in the yaml
                return new ExtensionSettings( ... );
            }

            @Override
            public List<ExtensionRestHandler> getExtensionRestHandlers() {
                return Collections.emptyList();
            }
        });
    }
}

What alternatives have you considered?

We could use the "sample" extensions for this purpose, or even create a test extension in the sample branch. This seems reasonable for an IT-based approach, but I think this approach is better for unit testing requiring an object with minimal settings.

Do you have any additional context?

I wonder if there's value in creating a "Test" version of the ExtensionsRunner. The no-arg constructor here is used exclusively for testing and I'd like to see it removed and replaced with some TestExtensionsRunner class in the test suite.

Originally posted by @dbwiddis in #170 (comment)

[roulpriya]

Hi @dbwiddis,
I am interested to work on this issue.

[dbwiddis]

Great, @roulpriya! Let us know if you have any questions!

[roulpriya]

I faced some dependency resolution issue and I appear to have fixed it by making these changes. Does this look good?

diff --git a/build.gradle b/build.gradle
index 7012c38..24c4024 100644
--- a/build.gradle
+++ b/build.gradle
@@ -48,7 +48,7 @@ publishing {
 repositories {
     mavenLocal()
     // Remove the commented code below once TransportService is published to maven
-    //maven { url "https://aws.oss.sonatype.org/content/repositories/snapshots" }
+    maven { url "https://aws.oss.sonatype.org/content/repositories/snapshots" }
     maven { url "https://d1nvenhzbhpy0q.cloudfront.net/snapshots/lucene/"}
     mavenCentral()
 }
@@ -70,8 +70,8 @@ dependencies {
         exclude module : 'hamcrest-core'
     }
     implementation 'javax.xml.bind:jaxb-api:2.2.2'
-    implementation 'com.fasterxml.jackson.core:jackson-databind: 2.12.6.1'
-    implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml: 2.12.6.1'
+    implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.6.1'
+    implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.12.6'
     testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
     testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.2'
     testImplementation "org.opensearch.test:framework:3.0.0-SNAPSHOT"

[dbwiddis]

See the developer guide. You will need to clone the main OpenSearch project, check out the feature/extensions branch, and publish it to Maven local.

https://github.com/opensearch-project/opensearch-sdk-java/blob/main/DEVELOPER_GUIDE.md

[roulpriya]

Thank you @dbwiddis I am able to set up the project.

However, on trying to compile I get this compilation error:

TransportActions.java:57: error: incompatible types: Map<String,Class> cannot be converted to StreamInput
                new RegisterTransportActionsRequest(transportActions),

I checked the corresponding source in open-search and perhaps the signature of the function being called here has changed?

[roulpriya]

I also have some additional issues with the code. I have opened a draft PR https://github.com/opensearch-project/opensearch-sdk-java/pull/172/files#r990832674

[dbwiddis]

TransportActions.java:57: error: incompatible types

I need more of that stack trace. There should be a request or response class associated with that.

It's also possible your build.gradle changes are bringing in the wrong dependency.

[dbwiddis]

Hey @roulpriya just summarizing since I've spread some comments across here and your PR:

1. I think your TransportActions error is associated with your changes you did to build.gradle in your first commit. Back those out and only use the locally published maven commits.
2. That startActionListener(0) in the inherited constructor is going to present a problem for us. I think moving it out of the constructor and into the run() method may work.
3. We need to decide where to have ./gradlew run go. If we can make it run the new test class you're creating (with a main method there) that'd be ideal.

Sorry this was a little more complicated than I first thought! Looks like you're making good progress though.

[roulpriya]

I think the TransportActions is trying to invoke this constructor in RegisterTransportActionRequest
https://github.com/opensearch-project/OpenSearch/blob/eee59c51b955eeea4313b36455bb07e5d4440dca/server/src/main/java/org/opensearch/extensions/RegisterTransportActionsRequest.java#L29

Looking at this file I don't see any single arg constructor taking a Map

[dbwiddis]

Looking at this file I don't see any single arg constructor taking a Map

Ah, yes, I see that line 57 should also include the unique ID. There was a single arg constructor in #4326, but we apparently broke it in #4598 2 days ago which needed a corresponding change in the SDK.

For now just add an empty string argument in TransportActions line 57, and @saratvemulapalli can follow up later to properly add the UniqueID to match his change in #4598.

[dbwiddis]

Actually, @roulpriya, there is an open PR #164 which should be merged and will fix this issue... sorry you caught us between a Friday and Monday merge of corresponding PRs.

[roulpriya]

Now I am able to build and run individual test but running all at once fails because we create a new ExtentionRunner in setup which starts the TransportService to listen on 4532. After test run we do not have a way to shut down this listener.

opensearch-sdk-java/src/main/java/org/opensearch/sdk/ExtensionsRunner.java

Line 154 in 8fbcc6e

nettyTransport.initializeExtensionTransportService(this.getSettings(), this);

I feel we need a corresponding shutdown method as well.

Failed to bind to 127.0.0.1:4532
BindTransportException[Failed to bind to 127.0.0.1:4532]; nested: BindException[Address already in use];
	at app//org.opensearch.transport.TcpTransport.bindToPort(TcpTransport.java:468)
	at app//org.opensearch.transport.TcpTransport.bindServer(TcpTransport.java:432)
	at app//org.opensearch.transport.netty4.Netty4Transport.doStart(Netty4Transport.java:163)
	at app//org.opensearch.common.component.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:77)
	at app//org.opensearch.transport.TransportService.doStart(TransportService.java:283)
	at app//org.opensearch.common.component.AbstractLifecycleComponent.start(AbstractLifecycleComponent.java:77)
	at app//org.opensearch.sdk.ExtensionsRunner.startTransportService(ExtensionsRunner.java:178)
	at app//org.opensearch.sdk.NettyTransport.initializeExtensionTransportService(NettyTransport.java:116)
	at app//org.opensearch.sdk.ExtensionsRunner.<init>(ExtensionsRunner.java:134)
	at app//org.opensearch.sdk.ExtensionsRunnerForTest.<init>(ExtensionsRunnerForTest.java:20)
	at app//org.opensearch.sdk.TestExtensionsRunner.setUp(TestExtensionsRunner.java:84)
	at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base@11.0.16.1/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base@11.0.16.1/java.lang.reflect.Method.invoke(Method.java:566)
	at app//org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
	at app//org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptLifecycleMethod(TimeoutExtension.java:126)
	at app//org.junit.jupiter.engine.extension.TimeoutExtension.interceptBeforeEachMethod(TimeoutExtension.java:76)
	at app//org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at app//org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at app//org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at app//org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
	at app//org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
	at app//org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.invokeMethodInExtensionContext(ClassBasedTestDescriptor.java:506)
	at app//org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$synthesizeBeforeEachMethodAdapter$21(ClassBasedTestDescriptor.java:491)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeBeforeEachMethods$3(TestMethodTestDescriptor.java:171)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeBeforeMethodsOrCallbacksUntilExceptionOccurs$6(TestMethodTestDescriptor.java:199)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeBeforeMethodsOrCallbacksUntilExceptionOccurs(TestMethodTestDescriptor.java:199)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeBeforeEachMethods(TestMethodTestDescriptor.java:168)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:131)
	at app//org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:66)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base@11.0.16.1/java.util.ArrayList.forEach(ArrayList.java:1541)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at java.base@11.0.16.1/java.util.ArrayList.forEach(ArrayList.java:1541)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
	at app//org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
	at app//org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
	at app//org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
	at app//org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at app//org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:108)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
	at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:96)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:75)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:99)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:79)
	at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:75)
	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:61)
	at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base@11.0.16.1/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base@11.0.16.1/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base@11.0.16.1/java.lang.reflect.Method.invoke(Method.java:566)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
	at com.sun.proxy.$Proxy5.stop(Unknown Source)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
	at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
	at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
	at app//worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
Caused by: java.net.BindException: Address already in use
	at java.base/sun.nio.ch.Net.bind0(Native Method)
	at java.base/sun.nio.ch.Net.bind(Net.java:459)
	at java.base/sun.nio.ch.Net.bind(Net.java:448)
	at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:227)
	at io.netty.channel.socket.nio.NioServerSocketChannel.doBind(NioServerSocketChannel.java:141)
	at io.netty.channel.AbstractChannel$AbstractUnsafe.bind(AbstractChannel.java:562)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.bind(DefaultChannelPipeline.java:1334)
	at io.netty.channel.AbstractChannelHandlerContext.invokeBind(AbstractChannelHandlerContext.java:506)
	at io.netty.channel.AbstractChannelHandlerContext.bind(AbstractChannelHandlerContext.java:491)
	at io.netty.channel.DefaultChannelPipeline.bind(DefaultChannelPipeline.java:973)
	at io.netty.channel.AbstractChannel.bind(AbstractChannel.java:260)
	at io.netty.bootstrap.AbstractBootstrap$2.run(AbstractBootstrap.java:356)
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174)
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167)
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:503)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at java.base/java.lang.Thread.run(Thread.java:829)

[dbwiddis]

After test run we do not have a way to shut down this listener.

Ugh.... so sorry this is more complex than I anticipated!

extensionsRunner.extensionTransportService.stop() should probably stop it, or creating it with a timeout in the action listener which I think shuts it down. But we would need to carefully sequence all the tests to make sure only one is using the object at any given time.

Sorry for all these complexities!