From a52fa4f5ae5d05c8bd96a6afa8c3f3c35039f61c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 18 Apr 2017 18:19:51 -0400 Subject: [PATCH 1/5] Painless: more testing for script_stack `script_stack` is super useful when debugging Painless scripts because it skips all the "weird" stuff involved that obfuscates where the actual error is. It skips Painless's internals and call site bootstrapping. It works fine, but it didn't have many tests. This converts a test that we had for line numbers into a test for the `script_stack`. The line numbers test was an indirect test for `script_stack`. --- .../painless/CompilerSettings.java | 3 +- .../elasticsearch/painless/RegexTests.java | 7 +- .../painless/ScriptTestCase.java | 39 +++++++ .../painless/WhenThingsGoWrongTests.java | 100 +++++++++++------- 4 files changed, 104 insertions(+), 45 deletions(-) diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java index 378cca7f58fb2..e723081e36c0c 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/CompilerSettings.java +++ b/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"; diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java index 83a592b3f2632..92ff9ef3c9334 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/RegexTests.java +++ b/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() { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index 74c6c9a5628f0..ce1ef7254a5c9 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/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. *

@@ -114,10 +116,30 @@ public void assertBytecodeHasPattern(String script, String pattern) { /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */ public static T expectScriptThrows(Class expectedType, ThrowingRunnable runnable) { + return expectScriptThrows(expectedType, true, runnable); + } + + /** Checks a specific exception class is thrown (boxed inside ScriptException) and returns it. */ + public static T expectScriptThrows(Class expectedType, boolean shouldHaveScriptStack, + ThrowingRunnable runnable) { try { runnable.run(); } catch (Throwable e) { if (e instanceof ScriptException) { + boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty(); + if (shouldHaveScriptStack) { + if (hasEmptyScriptStack) { + AssertionFailedError assertion = new AssertionFailedError("ScriptException should have a scriptStack"); + assertion.initCause(e); + throw assertion; + } + } else { + if (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 +156,21 @@ public static T expectScriptThrows(Class 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 failure 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; + } + } + } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index aaa337ae821ba..b4e1cf7c2fa61 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.painless; import org.apache.lucene.util.Constants; +import org.elasticsearch.script.ScriptException; import java.lang.invoke.WrongMethodTypeException; import java.util.Arrays; @@ -27,52 +28,72 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.instanceOf; public class WhenThingsGoWrongTests extends ScriptTestCase { public void testNullPointer() { expectScriptThrows(NullPointerException.class, () -> { exec("int x = params['missing']; return x;"); }); - } - - /** test "line numbers" in the bytecode, which are really 1-based offsets */ - public void testLineNumbers() { - // trigger NPE at line 1 of the script - NullPointerException exception = expectScriptThrows(NullPointerException.class, () -> { - exec("String x = null; boolean y = x.isEmpty();\n" + - "return y;"); + expectScriptThrows(NullPointerException.class, () -> { + exec("Double.parseDouble(params['missing'])"); }); - // null deref at x.isEmpty(), the '.' is offset 30 (+1) - assertEquals(30 + 1, exception.getStackTrace()[0].getLineNumber()); + } - // trigger NPE at line 2 of the script - exception = expectScriptThrows(NullPointerException.class, () -> { - exec("String x = null;\n" + - "return x.isEmpty();"); - }); - // null deref at x.isEmpty(), the '.' is offset 25 (+1) - assertEquals(25 + 1, exception.getStackTrace()[0].getLineNumber()); - - // trigger NPE at line 3 of the script - exception = expectScriptThrows(NullPointerException.class, () -> { - exec("String x = null;\n" + - "String y = x;\n" + - "return y.isEmpty();"); - }); - // null deref at y.isEmpty(), the '.' is offset 39 (+1) - assertEquals(39 + 1, exception.getStackTrace()[0].getLineNumber()); - - // trigger NPE at line 4 in script (inside conditional) - exception = expectScriptThrows(NullPointerException.class, () -> { - exec("String x = null;\n" + - "boolean y = false;\n" + - "if (!y) {\n" + - " y = x.isEmpty();\n" + - "}\n" + - "return y;"); - }); - // null deref at x.isEmpty(), the '.' is offset 53 (+1) - assertEquals(53 + 1, exception.getStackTrace()[0].getLineNumber()); + /** + * Test that the scriptStack looks good. By implication this tests that we build proper "line numbers" in stack trace. These line + * numbers are really 1 based character numbers. + */ + public void testScriptStack() { + for (String type : new String[] { + "String", + "def "}) { + // trigger NPE at line 1 of the script + ScriptException exception = expectThrows(ScriptException.class, () -> { + exec(type + " x = null; boolean y = x.isEmpty();\n" + + "return y;"); + }); + assertScriptStack(exception, + "y = x.isEmpty();\n", + " ^---- HERE"); + assertThat(exception.getCause(), instanceOf(NullPointerException.class)); + + // trigger NPE at line 2 of the script + exception = expectThrows(ScriptException.class, () -> { + exec(type + " x = null;\n" + + "return x.isEmpty();"); + }); + assertScriptStack(exception, + "return x.isEmpty();", + " ^---- HERE"); + assertThat(exception.getCause(), instanceOf(NullPointerException.class)); + + // trigger NPE at line 3 of the script + exception = expectThrows(ScriptException.class, () -> { + exec(type + " x = null;\n" + + "String y = x;\n" + + "return y.isEmpty();"); + }); + assertScriptStack(exception, + "return y.isEmpty();", + " ^---- HERE"); + assertThat(exception.getCause(), instanceOf(NullPointerException.class)); + + // trigger NPE at line 4 in script (inside conditional) + exception = expectThrows(ScriptException.class, () -> { + exec(type + " x = null;\n" + + "boolean y = false;\n" + + "if (!y) {\n" + + " y = x.isEmpty();\n" + + "}\n" + + "return y;"); + }); + assertScriptStack(exception, + "y = x.isEmpty();\n}\n", + " ^---- HERE"); + assertThat(exception.getCause(), instanceOf(NullPointerException.class)); + } } public void testInvalidShift() { @@ -161,7 +182,7 @@ public void testSourceLimits() { final char[] tooManyChars = new char[Compiler.MAXIMUM_SOURCE_LENGTH + 1]; Arrays.fill(tooManyChars, '0'); - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { + IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> { exec(new String(tooManyChars)); }); assertTrue(expected.getMessage().contains("Scripts may be no longer than")); @@ -282,5 +303,4 @@ public void testRegularUnexpectedCharacter() { e = expectScriptThrows(IllegalArgumentException.class, () -> exec("'cat", false)); assertEquals("unexpected character ['cat].", e.getMessage()); } - } From b066ce14e6e945951cd7f7ac68b26efdddd555ea Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 18 Apr 2017 18:24:00 -0400 Subject: [PATCH 2/5] Word --- .../test/java/org/elasticsearch/painless/ScriptTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index ce1ef7254a5c9..dd5ebd989a117 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -161,7 +161,7 @@ public static T expectScriptThrows(Class expectedType, * Asserts that the script_stack looks right. */ public static void assertScriptStack(ScriptException e, String... stack) { - // This particular incantation of assertions makes the error messages failure useful + // 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++) { From 684e043220e71773deb60f70f5d7b72639bd50f2 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 18 Apr 2017 18:24:23 -0400 Subject: [PATCH 3/5] Unused import --- .../java/org/elasticsearch/painless/WhenThingsGoWrongTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index b4e1cf7c2fa61..b31c04e781aeb 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -28,7 +28,6 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; -import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; public class WhenThingsGoWrongTests extends ScriptTestCase { From 4df466f8055e5ee5884412f1e6cf1747829d69ac Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 18 Apr 2017 20:18:43 -0400 Subject: [PATCH 4/5] Mark tests that don't expect a script_stack --- .../painless/BasicExpressionTests.java | 4 +-- .../painless/ImplementInterfacesTests.java | 22 ++++++------- .../elasticsearch/painless/LambdaTests.java | 4 +-- .../painless/ScriptTestCase.java | 20 +++++------- .../elasticsearch/painless/StringTests.java | 4 +-- .../painless/WhenThingsGoWrongTests.java | 32 ++++++++++++++++--- 6 files changed, 53 insertions(+), 33 deletions(-) diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java index ef2ddad5452d0..97e1f01fdfc94 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicExpressionTests.java +++ b/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()); } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ImplementInterfacesTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ImplementInterfacesTests.java index fe95e8c8c2316..c3861add319dd 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ImplementInterfacesTests.java +++ b/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()); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/LambdaTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/LambdaTests.java index bce70a080dbe6..bcb92a527d9e6 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/LambdaTests.java +++ b/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();"); }); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index dd5ebd989a117..3ff9b2318f501 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -127,18 +127,14 @@ public static T expectScriptThrows(Class expectedType, } catch (Throwable e) { if (e instanceof ScriptException) { boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty(); - if (shouldHaveScriptStack) { - if (hasEmptyScriptStack) { - AssertionFailedError assertion = new AssertionFailedError("ScriptException should have a scriptStack"); - assertion.initCause(e); - throw assertion; - } - } else { - if (false == hasEmptyScriptStack) { - AssertionFailedError assertion = new AssertionFailedError("ScriptException shouldn't have a scriptStack"); - assertion.initCause(e); - throw assertion; - } + 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)) { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java index da4558a693a0d..2888eca3db4fa 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java +++ b/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].")); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index b31c04e781aeb..d60da7b795fbc 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -19,6 +19,8 @@ package org.elasticsearch.painless; +import junit.framework.AssertionFailedError; + import org.apache.lucene.util.Constants; import org.elasticsearch.script.ScriptException; @@ -45,14 +47,14 @@ public void testNullPointer() { * numbers are really 1 based character numbers. */ public void testScriptStack() { - for (String type : new String[] { - "String", - "def "}) { + for (String type : new String[] {"String", "def "}) { // trigger NPE at line 1 of the script ScriptException exception = expectThrows(ScriptException.class, () -> { exec(type + " x = null; boolean y = x.isEmpty();\n" + "return y;"); }); + // null deref at x.isEmpty(), the '.' is offset 30 + assertScriptElementColumn(30, exception); assertScriptStack(exception, "y = x.isEmpty();\n", " ^---- HERE"); @@ -63,6 +65,8 @@ public void testScriptStack() { exec(type + " x = null;\n" + "return x.isEmpty();"); }); + // null deref at x.isEmpty(), the '.' is offset 25 + assertScriptElementColumn(25, exception); assertScriptStack(exception, "return x.isEmpty();", " ^---- HERE"); @@ -71,9 +75,11 @@ public void testScriptStack() { // trigger NPE at line 3 of the script exception = expectThrows(ScriptException.class, () -> { exec(type + " x = null;\n" + - "String y = x;\n" + + type + " y = x;\n" + "return y.isEmpty();"); }); + // null deref at y.isEmpty(), the '.' is offset 39 + assertScriptElementColumn(39, exception); assertScriptStack(exception, "return y.isEmpty();", " ^---- HERE"); @@ -88,6 +94,8 @@ public void testScriptStack() { "}\n" + "return y;"); }); + // null deref at x.isEmpty(), the '.' is offset 53 + assertScriptElementColumn(53, exception); assertScriptStack(exception, "y = x.isEmpty();\n}\n", " ^---- HERE"); @@ -95,6 +103,22 @@ public void testScriptStack() { } } + private void assertScriptElementColumn(int expectedColumn, ScriptException exception) { + StackTraceElement[] stackTrace = exception.getCause().getStackTrace(); + for (int i = 0; i < stackTrace.length; i++) { + if (WriterConstants.CLASS_NAME.equals(stackTrace[i].getClassName())) { + if (expectedColumn + 1 != stackTrace[i].getLineNumber()) { + AssertionFailedError assertion = new AssertionFailedError("Expected column to be [" + expectedColumn + "] but was [" + + stackTrace[i].getLineNumber() + "]"); + assertion.initCause(exception); + throw assertion; + } + return; + } + } + fail("didn't find script stack element"); + } + public void testInvalidShift() { expectScriptThrows(ClassCastException.class, () -> { exec("float x = 15F; x <<= 2; return x;"); From c2d886a3b62cf25ba9eb70a760b2de149a8e2925 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 18 Apr 2017 20:47:27 -0400 Subject: [PATCH 5/5] Weaken test because sometimes the jvm eats stack traces.... --- .../elasticsearch/painless/ArrayLikeObjectTestCase.java | 4 ++-- .../java/org/elasticsearch/painless/ScriptTestCase.java | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java index 69b40f141e2a2..5fc41c8c63038 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ArrayLikeObjectTestCase.java @@ -77,8 +77,8 @@ private void arrayLoadStoreTestCase(boolean declareAsDef, String valueType, Obje } private void expectOutOfBounds(int index, String script, Object val) { - IndexOutOfBoundsException e = expectScriptThrows(IndexOutOfBoundsException.class, - () -> exec(script, singletonMap("val", val), true)); + IndexOutOfBoundsException e = expectScriptThrows(IndexOutOfBoundsException.class, () -> + exec(script, singletonMap("val", val), true)); try { assertThat(e.getMessage(), outOfBoundsExceptionMessageMatcher(index, 5)); } catch (AssertionError ae) { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index 3ff9b2318f501..1ab5aa14508ce 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -128,9 +128,12 @@ public static T expectScriptThrows(Class expectedType, 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; + if (0 != e.getCause().getStackTrace().length) { + // Without -XX:-OmitStackTraceInFastThrow the jvm can eat the stack trace which causes us to ignore script_stack + 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);