Removing explicit SearchResponse usages in tests #102008

pmpailis · 2023-11-10T12:29:17Z

Tests covered in this PR:

org.elasticsearch.xpack.enrich.EnrichPolicyRunnerTests
org.elasticsearch.index.engine.frozen.FrozenIndexIT
org.elasticsearch.index.engine.frozen.FrozenIndexTests

…earch-response-test

original-brownbear

Lets fix the assertNoFailures blocks here and back out that one tricky change, otherwise this is perfect, thanks!

original-brownbear · 2023-11-10T12:41:01Z

...ndices/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexIT.java

-                SearchResponse resp = prepareSearch().setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)).get();
-                assertNoFailures(resp);
-                assertHitCount(resp, 0);
+                assertResponse(prepareSearch().setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)), searchResponse -> {


This case can use assertNoFailuresAndResponse to save us the assertNoFailures(searchResponse);, lets do that in all the spots we have here to keep the growing code size from this effort in check (at least a little)

original-brownbear · 2023-11-10T12:42:54Z

...ces/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexTests.java

                }
+                searchResponse = client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueMinutes(1)).get();


This isn't correct is it? Aren't we overwriting values for searchResponse without releasing them here? Either way, maybe leave this one out from this changeset, we can deal with it later on, it's a bit of a non-standard case?

Yeah, you're right. I've slightly updated this if you want to take a look, otherwise we can skip and revisit later on as suggested :)

elasticsearchmachine · 2023-11-10T14:13:13Z

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

…earch-response-test

original-brownbear

Two tiny nits and I think I still may prefer not doing the bigger change at the bottom, this looks super verbose, the test was very hard to understand before and is even harder to read now. Maybe just back that out and not mess with it right now?
We just need to improve tooling a little her eI think :)

original-brownbear · 2023-11-10T16:34:25Z

...ndices/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexIT.java

-                assertHitCount(resp, 0);
+                assertNoFailuresAndResponse(
+                    prepareSearch().setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)),
+                    searchResponse -> {


NIT: No need for the curly braces here :)

original-brownbear · 2023-11-10T16:34:37Z

...ndices/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexIT.java

-                assertHitCount(resp, numDocs);
+                assertNoFailuresAndResponse(
+                    prepareSearch().setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)),
+                    searchResponse -> {


NIT No need for curly braces

original-brownbear · 2023-11-10T16:36:44Z

...ces/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexTests.java

-                    shard = indexService.getShard(i);
-                    engine = IndexShardTestCase.getEngine(shard);
-                    assertFalse(((FrozenEngine) engine).isReaderOpen());
+            scrollResponse = client().prepareSearch()


If we actually want to adjust this one here, why don't we start from the standard assertResponse approach here. why use a tricky try-finally and deviatee from our usual approach?

pmpailis · 2023-11-10T18:06:22Z

Two tiny nits and I think I still may prefer not doing the bigger change at the bottom, this looks super verbose, the test was very hard to understand before and is even harder to read now. Maybe just back that out and not mess with it right now? We just need to improve tooling a little her eI think :)

Yeah makes sense. I wasn't too keen either in how it turned out. Reverted and we can go back at a later time.

benwtrent · 2023-11-10T18:28:53Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

-            ).actionGet();
-            assertEquals(0L, routingSearchResponse.getHits().getTotalHits().value);
+            final int targetIdx = idx;
+            assertResponse(


assertHitCount, this is only checking the hits count.

benwtrent · 2023-11-10T18:29:32Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

+                    assertThat(sourceDocMap.get("field5"), is(equalTo("value5")));
+                }
+            );
+            assertResponse(


assertHitCount only checking hits count

benwtrent · 2023-11-10T18:30:15Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

+        assertResponse(
+            client().search(
+                new SearchRequest(".enrich-test1").source(SearchSourceBuilder.searchSource().query(QueryBuilders.matchAllQuery()))
+            ),
+            allEnrichDocs -> {
+                assertThat(allEnrichDocs.getHits().getTotalHits().value, equalTo(2L));
+            }
+        );


assertHitCount only checking hits count

benwtrent · 2023-11-10T18:30:40Z

x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyRunnerTests.java

    }

-    public void testEnrichObjectField() {
+    public void testEnrichObjectField() throws ExecutionException, InterruptedException {


its a test, so it could just be throws Exception

benwtrent · 2023-11-10T18:31:59Z

...ndices/src/internalClusterTest/java/org/elasticsearch/index/engine/frozen/FrozenIndexIT.java

+                assertNoFailuresAndResponse(
+                    prepareSearch().setPreference(null).setPointInTime(new PointInTimeBuilder(pitId)),
+                    searchResponse -> assertHitCount(searchResponse, 0)
+                );


assertHitCount only checking hits count. If we need to double check for no failures, we should have a asserNoFailuresAndHitCount

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

@elasticmachine update branch

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

pmpailis · 2023-11-13T12:17:08Z

@elasticmachine update branch

Tests covered in this PR: * `org.elasticsearch.xpack.enrich.EnrichPolicyRunnerTests` * `org.elasticsearch.index.engine.frozen.FrozenIndexIT` * `org.elasticsearch.index.engine.frozen.FrozenIndexTests`

original-brownbear

LGTM

removing explicit SearchResponse usages

b602607

pmpailis requested review from original-brownbear and piergm November 10, 2023 12:29

elasticsearchmachine added needs:triage Requires assignment of a team area label v8.12.0 labels Nov 10, 2023

pmpailis added 3 commits November 10, 2023 14:32

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

02a4062

…earch-response-test

removing todo

ee916e7

spotless

acfbbc5

original-brownbear reviewed Nov 10, 2023

View reviewed changes

pmpailis added 2 commits November 10, 2023 16:54

updating

37dae8c

updating

215dd96

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

pmpailis added 2 commits November 10, 2023 17:13

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

6829f4a

…earch-response-test

updating

0282053

original-brownbear reviewed Nov 10, 2023

View reviewed changes

original-brownbear mentioned this pull request Nov 10, 2023

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

Closed

78 tasks

updating

f987579

benwtrent reviewed Nov 10, 2023

View reviewed changes

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

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

76cbf95

addressing PR comments

e31cf81

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

1d11fd5

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

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

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

e406f0e

elasticsearchmachine merged commit ebbcb71 into elastic:main Nov 13, 2023
13 checks passed

pmpailis deleted the fix/remove-explicit-search-response-test branch November 13, 2023 12:53

original-brownbear reviewed Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing explicit SearchResponse usages in tests #102008

Removing explicit SearchResponse usages in tests #102008

pmpailis commented Nov 10, 2023 •

edited

Loading

original-brownbear left a comment

original-brownbear Nov 10, 2023

original-brownbear Nov 10, 2023

pmpailis Nov 10, 2023

elasticsearchmachine commented Nov 10, 2023

original-brownbear left a comment

original-brownbear Nov 10, 2023

original-brownbear Nov 10, 2023

original-brownbear Nov 10, 2023

pmpailis commented Nov 10, 2023

benwtrent Nov 10, 2023

benwtrent Nov 10, 2023

benwtrent Nov 10, 2023

benwtrent Nov 10, 2023

benwtrent Nov 10, 2023

JVerwolf commented Nov 10, 2023

pmpailis commented Nov 13, 2023

original-brownbear left a comment

		}
		searchResponse = client().prepareSearchScroll(searchResponse.getScrollId()).setScroll(TimeValue.timeValueMinutes(1)).get();

Removing explicit SearchResponse usages in tests #102008

Removing explicit SearchResponse usages in tests #102008

Conversation

pmpailis commented Nov 10, 2023 • edited Loading

original-brownbear left a 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

elasticsearchmachine commented Nov 10, 2023

original-brownbear left a 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 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

JVerwolf commented Nov 10, 2023

pmpailis commented Nov 13, 2023

original-brownbear left a comment

Choose a reason for hiding this comment

pmpailis commented Nov 10, 2023 •

edited

Loading