Aggs: Unmute and fix CCSDuelIT flaky tests (elastic#138375)

Fixes elastic#132879
Fixed elastic#132880

Ignore `doc_count_error_upper_bound` in response comparison for terms aggs in CCSDuelIT, and unmute tests.

It was failing sometimes with a: `doc_count_error_upper_bound: expected Integer [5] but was Integer [2]`

Given it's an estimation (From https://www.elastic.co/docs/reference/aggregations/search-aggregations-bucket-terms-aggregation), ignoring should be safe for the test purposes.

Author: ivancea

muted-tests.yml

@@ -252,12 +252,6 @@ tests:
 - class: org.elasticsearch.xpack.eql.planner.QueryTranslatorTests
   method: testMatchOptimization
   issue: https://github.com/elastic/elasticsearch/issues/132894
-- class: org.elasticsearch.search.CCSDuelIT
-  method: testTermsAggs
-  issue: https://github.com/elastic/elasticsearch/issues/132879
-- class: org.elasticsearch.search.CCSDuelIT
-  method: testTermsAggsWithProfile
-  issue: https://github.com/elastic/elasticsearch/issues/132880
 - class: org.elasticsearch.xpack.security.authc.AuthenticationServiceTests
   method: testInvalidToken
   issue: https://github.com/elastic/elasticsearch/issues/133328

qa/multi-cluster-search/src/test/java/org/elasticsearch/search/CCSDuelIT.java

@@ -96,6 +96,7 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Predicate;
 
 import static org.hamcrest.CoreMatchers.anyOf;
 import static org.hamcrest.CoreMatchers.equalTo;
@@ -675,12 +676,12 @@ public void testTermsAggs() throws Exception {
         {
             SearchRequest searchRequest = initLocalAndRemoteSearchRequest();
             searchRequest.source(buildTermsAggsSource());
-            duelRequest(searchRequest, CCSDuelIT::assertAggs);
+            duelRequest(searchRequest, CCSDuelIT::assertAggs, true, path -> path.endsWith("/doc_count_error_upper_bound") == false);
         }
         {
             SearchRequest searchRequest = initRemoteOnlySearchRequest();
             searchRequest.source(buildTermsAggsSource());
-            duelRequest(searchRequest, CCSDuelIT::assertAggs);
+            duelRequest(searchRequest, CCSDuelIT::assertAggs, true, path -> path.endsWith("/doc_count_error_upper_bound") == false);
         }
     }
 
@@ -689,12 +690,12 @@ public void testTermsAggsWithProfile() throws Exception {
         {
             SearchRequest searchRequest = initLocalAndRemoteSearchRequest();
             searchRequest.source(buildTermsAggsSource().profile(true));
-            duelRequest(searchRequest, CCSDuelIT::assertAggs);
+            duelRequest(searchRequest, CCSDuelIT::assertAggs, true, path -> path.endsWith("/doc_count_error_upper_bound") == false);
         }
         {
             SearchRequest searchRequest = initRemoteOnlySearchRequest();
             searchRequest.source(buildTermsAggsSource().profile(true));
-            duelRequest(searchRequest, CCSDuelIT::assertAggs);
+            duelRequest(searchRequest, CCSDuelIT::assertAggs, true, path -> path.endsWith("/doc_count_error_upper_bound") == false);
         }
     }
 
@@ -875,7 +876,7 @@ public void testShardFailures() throws Exception {
             assertNull(response.evaluate("suggest"));
             assertThat(response.evaluateArraySize("hits.hits"), greaterThan(0));
             assertThat(response.evaluate("_shards.failed"), greaterThanOrEqualTo(2));
-        }, compareAsyncAndSyncResponses);
+        }, compareAsyncAndSyncResponses, path -> true);
     }
 
     public void testTermSuggester() throws Exception {
@@ -984,18 +985,19 @@ private static SearchRequest initRemoteOnlySearchRequest() {
     }
 
     private void duelRequest(SearchRequest searchRequest, CheckedConsumer<ObjectPath, IOException> responseChecker) throws Exception {
-        duelRequest(searchRequest, responseChecker, true);
+        duelRequest(searchRequest, responseChecker, true, p -> true);
     }
 
     private void duelRequest(
         SearchRequest searchRequest,
         CheckedConsumer<ObjectPath, IOException> responseChecker,
-        boolean compareAsyncToSyncResponses
+        boolean compareAsyncToSyncResponses,
+        Predicate<String> pathFilter
     ) throws Exception {
-        Map<String, Object> syncResponseMap = duelSearchSync(searchRequest, responseChecker);
-        Map<String, Object> asyncResponseMap = duelSearchAsync(searchRequest, responseChecker);
+        Map<String, Object> syncResponseMap = duelSearchSync(searchRequest, responseChecker, pathFilter);
+        Map<String, Object> asyncResponseMap = duelSearchAsync(searchRequest, responseChecker, pathFilter);
         if (compareAsyncToSyncResponses) {
-            compareResponseMaps(syncResponseMap, asyncResponseMap, "Comparing sync_search CCS vs. async_search CCS");
+            compareResponseMaps(syncResponseMap, asyncResponseMap, "Comparing sync_search CCS vs. async_search CCS", pathFilter);
         }
     }
 
@@ -1004,11 +1006,24 @@ private void duelRequest(
      */
     private static Map<String, Object> duelSearchSync(SearchRequest searchRequest, CheckedConsumer<ObjectPath, IOException> responseChecker)
         throws Exception {
+        return duelSearchSync(searchRequest, responseChecker, p -> true);
+    }
+
+    /**
+     * @return responseMap from one of the Synchronous Search Requests
+     */
+    private static Map<String, Object> duelSearchSync(
+        SearchRequest searchRequest,
+        CheckedConsumer<ObjectPath, IOException> responseChecker,
+        Predicate<String> pathFilter
+    ) throws Exception {
         CountDownLatch latch = new CountDownLatch(2);
+
         AtomicReference<Exception> exception1 = new AtomicReference<>();
         AtomicReference<Response> minimizeRoundtripsResponse = new AtomicReference<>();
         searchRequest.setCcsMinimizeRoundtrips(true);
         submitSyncSearch(searchRequest, minimizeRoundtripsResponse, exception1, latch);
+
         AtomicReference<Exception> exception2 = new AtomicReference<>();
         AtomicReference<Response> fanOutResponse = new AtomicReference<>();
         searchRequest.setCcsMinimizeRoundtrips(false);
@@ -1075,7 +1090,12 @@ private static Map<String, Object> duelSearchSync(SearchRequest searchRequest, C
             Map<String, Object> minimizeRoundtripsResponseMap = responseToMap(minimizeRoundtripsSearchResponse);
             if (minimizeRoundtripsSearchResponse.evaluate("_clusters") != null && fanOutSearchResponse.evaluate("_clusters") != null) {
                 Map<String, Object> fanOutResponseMap = responseToMap(fanOutSearchResponse);
-                compareResponseMaps(minimizeRoundtripsResponseMap, fanOutResponseMap, "Comparing sync_search minimizeRoundTrip vs. fanOut");
+                compareResponseMaps(
+                    minimizeRoundtripsResponseMap,
+                    fanOutResponseMap,
+                    "Comparing sync_search minimizeRoundTrip vs. fanOut",
+                    pathFilter
+                );
                 assertThat(
                     minimizeRoundtripsSearchResponse.evaluate("_shards.skipped"),
                     lessThanOrEqualTo((Integer) fanOutSearchResponse.evaluate("_shards.skipped"))
@@ -1123,6 +1143,17 @@ public void onFailure(Exception exception) {
     private static Map<String, Object> duelSearchAsync(
         SearchRequest searchRequest,
         CheckedConsumer<ObjectPath, IOException> responseChecker
+    ) throws Exception {
+        return duelSearchAsync(searchRequest, responseChecker, p -> true);
+    }
+
+    /**
+     * @return responseMap from one of the async searches
+     */
+    private static Map<String, Object> duelSearchAsync(
+        SearchRequest searchRequest,
+        CheckedConsumer<ObjectPath, IOException> responseChecker,
+        Predicate<String> pathFilter
     ) throws Exception {
         searchRequest.setCcsMinimizeRoundtrips(true);
         ObjectPath minimizeRoundtripsResponse = submitAsyncSearch(searchRequest, TimeValue.timeValueSeconds(1));
@@ -1183,7 +1214,12 @@ private static Map<String, Object> duelSearchAsync(
         Map<String, Object> minimizeRoundtripsResponseMap = responseToMap(minimizeRoundtripsResponse);
         if (minimizeRoundtripsResponse.evaluate("_clusters") != null && fanOutResponse.evaluate("_clusters") != null) {
             Map<String, Object> fanOutResponseMap = responseToMap(fanOutResponse);
-            compareResponseMaps(minimizeRoundtripsResponseMap, fanOutResponseMap, "Comparing async_search minimizeRoundTrip vs. fanOut");
+            compareResponseMaps(
+                minimizeRoundtripsResponseMap,
+                fanOutResponseMap,
+                "Comparing async_search minimizeRoundTrip vs. fanOut",
+                pathFilter
+            );
             assertThat(
                 minimizeRoundtripsResponse.evaluate("_shards.skipped"),
                 lessThanOrEqualTo((Integer) fanOutResponse.evaluate("_shards.skipped"))
@@ -1192,8 +1228,13 @@ private static Map<String, Object> duelSearchAsync(
         return minimizeRoundtripsResponseMap;
     }
 
-    private static void compareResponseMaps(Map<String, Object> responseMap1, Map<String, Object> responseMap2, String info) {
-        String diff = XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder(responseMap1, responseMap2);
+    private static void compareResponseMaps(
+        Map<String, Object> responseMap1,
+        Map<String, Object> responseMap2,
+        String info,
+        Predicate<String> pathFilter
+    ) {
+        String diff = XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder(responseMap1, responseMap2, pathFilter);
         if (diff != null) {
             NotEqualMessageBuilder builder = new NotEqualMessageBuilder();
             builder.compareMaps(responseMap1, responseMap2);

test/framework/src/main/java/org/elasticsearch/test/XContentTestUtils.java

@@ -82,16 +82,39 @@ public static BytesReference convertToXContent(Map<String, ?> map, XContentType
      * @return null if maps are equal or path to the element where the difference was found
      */
     public static String differenceBetweenMapsIgnoringArrayOrder(Map<String, Object> first, Map<String, Object> second) {
-        return differenceBetweenMapsIgnoringArrayOrder("", first, second);
+        return differenceBetweenMapsIgnoringArrayOrder("", first, second, p -> true);
     }
 
-    private static String differenceBetweenMapsIgnoringArrayOrder(String path, Map<String, Object> first, Map<String, Object> second) {
+    /**
+     * Compares two maps generated from XContentObjects. The order of elements in arrays is ignored.
+     *
+     * @param pathFilter Predicate to filter a path and its children. True if the path should be checked, false to exclude it.
+     * @return null if maps are equal or path to the element where the difference was found
+     */
+    public static String differenceBetweenMapsIgnoringArrayOrder(
+        Map<String, Object> first,
+        Map<String, Object> second,
+        Predicate<String> pathFilter
+    ) {
+        return differenceBetweenMapsIgnoringArrayOrder("", first, second, pathFilter);
+    }
+
+    private static String differenceBetweenMapsIgnoringArrayOrder(
+        String path,
+        Map<String, Object> first,
+        Map<String, Object> second,
+        Predicate<String> pathFilter
+    ) {
+        if (pathFilter.test(path) == false) {
+            return null;
+        }
+
         if (first.size() != second.size()) {
             return path + ": sizes of the maps don't match: " + first.size() + " != " + second.size();
         }
 
         for (String key : first.keySet()) {
-            String reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/" + key, first.get(key), second.get(key));
+            String reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/" + key, first.get(key), second.get(key), pathFilter);
             if (reason != null) {
                 return reason;
             }
@@ -100,7 +123,16 @@ private static String differenceBetweenMapsIgnoringArrayOrder(String path, Map<S
     }
 
     @SuppressWarnings("unchecked")
-    private static String differenceBetweenObjectsIgnoringArrayOrder(String path, Object first, Object second) {
+    private static String differenceBetweenObjectsIgnoringArrayOrder(
+        String path,
+        Object first,
+        Object second,
+        Predicate<String> pathFilter
+    ) {
+        if (pathFilter.test(path) == false) {
+            return null;
+        }
+
         if (first == null) {
             if (second == null) {
                 return null;
@@ -116,7 +148,7 @@ private static String differenceBetweenObjectsIgnoringArrayOrder(String path, Ob
                     for (Object firstObj : firstList) {
                         boolean found = false;
                         for (Object secondObj : secondList) {
-                            reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/*", firstObj, secondObj);
+                            reason = differenceBetweenObjectsIgnoringArrayOrder(path + "/*", firstObj, secondObj, pathFilter);
                             if (reason == null) {
                                 secondList.remove(secondObj);
                                 found = true;
@@ -140,7 +172,7 @@ private static String differenceBetweenObjectsIgnoringArrayOrder(String path, Ob
             }
         } else if (first instanceof Map) {
             if (second instanceof Map) {
-                return differenceBetweenMapsIgnoringArrayOrder(path, (Map<String, Object>) first, (Map<String, Object>) second);
+                return differenceBetweenMapsIgnoringArrayOrder(path, (Map<String, Object>) first, (Map<String, Object>) second, pathFilter);
             } else {
                 return path + ": the second element is not a map (got " + second + ")";
             }

test/framework/src/test/java/org/elasticsearch/test/XContentTestUtilsTests.java

@@ -27,10 +27,12 @@
 import java.util.Map;
 import java.util.function.Predicate;
 
+import static org.elasticsearch.test.XContentTestUtils.differenceBetweenMapsIgnoringArrayOrder;
 import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.nullValue;
 
 public class XContentTestUtilsTests extends ESTestCase {
 
@@ -222,4 +224,56 @@ public void testInsertRandomXContent() throws IOException {
         assertEquals(1, foo4List.size());
         assertEquals(2, ((Map<String, Object>) foo4List.get(0)).keySet().size());
     }
+
+    public void testDifferenceBetweenMapsIgnoringArrayOrder() {
+        var map1 = Map.of("foo", List.of(1, 2, 3), "bar", Map.of("a", 2, "b", List.of(3, 2, 1)), "baz", List.of(3, 2, 1, "test"));
+        var map2 = Map.of("foo", List.of(3, 2, 1), "bar", Map.of("b", List.of(3, 1, 2), "a", 2), "baz", List.of(1, "test", 2, 3));
+
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2), nullValue());
+    }
+
+    public void testDifferenceBetweenMapsIgnoringArrayOrder_WithFilter_Object() {
+        var map1 = Map.of("foo", List.of(1, 2, 3), "bar", Map.of("a", 2, "b", List.of(3, 2, 1)), "different", Map.of("a", 1, "x", 8));
+        var map2 = Map.of(
+            "foo",
+            List.of(3, 2, 1),
+            "bar",
+            Map.of("b", List.of(3, 1, 2), "a", 2),
+            "different",
+            Map.of("a", 1, "x", "different value")
+        );
+
+        assertThat(
+            differenceBetweenMapsIgnoringArrayOrder(map1, map2),
+            equalTo("/different/x: the elements don't match: [8] != [different value]")
+        );
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.equals("/different/x") == false), nullValue());
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.equals("/different") == false), nullValue());
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.isEmpty() == false), nullValue());
+    }
+
+    public void testDifferenceBetweenMapsIgnoringArrayOrder_WithFilter_Array() {
+        var map1 = Map.of(
+            "foo",
+            List.of(1, 2, 3),
+            "bar",
+            Map.of("a", 2, "b", List.of(3, 2, 1)),
+            "different",
+            List.of(3, Map.of("x", 10), 1)
+        );
+        var map2 = Map.of(
+            "foo",
+            List.of(3, 2, 1),
+            "bar",
+            Map.of("b", List.of(3, 1, 2), "a", 2),
+            "different",
+            List.of(3, Map.of("x", 5), 1)
+        );
+
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2), equalTo("/different/*: the second element is not a map (got 1)"));
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.equals("/different/*/x") == false), nullValue());
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.equals("/different/*") == false), nullValue());
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.equals("/different") == false), nullValue());
+        assertThat(differenceBetweenMapsIgnoringArrayOrder(map1, map2, path -> path.isEmpty() == false), nullValue());
+    }
 }