Add coverage for NeuralSearch class

[dbwiddis]

Description

Adds test coverage for the main plugin class file

Related Issues

Part of #429

Check List

• New functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[vibrantvarun]

@dbwiddis can you check why gradle check is failing?

[dbwiddis]

@dbwiddis can you check why gradle check is failing?

Unrelated to my PR:

Tests with failures:
 - org.opensearch.neuralsearch.query.HybridQueryWeightTests.testSubQueries_whenMultipleEqualSubQueries_thenSuccessful
 - org.opensearch.neuralsearch.query.HybridQueryWeightTests.classMethod
 - org.opensearch.neuralsearch.search.query.HybridQueryPhaseSearcherTests.testQueryResult_whenMultipleTextSubQueriesWithSomeHits_thenHybridResultsAreSet
 - org.opensearch.neuralsearch.search.query.HybridQueryPhaseSearcherTests.classMethod
446 tests completed, 4 failed

I can keep force-pushing until it's green but generally it's easier for repo maintainers to retry just the failed tests.

[vibrantvarun]

@dbwiddis can you add

@ThreadLeakScope(ThreadLeakScope.Scope.NONE)

HybridQueryWeightTests and HybridQueryPhaseSearcherTests classes and add it into your PR ?

[dbwiddis]

@dbwiddis can you check why gradle check is failing?

Looking into the log, it seems it's timing out and it's likely related to thread locking. Multiple threads show

  2> 	Locked synchronizers:
  2> 	- java.util.concurrent.ThreadPoolExecutor$Worker@386f0da3

This makes me suspect some interaction with this PR which does mock a Threadpool and an ExcecutorService to be returned by the threadpool:

when(threadPool.executor(anyString())).thenReturn(executorService);

I expected that mock to be isolated only to this test, and of course when run in isolation the test passes. However, looking at the plugin createComponents, I see this line:

neural-search/src/main/java/org/opensearch/neuralsearch/plugin/NeuralSearch.java

Line 101 in f58d989

HybridQueryExecutor.initialize(threadPool);

That leads to this line:

neural-search/src/main/java/org/opensearch/neuralsearch/executors/HybridQueryExecutor.java

Line 60 in f58d989

taskExecutor = new TaskExecutor(threadPool.executor(HYBRID_QUERY_EXEC_THREAD_POOL_NAME));

That sets the value of a static variable, which is therefore no longer isolated to that test: it remains stuck with a mocked value for any test run after this one.

neural-search/src/main/java/org/opensearch/neuralsearch/executors/HybridQueryExecutor.java

Line 31 in f58d989

private static TaskExecutor taskExecutor;

@dbwiddis can you add

@ThreadLeakScope(ThreadLeakScope.Scope.NONE)

I could replace the mock with a real thread pool and executor, somewhat like this pattern we use in Flow Framework and that I've carried over into ML Commons in a few places, and shut it down (the correct way to avoid the thread leak scope alert)

public class ProcessNodeTests extends OpenSearchTestCase {

    private static TestThreadPool testThreadPool;

    @BeforeClass
    public static void setup() {
        testThreadPool = new TestThreadPool(
            ProcessNodeTests.class.getName(),
            new ScalingExecutorBuilder(
                PROVISION_WORKFLOW_THREAD_POOL,
                1,
                Math.max(1, OpenSearchExecutors.allocatedProcessors(Settings.EMPTY) - 1),
                TimeValue.timeValueMinutes(5),
                FLOW_FRAMEWORK_THREAD_POOL_PREFIX + PROVISION_WORKFLOW_THREAD_POOL
            )
        );
    }

    @AfterClass
    public static void cleanup() {
        ThreadPool.terminate(testThreadPool, 500, TimeUnit.MILLISECONDS);
    }

But this works because the static variable is confined to this class and used only within this test. If I used this pattern in this test, it would still initialize the TaskExecutor in HybridQueryExecutor to this pool, and it would break tests when it was shut down.

I could leave it running for the entire duration of the test suite, but OpenSearchTestCase has leak detection mechanisms, which you've alluded to with the @ThreadLeakScope(ThreadLeakScope.Scope.NONE) annotation, but I submit this is just hiding the symptom of a real thread leak.

This still doesn't solve the underlying problem: there is no way to ever execute createComponents without a real thread pool you want to use for all the tests, that you can shut down at the end of your testing.

Putting aside the question of whether a static object initialized by a different class is even a good idea at all (which I don't think it is), at this point I would prefer to just remove the perfunctory test of createComponents() whose entire raison d'être is to exercise the method and cover those lines to increase an arbitrary coverage metric.

[dbwiddis]

TLDR: I'll push a commit removing the coverage for createComponents() and leave it to a more well thought out design.

[dbwiddis]

OpenSearch has a direct executor service that simulates the (Runnable::run) default behavior of the non-initialized taskExecutor, so I used that.

[vibrantvarun]

@martin-gaievski can you review this PR?