Fixing flaky test GeoPointShapeQueryTests.testQueryLinearRing

[shwetathareja]

Description

It fixes the GeoPointShapeQueryTests.testQueryLinearRing which has been failing across multiple PR during gradle-check

Related Issues

Resolves 11688

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)
• Public documentation issue/PR created

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.

[shwetathareja]

@peternied to help with review.

[github-actions]

Compatibility status:

Checks if related components are compatible with change e2276ac

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

[github-actions]

✅ Gradle check result for e2276ac: SUCCESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.44%. Comparing base (b15cb0c) to head (e2276ac).
Report is 57 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12783      +/-   ##
============================================
+ Coverage     71.42%   71.44%   +0.02%     
- Complexity    59978    60128     +150     
============================================
  Files          4985     4999      +14     
  Lines        282275   283041     +766     
  Branches      40946    41027      +81     
============================================
+ Hits         201603   202221     +618     
- Misses        63999    64030      +31     
- Partials      16673    16790     +117     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shwetathareja]

tagging @andrross / @rishabhmaurya as well

[Bukhtawar]

LGTM. Assuming that the test expects serialisation failure to be a valid test case and doesn't expect the test to fail

[shwetathareja]

Thanks @Bukhtawar for the review.

[peternied]

@shwetathareja Thanks for this improvement!

[dblock]

@shwetathareja Can you explain in what scenario we get "line ring cannot be serialized using GeoJson"? I read both the issue and this PR and I don't understand the conditions in which it's ok, and why we should be relying on an exception, and a broad one like UnsupportedOperationException, to ignore the failure, which can easily hide bugs when that error is thrown from elsewhere. Won't users get the same error when trace logging is enabled?

[shwetathareja]

@dblock It is a negative test. It expects the exception that serialization fail for Line ring and today for GeoJson, serialization of line ring is not supported.

Below is the existing catch and assertion in the test, we don't support Line ring as we can't serialize it. This is more readable message and wrapped under SearchPhaseExecutionException. But the inner cause for the same is UnsupportedOperationException exception only.

catch (SearchPhaseExecutionException e) {
            assertThat(
                e.getCause().getMessage(),
                containsString("Field [" + defaultGeoFieldName + "] does not support LINEARRING queries")
            );

a broad one like UnsupportedOperationException, to ignore the failure, which can easily hide bugs when that error is thrown from elsewhere.

we are not ignoring failure, it is asserting on exception message, if it fails for any other reason than serialization error, it will result in assertion failure.

[reta]

@shwetathareja The OpenSearch uses exception hierarchy to catch (and wrap) exceptions that happens during search into one of OpenSearchException subclasses. UnsupportedOperationException should never leak to the outside: if that occurs (which seems to be the case), we have a gap somewhere in the implementation, this is the place the fix is expected, it looks to me.

[dblock]

Consistent repro with ./gradlew ':server:test' --tests "org.opensearch.search.geo.GeoPointShapeQueryTests.testQueryLinearRing" -Dtests.opensearch.logger.level=TRACE.

[shwetathareja]

@reta : I don't think we wrap all sort of exceptions like IllegalArgumentException etc. are just thrown as is.

[reta]

@reta : I don't think we wrap all sort of exceptions like IllegalArgumentException etc. are just thrown as is.

@shwetathareja I am specifically referring to search flow in the comment

[andrross]

I think the fact that an UnsupportedOperationException is thrown when TRACE logging is enabled is a bug, and a serialization failure is not expected behavior when writing a log statement. Users will face the same pain that we see with tests if they enable TRACE logging in a running system (see #12324)