Painless: more testing for script_stack

modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java

@@ -43,7 +43,8 @@ public final class CompilerSettings {
     public static final String PICKY = "picky";
 
     /**
-     * For testing: do not use.
+     * Hack to set the initial "depth" for the {@link DefBootstrap.PIC} and {@link DefBootstrap.MIC}. Only used for testing: do not
+     * overwrite.
      */
     public static final String INITIAL_CALL_SITE_DEPTH = "initialCallSiteDepth";
 

modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java

@@ -186,7 +186,7 @@ public void testNullSafeDeref() {
         assertNull(         exec("def    a = null;  return a?.toString()"));
         assertEquals("foo", exec("def    a = 'foo'; return a?.toString()"));
         //   Call with primitive result
-        assertMustBeNullable(    "String a = null; return a?.length()");
+        assertMustBeNullable(    "String a = null;  return a?.length()");
         assertMustBeNullable(    "String a = 'foo'; return a?.length()");
         assertNull(         exec("def    a = null;  return a?.length()"));
         assertEquals(3,     exec("def    a = 'foo'; return a?.length()"));
@@ -265,7 +265,7 @@ public void testNullSafeDeref() {
     }
 
     private void assertMustBeNullable(String script) {
-        Exception e = expectScriptThrows(IllegalArgumentException.class , () -> exec(script));
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () -> exec(script));
         assertEquals("Result of null safe operator must be nullable", e.getMessage());
     }
 }

modules/lang-painless/src/test/java/org/elasticsearch/painless/ImplementInterfacesTests.java

@@ -325,7 +325,7 @@ public interface NoArgumentsConstant {
         Object execute(String foo);
     }
     public void testNoArgumentsConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(NoArgumentsConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("Painless needs a constant [String[] ARGUMENTS] on all interfaces it implements with the "
                 + "names of the method arguments but [" + NoArgumentsConstant.class.getName() + "] doesn't have one."));
@@ -336,7 +336,7 @@ public interface WrongArgumentsConstant {
         Object execute(String foo);
     }
     public void testWrongArgumentsConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(WrongArgumentsConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("Painless needs a constant [String[] ARGUMENTS] on all interfaces it implements with the "
                 + "names of the method arguments but [" + WrongArgumentsConstant.class.getName() + "] doesn't have one."));
@@ -347,7 +347,7 @@ public interface WrongLengthOfArgumentConstant {
         Object execute(String foo);
     }
     public void testWrongLengthOfArgumentConstant() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(WrongLengthOfArgumentConstant.class, null, "1", emptyMap()));
         assertThat(e.getMessage(), startsWith("[" + WrongLengthOfArgumentConstant.class.getName() + "#ARGUMENTS] has length [2] but ["
                 + WrongLengthOfArgumentConstant.class.getName() + "#execute] takes [1] argument."));
@@ -358,7 +358,7 @@ public interface UnknownArgType {
         Object execute(UnknownArgType foo);
     }
     public void testUnknownArgType() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(UnknownArgType.class, null, "1", emptyMap()));
         assertEquals("[foo] is of unknown type [" + UnknownArgType.class.getName() + ". Painless interfaces can only accept arguments "
                 + "that are of whitelisted types.", e.getMessage());
@@ -369,7 +369,7 @@ public interface UnknownReturnType {
         UnknownReturnType execute(String foo);
     }
     public void testUnknownReturnType() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(UnknownReturnType.class, null, "1", emptyMap()));
         assertEquals("Painless can only implement execute methods returning a whitelisted type but [" + UnknownReturnType.class.getName()
                 + "#execute] returns [" + UnknownReturnType.class.getName() + "] which isn't whitelisted.", e.getMessage());
@@ -380,7 +380,7 @@ public interface UnknownArgTypeInArray {
         Object execute(UnknownArgTypeInArray[] foo);
     }
     public void testUnknownArgTypeInArray() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(UnknownArgTypeInArray.class, null, "1", emptyMap()));
         assertEquals("[foo] is of unknown type [" + UnknownArgTypeInArray.class.getName() + ". Painless interfaces can only accept "
                 + "arguments that are of whitelisted types.", e.getMessage());
@@ -391,7 +391,7 @@ public interface TwoExecuteMethods {
         Object execute(boolean foo);
     }
     public void testTwoExecuteMethods() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(TwoExecuteMethods.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement interfaces that have a single method named [execute] but ["
                 + TwoExecuteMethods.class.getName() + "] has more than one.", e.getMessage());
@@ -401,7 +401,7 @@ public interface BadMethod {
         Object something();
     }
     public void testBadMethod() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadMethod.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement methods named [execute] and [uses$argName] but [" + BadMethod.class.getName()
                 + "] contains a method named [something]", e.getMessage());
@@ -413,7 +413,7 @@ public interface BadUsesReturn {
         Object uses$foo();
     }
     public void testBadUsesReturn() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesReturn.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement uses$ methods that return boolean but [" + BadUsesReturn.class.getName()
                 + "#uses$foo] returns [java.lang.Object].", e.getMessage());
@@ -425,7 +425,7 @@ public interface BadUsesParameter {
         boolean uses$bar(boolean foo);
     }
     public void testBadUsesParameter() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesParameter.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement uses$ methods that do not take parameters but [" + BadUsesParameter.class.getName()
                 + "#uses$bar] does.", e.getMessage());
@@ -437,7 +437,7 @@ public interface BadUsesName {
         boolean uses$baz();
     }
     public void testBadUsesName() {
-        Exception e = expectScriptThrows(IllegalArgumentException.class, () ->
+        Exception e = expectScriptThrows(IllegalArgumentException.class, false, () ->
             scriptEngine.compile(BadUsesName.class, null, "null", emptyMap()));
         assertEquals("Painless can only implement uses$ methods that match a parameter name but [" + BadUsesName.class.getName()
                 + "#uses$baz] doesn't match any of [foo, bar].", e.getMessage());

modules/lang-painless/src/test/java/org/elasticsearch/painless/LambdaTests.java

@@ -204,7 +204,7 @@ public void testNestedCaptureParams() {
 
     public void testWrongArity() {
         assumeFalse("JDK is JDK 9", Constants.JRE_IS_MINIMUM_JAVA9);
-        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
+        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> {
             exec("Optional.empty().orElseGet(x -> x);");
         });
         assertTrue(expected.getMessage().contains("Incorrect number of parameters"));
@@ -220,7 +220,7 @@ public void testWrongArityDef() {
 
     public void testWrongArityNotEnough() {
         assumeFalse("JDK is JDK 9", Constants.JRE_IS_MINIMUM_JAVA9);
-        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> {
+        IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> {
             exec("List l = new ArrayList(); l.add(1); l.add(1); "
                + "return l.stream().mapToInt(() -> 5).sum();");
         });

modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java

@@ -26,10 +26,8 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.regex.Pattern;
-import java.util.regex.PatternSyntaxException;
 
 import static java.util.Collections.singletonMap;
-import static org.hamcrest.Matchers.containsString;
 
 public class RegexTests extends ScriptTestCase {
     @Override
@@ -264,8 +262,9 @@ public void testBadRegexPattern() {
         assertEquals("Error compiling regex: Illegal Unicode escape sequence", e.getCause().getMessage());
 
         // And make sure the location of the error points to the offset inside the pattern
-        assertEquals("/\\ujjjj/", e.getScriptStack().get(0));
-        assertEquals("   ^---- HERE", e.getScriptStack().get(1));
+        assertScriptStack(e,
+                "/\\ujjjj/",
+                "   ^---- HERE");
     }
 
     public void testRegexAgainstNumber() {

modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java

@@ -35,6 +35,8 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.hamcrest.Matchers.hasSize;
+
 /**
  * Base test case for scripting unit tests.
  * <p>
@@ -114,10 +116,26 @@ public void assertBytecodeHasPattern(String script, String pattern) {
 
     /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */
     public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType, ThrowingRunnable runnable) {
+        return expectScriptThrows(expectedType, true, runnable);
+    }
+
+    /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */
+    public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType, boolean shouldHaveScriptStack,
+            ThrowingRunnable runnable) {
         try {
             runnable.run();
         } catch (Throwable e) {
             if (e instanceof ScriptException) {
+                boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty();
+                if (shouldHaveScriptStack && hasEmptyScriptStack) {
+                    AssertionFailedError assertion = new AssertionFailedError("ScriptException should have a scriptStack");
+                    assertion.initCause(e);
+                    throw assertion;
+                } else if (false == shouldHaveScriptStack && false == hasEmptyScriptStack) {
+                    AssertionFailedError assertion = new AssertionFailedError("ScriptException shouldn't have a scriptStack");
+                    assertion.initCause(e);
+                    throw assertion;
+                }
                 e = e.getCause();
                 if (expectedType.isInstance(e)) {
                     return expectedType.cast(e);
@@ -134,4 +152,21 @@ public static <T extends Throwable> T expectScriptThrows(Class<T> expectedType,
         }
         throw new AssertionFailedError("Expected exception " + expectedType.getSimpleName());
     }
+
+    /**
+     * Asserts that the script_stack looks right.
+     */
+    public static void assertScriptStack(ScriptException e, String... stack) {
+        // This particular incantation of assertions makes the error messages more useful
+        try {
+            assertThat(e.getScriptStack(), hasSize(stack.length));
+            for (int i = 0; i < stack.length; i++) {
+                assertEquals(stack[i], e.getScriptStack().get(i));
+            }
+        } catch (AssertionError assertion) {
+            assertion.initCause(e);
+            throw assertion;
+        }
+    }
+
 }

modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java

@@ -165,12 +165,12 @@ public void testStringAndCharacter() {
         assertEquals('c', exec("String s = \"c\"; (char)s"));
         assertEquals('c', exec("String s = 'c'; (char)s"));
 
-        ClassCastException expected = expectScriptThrows(ClassCastException.class, () -> {
+        ClassCastException expected = expectScriptThrows(ClassCastException.class, false, () -> {
             assertEquals("cc", exec("return (String)(char)\"cc\""));
         });
         assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char]."));
 
-        expected = expectScriptThrows(ClassCastException.class, () -> {
+        expected = expectScriptThrows(ClassCastException.class, false, () -> {
             assertEquals("cc", exec("return (String)(char)'cc'"));
         });
         assertTrue(expected.getMessage().contains("Cannot cast [String] with length greater than one to [char]."));