[TEST] Move back some messy tests from Groovy plugin to core#19280
[TEST] Move back some messy tests from Groovy plugin to core#19280tlrx merged 1 commit intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
It feels weird to default here. Maybe just let the NPE bubble up?
There was a problem hiding this comment.
Yes, I agree, I just forgot to change that.
|
I left a few comments. I love the idea of using a script-looking implementation as the id of the script. I think I'd prefer if the custom scripts used for aggregations were shared. I see that not all of them are shared, but the ones that are it'd be nice if they were in a top level class in an appropriate package. Mostly because I think those implementations might drift apart over time and while they are the same now they'll change and we'll have subtle bugs you can only find by comparing two nearly identical chunks of code. |
|
@nik9000 Thanks for your review! I updated the code, can you please have another look?
I agree too, let's do that. |
|
LGTM |
2d1c818 to
dc8b202
Compare
This commit moves back some messy tests that have been placed in lang-groovy module in elastic#13834. It removes the dependency on Groovy plugin as well as change back the tests to integration tests (IT suffix). It also changes the current MockScriptEngine and MockScriptPlugin to make it easier to use.
dc8b202 to
b58f2eb
Compare
|
Thanks @nik9000 ! |
After elastic#13834 many tests that used Groovy scripts (for good or bad reason) in their tests have been moved in the lang-groovy module and the issue elastic#13837 has been created to track these messy tests in order to clean them up. The work started with elastic#19280, elastic#19302 and elastic#19336 and this PR moves the remaining messy tests back in core, removes the dependency on Groovy, changes the scripts in order to use the mocked script engine, and change the tests to integration tests. It also moves IndexLookupIT test back (even if it has good chance to be removed soon) and fixes its tests. It also changes AbstractQueryTestCase to use custom script plugins in tests. closes elastic#13837
After #13834 many tests that used Groovy scripts (for good or bad reason) in their tests have been moved in the
lang-groovymodule and the issue #13837 has been created to track these messy tests in order to clean them up.This PR moves
BucketSelectorIT,BucketScriptIT,HDRPercentileRanksITandHDRPercentilesITback at their original place in core, removes the dependency on Groovy, changes the scripts in order to use the mocked script engineMockScriptEngineinstead, and change them back to integration tests.This PR also changes
MockScriptEngineandMockScriptPluginso that it is easier, I hope, to mock scripts.