From 9a2be28a404cb709374649f18bbab686b5c8c1ab Mon Sep 17 00:00:00 2001
From: Chen Dai <46505291+dai-chen@users.noreply.github.com>
Date: Thu, 10 Dec 2020 13:03:37 -0800
Subject: [PATCH] Enable failed test logging and fix flaky UT (#910)

* Fix UT and add logging in gradle

* Fix UT and add logging in gradle

* Simplify test logging config
---
 core/build.gradle                             |  1 +
 elasticsearch/build.gradle                    |  1 +
 legacy/build.gradle                           |  5 +++++
 ppl/build.gradle                              |  7 +++++++
 protocol/build.gradle                         |  1 +
 sql/build.gradle                              |  1 +
 .../parser/context/QuerySpecification.java    |  4 ++--
 .../sql/sql/parser/AstBuilderTest.java        | 21 +++++++++++++++----
 8 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/core/build.gradle b/core/build.gradle
index 3db5ed1e7b..45f484c2dc 100644
--- a/core/build.gradle
+++ b/core/build.gradle
@@ -34,6 +34,7 @@ test {
     useJUnitPlatform()
     testLogging {
         events "passed", "skipped", "failed"
+        exceptionFormat "full"
     }
 }
 
diff --git a/elasticsearch/build.gradle b/elasticsearch/build.gradle
index 28bdec498a..00bfdfbfe4 100644
--- a/elasticsearch/build.gradle
+++ b/elasticsearch/build.gradle
@@ -28,6 +28,7 @@ test {
     useJUnitPlatform()
     testLogging {
         events "passed", "skipped", "failed"
+        exceptionFormat "full"
     }
 }
 
diff --git a/legacy/build.gradle b/legacy/build.gradle
index 5e43ac29c1..58da5da6cd 100644
--- a/legacy/build.gradle
+++ b/legacy/build.gradle
@@ -53,6 +53,11 @@ test {
     // set 'project.projectDir' property to allow unit test classes to access test resources
     // in src/test/resources in current module
     systemProperty('project.root', project.projectDir.absolutePath)
+
+    testLogging {
+        events "passed", "skipped", "failed"
+        exceptionFormat "full"
+    }
 }
 
 dependencies {
diff --git a/ppl/build.gradle b/ppl/build.gradle
index 8fd6ee1ebc..69a27264a5 100644
--- a/ppl/build.gradle
+++ b/ppl/build.gradle
@@ -42,6 +42,13 @@ dependencies {
 
 }
 
+test {
+    testLogging {
+        events "passed", "skipped", "failed"
+        exceptionFormat "full"
+    }
+}
+
 jacoco {
     toolVersion = "0.8.5"
 }
diff --git a/protocol/build.gradle b/protocol/build.gradle
index 3bb6b58ff1..fb311bcc4b 100644
--- a/protocol/build.gradle
+++ b/protocol/build.gradle
@@ -27,6 +27,7 @@ test {
     useJUnitPlatform()
     testLogging {
         events "passed", "skipped", "failed"
+        exceptionFormat "full"
     }
 }
 
diff --git a/sql/build.gradle b/sql/build.gradle
index f0e923cdad..d57f050fef 100644
--- a/sql/build.gradle
+++ b/sql/build.gradle
@@ -44,6 +44,7 @@ test {
     useJUnitPlatform()
     testLogging {
         events "passed", "skipped", "failed"
+        exceptionFormat "full"
     }
 }
 
diff --git a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/context/QuerySpecification.java b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/context/QuerySpecification.java
index 6927e2b712..1124f0150a 100644
--- a/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/context/QuerySpecification.java
+++ b/sql/src/main/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/context/QuerySpecification.java
@@ -40,7 +40,7 @@
 import com.amazon.opendistroforelasticsearch.sql.sql.parser.AstExpressionBuilder;
 import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -82,7 +82,7 @@ public class QuerySpecification {
    * Aggregate function calls that spreads in SELECT, HAVING clause. Since this is going to be
    * pushed to aggregation operator, de-duplicate is necessary to avoid duplication.
    */
-  private final Set<UnresolvedExpression> aggregators = new HashSet<>();
+  private final Set<UnresolvedExpression> aggregators = new LinkedHashSet<>();
 
   /**
    * Items in GROUP BY clause that may be:
diff --git a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java
index 86cbcf6e71..da92cce889 100644
--- a/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java
+++ b/sql/src/test/java/com/amazon/opendistroforelasticsearch/sql/sql/parser/AstBuilderTest.java
@@ -197,20 +197,33 @@ public void can_build_where_clause() {
   }
 
   @Test
-  public void can_build_count_star_and_count_literal() {
+  public void can_build_count_literal() {
     assertEquals(
         project(
             agg(
                 relation("test"),
                 ImmutableList.of(
-                    alias("COUNT(*)", aggregate("COUNT", AllFields.of())),
                     alias("COUNT(1)", aggregate("COUNT", intLiteral(1)))),
                 emptyList(),
                 emptyList(),
                 emptyList()),
-            alias("COUNT(*)", aggregate("COUNT", AllFields.of())),
             alias("COUNT(1)", aggregate("COUNT", intLiteral(1)))),
-        buildAST("SELECT COUNT(*), COUNT(1) FROM test"));
+        buildAST("SELECT COUNT(1) FROM test"));
+  }
+
+  @Test
+  public void can_build_count_star() {
+    assertEquals(
+        project(
+            agg(
+                relation("test"),
+                ImmutableList.of(
+                    alias("COUNT(*)", aggregate("COUNT", AllFields.of()))),
+                emptyList(),
+                emptyList(),
+                emptyList()),
+            alias("COUNT(*)", aggregate("COUNT", AllFields.of()))),
+        buildAST("SELECT COUNT(*) FROM test"));
   }
 
   @Test