Removing explicit SearchResponse usages in tests - v2 #102021

pmpailis · 2023-11-10T14:48:10Z

Tests covered in this PR:

org.elasticsearch.index.reindex.BulkByScrollUsesAllScrollDocumentsAfterConflictsIntegTests
org.elasticsearch.reindex.ClientScrollableHitSourceTests
org.elasticsearch.search.CrossClusterSearchUnavailableClusterIT
org.elasticsearch.searchCCSDuelIT
org.elasticsearch.xpack.analytics.aggregations.metrics.HistogramPercentileAggregationTests
org.elasticsearch.xpack.CcrIntegTestCase
org.elasticsearch.snapshots.sourceonly.SourceOnlySnapshotIT
org.elasticsearch.xpack.downsample.DownsampleClusterDisruptionIT
org.elasticsearch.xpack.downsample.DownsampleTransportFailureIT
org.elasticsearch.xpack.downsample.ILMDownsampleDisruptionIT
org.elasticsearch.xpack.downsample.DownsampleDataStreamTests

elasticsearchmachine · 2023-11-10T14:48:34Z

Pinging @elastic/es-search (Team:Search)

…earch-response-test-v2

JVerwolf · 2023-11-10T22:07:24Z

@elasticmachine update branch

…com:pmpailis/elasticsearch into fix/remove-explicit-search-response-test-v2

iverase · 2023-11-12T09:43:13Z

.../elasticsearch/index/reindex/BulkByScrollUsesAllScrollDocumentsAfterConflictsIntegTests.java

-            if (scriptEnabled && searchHit.getSourceAsMap().containsKey(RETURN_NOOP_FIELD)) {
-                conflictingOps--;
+        SearchResponse searchResponse = null;
+        try {


I think we can use assertResponse here too so we don't need to decref in the test:

int conflictingOps = randomIntBetween(maxDocs, numDocs); final int finalConflictingOps = conflictingOps; final List<SearchHit> docsModifiedConcurrently = new ArrayList<>(); assertResponse(prepareSearch(sourceIndex).setSize(numDocs) // Get all indexed docs .addSort(SORTING_FIELD, SortOrder.DESC), response -> { // Modify a subset of the target documents concurrently final List<SearchHit> originalDocs = Arrays.asList(response.getHits().getHits()); docsModifiedConcurrently.addAll(randomSubsetOf(finalConflictingOps, originalDocs)); });

There are some subsequent calls to things initialized in the snippet above, which end up going up until the end of the method (including further assertions). So, instead of having a very large wrapper using assertResponse or declaring things outside of the consumer, I opted for a try/finally block. WDYT? 🤔

As I said bellow, we should try to call decRef only from ElasticAssertions for maintainability.

Ok, fair enough! Updated.

As we also have a nested assertBusy that throws a generic Exception, is it ok to update the signature of assertCheckedResponse to throw an Exception, or do we want to keep it restricted to only specific types (currently it's IOException, ExecutionException, InterruptedException ) ?

I don't think the assertBusy needs to be nested? you just need to collect the result like in my example?

docsModifiedConcurrently.addAll(randomSubsetOf(finalConflictingOps, originalDocs));

🤦 Yeah you're right.. For some reason I missed the declarations outside of the wrapper in your example (hence why I mentioned them as an "alternative" in the subsequent comment). Apologies for the noise. :/

iverase · 2023-11-12T10:01:13Z

.../elasticsearch/xpack/analytics/aggregations/metrics/HistogramPercentileAggregationTests.java

+        SearchResponse responseRaw = null;
+        SearchResponse responsePreAgg = null;
+        SearchResponse responseBoth = null;
+        try {


We can nest assertResponse calls here like:

assertResponse( client().prepareSearch("raw").addAggregation(builder), responseRaw -> assertResponse( client().prepareSearch("pre_agg").addAggregation(builder), responsePreAgg -> assertResponse(client().prepareSearch("pre_agg", "raw").addAggregation(builder), responseBoth -> { InternalHDRPercentiles percentilesRaw = responseRaw.getAggregations().get("agg"); InternalHDRPercentiles percentilesPreAgg = responsePreAgg.getAggregations().get("agg"); InternalHDRPercentiles percentilesBoth = responseBoth.getAggregations().get("agg"); for (int i = 1; i < 100; i++) { assertEquals(percentilesRaw.percentile(i), percentilesPreAgg.percentile(i), 0.0); assertEquals(percentilesRaw.percentile(i), percentilesBoth.percentile(i), 0.0); } }) ) );

Tbh opted to avoid multiple nesting calls to be more straightforward to read, but I can see that we also want to move the decRef responsibility outside of each test method whenever's possible. Updated.

We should avoid calling decRef all over the tests as this makes very difficult to maintain. We should minimise as much as possible the places where we call that method.

iverase · 2023-11-12T12:47:45Z

...s/src/javaRestTest/java/org/elasticsearch/search/CrossClusterSearchUnavailableClusterIT.java

-                assertSame(SearchResponse.Clusters.EMPTY, response.getClusters());
-                assertEquals(10, response.getHits().getTotalHits().value);
-                assertEquals(10, response.getHits().getHits().length);
+                SearchResponse searchResponseIndex = null;


I don't think this is the right solution here. I open #102049 to remove the high level rest client all together.

Thanks for handling this @iverase ! Reverting in favor of #102049 .

…erIT changes & using nested assertResponse HistogramPercentileAggregationTests"

pmpailis · 2023-11-13T12:16:49Z

@elasticmachine update branch

…tResponse

…com:pmpailis/elasticsearch into fix/remove-explicit-search-response-test-v2

iverase · 2023-11-13T13:46:14Z

...lugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncStatusResponse.java

-            failedShards = searchResponse.getFailedShards();
-            if (searchResponse.getClusters() != null && searchResponse.getClusters() != SearchResponse.Clusters.EMPTY) {
-                clusters = searchResponse.getClusters();
+        SearchResponse searchResponse = null;


This is production code, we should not change it in this PR as it is label as test. Let's do those changes once we have all test in place.

Yeap, you're right. Shouldn't have been included in the PR :/
Reverted.

iverase

Let's revert the changes on the production code, that should be done in a different PR. Otherwise LGTM

pmpailis · 2023-11-13T14:01:09Z

Thanks for the 👀 @iverase! (and apologies for the trouble and stupid issues!)

iverase

LGTM

pmpailis added 5 commits November 10, 2023 16:08

updating

759740e

updating

ee2c0b1

updating

2263ae9

updating

71961d3

updating

8c4bef4

pmpailis requested review from original-brownbear and piergm November 10, 2023 14:48

pmpailis removed the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 10, 2023

Merge remote-tracking branch 'origin/main' into fix/remove-explicit-s…

bf7363c

…earch-response-test-v2

original-brownbear mentioned this pull request Nov 10, 2023

[Meta] Make SearchResponse use pooled buffers for search hits #102030

Closed

78 tasks

updating

358b450

elasticmachine and others added 3 commits November 10, 2023 23:07

Merge branch 'main' into fix/remove-explicit-search-response-test-v2

f6a4365

updating

f8f0aa0

Merge branch 'fix/remove-explicit-search-response-test-v2' of github.…

51d1b5f

…com:pmpailis/elasticsearch into fix/remove-explicit-search-response-test-v2

iverase reviewed Nov 12, 2023

View reviewed changes

addressing PR comments - reverting CrossClusterSearchUnavailableClust…

b9b1e8c

…erIT changes & using nested assertResponse HistogramPercentileAggregationTests"

pmpailis added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 13, 2023

elasticmachine and others added 3 commits November 13, 2023 13:16

Merge branch 'main' into fix/remove-explicit-search-response-test-v2

ecd3eb7

addressing PR comments - removing try/finally block in favor of asser…

50a4342

…tResponse

Merge branch 'fix/remove-explicit-search-response-test-v2' of github.…

eced0ac

…com:pmpailis/elasticsearch into fix/remove-explicit-search-response-test-v2

pmpailis removed the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 13, 2023

iverase reviewed Nov 13, 2023

View reviewed changes

iverase requested changes Nov 13, 2023

View reviewed changes

reverting changes

18a307e

iverase approved these changes Nov 13, 2023

View reviewed changes

pmpailis merged commit 25b80ac into elastic:main Nov 13, 2023
13 checks passed

pmpailis deleted the fix/remove-explicit-search-response-test-v2 branch February 20, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing explicit SearchResponse usages in tests - v2 #102021

Removing explicit SearchResponse usages in tests - v2 #102021

pmpailis commented Nov 10, 2023 •

edited

Loading

elasticsearchmachine commented Nov 10, 2023

JVerwolf commented Nov 10, 2023

iverase Nov 12, 2023

pmpailis Nov 13, 2023

iverase Nov 13, 2023

pmpailis Nov 13, 2023

pmpailis Nov 13, 2023

iverase Nov 13, 2023

pmpailis Nov 13, 2023

iverase Nov 12, 2023

pmpailis Nov 13, 2023

iverase Nov 13, 2023

iverase Nov 12, 2023

pmpailis Nov 13, 2023

pmpailis commented Nov 13, 2023

iverase Nov 13, 2023

pmpailis Nov 13, 2023

iverase left a comment

pmpailis commented Nov 13, 2023

iverase left a comment

Removing explicit SearchResponse usages in tests - v2 #102021

Removing explicit SearchResponse usages in tests - v2 #102021

Conversation

pmpailis commented Nov 10, 2023 • edited Loading

elasticsearchmachine commented Nov 10, 2023

JVerwolf commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmpailis commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase left a comment

Choose a reason for hiding this comment

pmpailis commented Nov 13, 2023

iverase left a comment

Choose a reason for hiding this comment

pmpailis commented Nov 10, 2023 •

edited

Loading