Skip to content

[TEST] Kill remaining lang-groovy messy tests#19621

Merged
tlrx merged 1 commit intoelastic:masterfrom
tlrx:kill-messy-tests
Aug 1, 2016
Merged

[TEST] Kill remaining lang-groovy messy tests#19621
tlrx merged 1 commit intoelastic:masterfrom
tlrx:kill-messy-tests

Conversation

@tlrx
Copy link
Copy Markdown
Member

@tlrx tlrx commented Jul 27, 2016

After #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 #13837 has been created to track these messy tests in order to clean them up.

The work started with #19280, #19302 and #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.

closes #13837

@tlrx tlrx added >test Issues or PRs that are addressing/adding tests review :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v5.0.0-alpha5 labels Jul 27, 2016
@nik9000 nik9000 self-assigned this Jul 27, 2016
nik9000
nik9000 reviewed Jul 27, 2016
View reviewed changes
core/src/test/java/org/elasticsearch/script/IndexLookupIT.java Outdated
Copy link
Copy Markdown
Member

@nik9000 nik9000 Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe s/_FREQUENCIES|_OFFSETS|_PAYLOADS|_POSITIONS|_CACHE/" + INCLUDE_ALL + "/?

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Jul 28, 2016

@nik9000 Thanks for your review!

I addressed all your comments. I also made the MockScriptEngine stricter so that it now throws an exception when compiling undefined scripts.

Can you please have another look?

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Jul 29, 2016

@nik9000 Do you need any more information or do you have any more comment? I'd like to get this in. Thanks

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Jul 29, 2016

LGTM. Sorry to wedge this open so long.

Elasticmachine is complaining about:

  2> REPRODUCE WITH: gradle :core:integTest -Dtests.seed=FBA82156698EB076 -Dtests.class=org.elasticsearch.script.IndexLookupIT -Dtests.method="testCallWithDifferentFlagsFails" -Dtests.security.manager=true -Dtests.locale=mk -Dtests.timezone=America/Argentina/San_Juan

so I guess that is all that is left to solve before merging.

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Jul 29, 2016

@nik9000 Thanks! I thought I fixed this test but obviously not. It should be OK now.

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Jul 29, 2016

:(

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Jul 29, 2016

Sad jenkins.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Jul 29, 2016

Lots of fun failures this time!

   > Caused by: java.lang.IllegalArgumentException: No pre defined script matching [{ "match_all" : {}}] for script with name [null], did you declare the mocked script?

@tlrx
Copy link
Copy Markdown
Member Author

tlrx commented Jul 29, 2016

Yes, I made MockScriptEngine more strict and it now shouts when a script is used but not registered first. Looks like TemplateQueryBuilderTests uses mocked scripts and mustache scripts but the way the test is initialized only MockScriptEngine is used. It's almost a useless test.

@nik9000
Copy link
Copy Markdown
Member

nik9000 commented Aug 1, 2016

Last change makes sense to me. LGTM!

@tlrx tlrx removed the review label Aug 1, 2016
@tlrx tlrx force-pushed the kill-messy-tests branch from 56612c9 to 25836af Compare August 1, 2016 14:59
@tlrx
[TEST] Kill remaining lang-groovy messy tests
3869029 
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
@tlrx tlrx force-pushed the kill-messy-tests branch from 25836af to 3869029 Compare August 1, 2016 14:59
@tlrx tlrx merged commit 3869029 into elastic:master Aug 1, 2016
@tlrx tlrx mentioned this pull request Aug 1, 2016
Closed
@tlrx tlrx deleted the kill-messy-tests branch August 1, 2016 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nik9000 nik9000

Labels

:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >test Issues or PRs that are addressing/adding tests v5.0.0-alpha5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rip the grooviness out of 'messy tests' in groovy plugin

3 participants

@tlrx @nik9000 @clintongormley