[Tests] Test utils update to fix IT tests for serverless #2869

manasvinibs · 2024-07-29T22:35:21Z

Description

Rounding difference between test results when run with different sources - added 2 digit precision round of for the failing tests. Sample test failure -

Expected: (a collection containing [-6.12])
     but: a collection containing [-6.12] mismatches were: [was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>, was <[-6.119999999999999]>]
	at __randomizedtesting.SeedInfo.seed([B1A9C07A3606A61E:6E333FC43A7C1CD3]:0)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.opensearch.sql.util.MatcherUtils.verifySome(MatcherUtils.java:192)
	at org.opensearch.sql.sql.AggregationIT.testAvgDoubleInMemory(AggregationIT.java:461)

Score Differences between test results when run with different data sources if one of the data source contains multiple shards and OpenSearch assumes to be run against single shard - skipped asserting on score part of the address array in the response as results can be different for different data sources and is expected behavior.

java.lang.AssertionError: 
Expected: iterable with items [[305 Powell Street, 6.501515]] in any order
     but: not matched: <["305 Powell Street",4.414816]>
	at __randomizedtesting.SeedInfo.seed([B1A9C07A3606A61E:726F6D646D7EFC49]:0)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at org.opensearch.sql.util.MatcherUtils.verify(MatcherUtils.java:173)
	at org.opensearch.sql.util.MatcherUtils.verifyDataRows(MatcherUtils.java:149)
	at org.opensearch.sql.sql.ScoreQueryCloudNativeIT.scoreQueryDefaultBoostQueryTest(ScoreQueryIT.java:106)

Retrying the index creation without refresh policy in the request URL for tests when it is not supported by API. Tests shouldn't fail as it is expected behavior.

java.lang.IllegalStateException: Failed to perform request
	at __randomizedtesting.SeedInfo.seed([B1A9C07A3606A61E:D3800B42483A3073]:0)
	at org.opensearch.sql.util.TestUtils.performRequest(TestUtils.java:124)
	at org.opensearch.sql.sql.IdentifierIT$Index.addDocHelper(IdentifierIT.java:287)
	at org.opensearch.sql.sql.IdentifierIT.createIndexWithOneDoc(IdentifierIT.java:242)
	at org.opensearch.sql.sql.IdentifierIT.testIndexNames(IdentifierIT.java:34)

at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: org.opensearch.client.ResponseException: method [PUT], host [https://ruh7jos10anqd6bsp844.beta-us-east-1.aoss.amazonaws.com], URI [/logs/_doc/1?refresh=true], status line [HTTP/1.1 400 Bad Request]
{"error":{"root_cause":[{"type":"status_exception","reason":"true refresh policy is not supported."}],"type":"status_exception","reason":"true refresh policy is not supported."},"status":400}
	at app//org.opensearch.client.RestClient.convertResponse(RestClient.java:376)

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
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.

rupal-bq · 2024-08-01T01:15:26Z

integ-test/src/test/java/org/opensearch/sql/util/TestUtils.java

@@ -772,6 +779,29 @@ public static String getResponseBody(Response response, boolean retainNewLines)
    return sb.toString();
  }

+  // TODO: this is temporary fix for fixing serverless tests to pass with 2 digit precision value


any thoughts on what should be long term fix for this and other temp fixes? Also, should we create issues to track long term fix?

ya I can check with serverless team on this and see if we need a issue to track the long term fix.

integ-test/src/test/java/org/opensearch/sql/sql/AggregationIT.java

integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java

integ-test/src/test/java/org/opensearch/sql/util/TestUtils.java

ykmr1224 · 2024-08-01T18:53:50Z

integ-test/src/test/java/org/opensearch/sql/util/TestUtils.java

@@ -123,6 +118,18 @@ public static Response performRequest(RestClient client, Request request) {
      }
      return response;
    } catch (IOException e) {
+      if (e instanceof ResponseException
+          && ((ResponseException) e).getResponse().getStatusLine().getStatusCode() == 400
+          && e.getMessage().contains("true refresh policy is not supported.")) {


Is it safe to match with message? If there is no other way, that's fine as it is test code, but error message tends to be easily changed.

I looked into specific exception we get for refresh policy related error. But today its wrapped as ResponseException. I think I need to have this check to make sure I'm retrying exactly for the refresh issue only. As it is a test code, if there are message updates on the exception, tests would fail and we might have to adjust the message here for that in future.

ykmr1224 · 2024-08-01T18:56:33Z

integ-test/src/test/java/org/opensearch/sql/util/TestUtils.java

@@ -123,6 +118,18 @@ public static Response performRequest(RestClient client, Request request) {
      }
      return response;
    } catch (IOException e) {
+      if (e instanceof ResponseException


Can't we remove the refresh policy when it is requesting collection from the beginning?

I don't think the URL with refresh policy token is set during @before setup or during client connection. I see we have few tests which adds this token within test files before submitting query like this: https://github.com/opensearch-project/sql/blob/main/integ-test/src/test/java/org/opensearch/sql/sql/IdentifierIT.java#L245 All those tests rely on this test util function, so I thought of adding the retry here.

Signed-off-by: Manasvini B S <manasvis@amazon.com>

Signed-off-by: Manasvini B S <manasvis@amazon.com> (cherry picked from commit 7e73f12) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…2869) Signed-off-by: Manasvini B S <manasvis@amazon.com>

manasvinibs marked this pull request as ready for review July 29, 2024 23:21

manasvinibs requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224 and LantaoJin as code owners July 29, 2024 23:21

rupal-bq reviewed Aug 1, 2024

View reviewed changes

rupal-bq previously approved these changes Aug 1, 2024

View reviewed changes

ykmr1224 reviewed Aug 1, 2024

View reviewed changes

manasvinibs dismissed rupal-bq’s stale review via 278a2d3 August 2, 2024 06:11

manasvinibs force-pushed the IT-tests-fix branch from 0bdeec9 to 278a2d3 Compare August 2, 2024 06:11

Test utils update to fix IT tests for serverless

c72c89a

Signed-off-by: Manasvini B S <manasvis@amazon.com>

manasvinibs force-pushed the IT-tests-fix branch from 278a2d3 to c72c89a Compare August 2, 2024 06:17

manasvinibs requested review from ykmr1224 and rupal-bq August 2, 2024 17:21

rupal-bq approved these changes Aug 2, 2024

View reviewed changes

ykmr1224 approved these changes Aug 5, 2024

View reviewed changes

ykmr1224 merged commit 7e73f12 into opensearch-project:main Aug 5, 2024
14 checks passed

manasvinibs added the backport 2.x label Aug 5, 2024

opensearch-trigger-bot bot mentioned this pull request Aug 5, 2024

[Backport 2.x] [Tests] Test utils update to fix IT tests for serverless #2898

Open

manasvinibs added a commit to manasvinibs/sql that referenced this pull request Aug 14, 2024

Test utils update to fix IT tests for serverless (opensearch-project#…

6ff3f78

…2869) Signed-off-by: Manasvini B S <manasvis@amazon.com>

jzonthemtn pushed a commit to jzonthemtn/sql that referenced this pull request Aug 28, 2024

Test utils update to fix IT tests for serverless (opensearch-project#…

807e5e7

…2869) Signed-off-by: Manasvini B S <manasvis@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tests] Test utils update to fix IT tests for serverless #2869

[Tests] Test utils update to fix IT tests for serverless #2869

manasvinibs commented Jul 29, 2024 •

edited

Loading

rupal-bq Aug 1, 2024

manasvinibs Aug 1, 2024

ykmr1224 Aug 1, 2024

manasvinibs Aug 2, 2024

ykmr1224 Aug 1, 2024

manasvinibs Aug 2, 2024 •

edited

Loading

[Tests] Test utils update to fix IT tests for serverless #2869

[Tests] Test utils update to fix IT tests for serverless #2869

Conversation

manasvinibs commented Jul 29, 2024 • edited Loading

Description

Issues Resolved

Check List

rupal-bq Aug 1, 2024

Choose a reason for hiding this comment

manasvinibs Aug 1, 2024

Choose a reason for hiding this comment

ykmr1224 Aug 1, 2024

Choose a reason for hiding this comment

manasvinibs Aug 2, 2024

Choose a reason for hiding this comment

ykmr1224 Aug 1, 2024

Choose a reason for hiding this comment

manasvinibs Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

manasvinibs commented Jul 29, 2024 •

edited

Loading

manasvinibs Aug 2, 2024 •

edited

Loading