Painless: more testing for script_stack #24168
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`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`.
nik9000
added
:Plugin Lang Painless
>test
nik9000
commented
Apr 18, 2017
| try { | ||
| runnable.run(); | ||
| } catch (Throwable e) { | ||
| if (e instanceof ScriptException) { | ||
| boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty(); |
There was a problem hiding this comment.
I was originally hunting down a case where we weren't producing the stack. I never found it but I figure something like this is nice to have around anyway.
rjernst
reviewed
Apr 18, 2017
| try { | ||
| runnable.run(); | ||
| } catch (Throwable e) { | ||
| if (e instanceof ScriptException) { | ||
| boolean hasEmptyScriptStack = ((ScriptException) e).getScriptStack().isEmpty(); | ||
| if (shouldHaveScriptStack) { |
There was a problem hiding this comment.
This would read cleaner as:
if (shouldHaveScriptStack && hasEmptyScripStack) {
...
} else if (shouldHaveScriptStack == false && hasEmptyScriptStack == false) {
...
}
There was a problem hiding this comment.
I can do it that way, sure.
| }); | ||
| // null deref at x.isEmpty(), the '.' is offset 30 (+1) | ||
| assertEquals(30 + 1, exception.getStackTrace()[0].getLineNumber()); |
There was a problem hiding this comment.
I don't think we should lose the line number testing?
There was a problem hiding this comment.
The line numbers don't line up in the def and String case. I can put something together to keep it but in general script_stack is the thing we expect people to use. The line numbers leak a lot of painless internals that we strip from script_stack and script_stack is built from them anyway. I can keep them but it'll be funky.
There was a problem hiding this comment.
I don't care what we test it with, but it is something we return to users, so it should remain tested.
|
@rjernst I pushed updates. |
rjernst
approved these changes
Apr 19, 2017
nik9000
added a commit
that referenced
this pull request
Apr 19, 2017
`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`.
|
Thanks for reviewing @rjernst! |
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Apr 19, 2017
* master: Add BucketMetricValue interface (elastic#24188) Enable index-time sorting (elastic#24055) Clarify elasticsearch user uid:gid mapping in Docker docs Update field-names-field.asciidoc (elastic#24178) ElectMasterService.hasEnoughMasterNodes should return false if no masters were found Remove Ubuntu 12.04 (elastic#24161) [Test] Add unit tests for InternalHDRPercentilesTests (elastic#24157) Replicate write failures (elastic#23314) Rename variable in translog simple commit test Strengthen translog commit with open view test Stronger check in translog prepare and commit test Fix translog prepare commit and commit test ingest-node.asciidoc - Clarify json processor (elastic#21876) Painless: more testing for script_stack (elastic#24168)
clintongormley
added
:Core/Infra/Scripting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
script_stackis super useful when debugging Painless scriptsbecause 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 testfor
script_stack.