Skip to content

Skip AnalyticsEngineCompatIT when analytics-engine plugin is absent#5510

Merged
ahkcs merged 1 commit into
opensearch-project:mainfrom
ahkcs:fix/analytics-engine-compat-self-gate-main
Jun 4, 2026
Merged

Skip AnalyticsEngineCompatIT when analytics-engine plugin is absent#5510
ahkcs merged 1 commit into
opensearch-project:mainfrom
ahkcs:fix/analytics-engine-compat-self-gate-main

Conversation

@ahkcs
Copy link
Copy Markdown
Collaborator

@ahkcs ahkcs commented Jun 4, 2026

Description

AnalyticsEngineCompatIT is a coexistence smoke test whose only assertion is that the cluster boots. It is meant to run only in the dedicated :integ-test:analyticsEngineCompatIT Gradle task, which bundles the arrow + analytics-engine plugin stack on a non-security cluster.

The distribution integ-test pipeline behaves differently: it scans every *IT class against the built distribution and runs the with-security variant without the analytics-engine plugin. In that lane the test fails before any test body runs:

org.opensearch.sql.plugin.AnalyticsEngineCompatIT.testClusterStarted
  → org.apache.hc.core5.http.ConnectionClosedException: Connection closed by peer
      at OpenSearchRestTestCase.initClient(OpenSearchRestTestCase.java:216)

Root cause: the test extends a bare OpenSearchRestTestCase, which speaks plain HTTP. Against a TLS-secured cluster the security plugin closes the connection during client init. The existing build.gradle exclude only governs this repo's own integTest task — it does not cover the external distribution pipeline that discovers the class generically.

Fix

Make the test self-inert no matter how it is discovered:

  • Extend OpenSearchSQLRestTestCase so the REST client honours the https / user / password system properties of a secured cluster (TLS trust + basic auth), instead of failing the connection.
  • Skip via an assumption (assumeTrue) when the analytics-engine plugin is not installed — verified through _cat/plugins. A build without the plugin now reports the test as skipped rather than failed.

Resulting behaviour:

Lane Before After
analyticsEngineCompatIT task (plugin present) pass pass
Distribution build, no security, no analytics-engine n/a skipped
Distribution build, with security, no analytics-engine FAIL (connection closed) skipped

Check List

  • New functionality includes testing — change is itself an integration test; behaviour validated against secured / unsecured / plugin-absent lanes.
  • 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

PR Reviewer Guide 🔍

(Review updated until commit 01ada06)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

PR Code Suggestions ✨

Latest suggestions up to 01ada06
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add null-safety check for response body

The plugin check may fail if the response body is null or the API returns an
unexpected format. Add null-safety checks and handle potential exceptions from
getResponseBody to prevent test failures in edge cases where the cluster state is
unusual.

integ-test/src/test/java/org/opensearch/sql/plugin/AnalyticsEngineCompatIT.java [47-51]

 Response response = client().performRequest(new Request("GET", "/_cat/plugins?h=component"));
 String installedPlugins = TestUtils.getResponseBody(response, true);
 assumeTrue(
     "analytics-engine plugin not installed — skipping coexistence smoke test",
-    installedPlugins.contains("analytics-engine"));
+    installedPlugins != null && installedPlugins.contains("analytics-engine"));
Suggestion importance[1-10]: 5

__

Why: Adding a null check for installedPlugins is a reasonable defensive programming practice. However, the impact is moderate since TestUtils.getResponseBody likely handles null cases internally, and the test would fail earlier if the response was malformed. The suggestion correctly identifies the code location and provides a valid improvement.

Low

Previous suggestions

Suggestions up to commit 1070afd
CategorySuggestion                                                                                                                                    Impact
General
Add null-safety check for response body

The plugin check may fail if the response body is null or the API returns an
unexpected format. Add null-safety checks and handle potential exceptions from
getResponseBody to prevent test failures in edge cases where the cluster state is
unusual.

integ-test/src/test/java/org/opensearch/sql/plugin/AnalyticsEngineCompatIT.java [47-51]

 Response response = client().performRequest(new Request("GET", "/_cat/plugins?h=component"));
 String installedPlugins = TestUtils.getResponseBody(response, true);
 assumeTrue(
     "analytics-engine plugin not installed — skipping coexistence smoke test",
-    installedPlugins.contains("analytics-engine"));
+    installedPlugins != null && installedPlugins.contains("analytics-engine"));
Suggestion importance[1-10]: 5

__

Why: Adding a null check for installedPlugins is a reasonable defensive programming practice. However, the suggestion assumes TestUtils.getResponseBody() might return null without evidence from the PR context, and the impact is relatively minor since the test would fail clearly if null were returned.

Low

AnalyticsEngineCompatIT is a coexistence smoke test meant to run only in the
dedicated :integ-test:analyticsEngineCompatIT task, which bundles the
analytics-engine plugin stack. The distribution integ-test pipeline, however,
scans every *IT class against the built distribution and runs it with the
security plugin enabled and without analytics-engine. There it fails during
REST client init with "ConnectionClosedException: Connection closed by peer",
because the test extends a bare OpenSearchRestTestCase that speaks plain HTTP
to the TLS-secured port. A build.gradle exclude does not cover that pipeline.

Make the test self-inert regardless of how it is discovered:

- Extend OpenSearchSQLRestTestCase so the REST client honours the https/user/
  password system properties of a secured cluster.
- Skip via an assumption when the analytics-engine plugin is not installed, so
  a build without the plugin reports the test as skipped rather than failed.

Signed-off-by: Kai Huang <ahkcs@amazon.com>
@ahkcs ahkcs force-pushed the fix/analytics-engine-compat-self-gate-main branch from 1070afd to 01ada06 Compare June 4, 2026 05:42
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

Persistent review updated to latest commit 01ada06

@ahkcs ahkcs merged commit 7054bb5 into opensearch-project:main Jun 4, 2026
40 checks passed
@ahkcs ahkcs deleted the fix/analytics-engine-compat-self-gate-main branch June 4, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants