[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