Factor groovy out of core into lang-groovy #13834
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
| @@ -758,7 +757,7 @@ public void testIds() { | |||
| ids.put(92, org.elasticsearch.indices.recovery.DelayRecoveryException.class); | |||
| ids.put(93, org.elasticsearch.search.warmer.IndexWarmerMissingException.class); | |||
| ids.put(94, org.elasticsearch.client.transport.NoNodeAvailableException.class); | |||
| ids.put(95, org.elasticsearch.script.groovy.GroovyScriptCompilationException.class); | |||
| ids.put(95, null); | |||
There was a problem hiding this comment.
Should this really be null?
There was a problem hiding this comment.
Yes, see the note in ElasticsearchException. numbers should never be removed, only assigned to null if the exception is removed.
|
LGTM. Glad to see this moved to a plugin. |
|
+1 on moving the tests into the plugin - We can gracefully move them back or to unittests while we go.. I think we should just open a issue that we have to do that so we don't forget! Thanks for working on this! |
rmuir
added a commit
that referenced
this pull request
Sep 29, 2015
Factor groovy out of core into lang-groovy
|
I just backported this to 2.x |
brwe
added a commit
to brwe/elasticsearch
that referenced
this pull request
Oct 5, 2015
groovy moved to a plugin but the tests rely on it see elastic#13834
brwe
added a commit
that referenced
this pull request
Oct 6, 2015
groovy moved to a plugin but the tests rely on it see #13834
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Jul 7, 2016
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.
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Jul 7, 2016
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. This commit moves more 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.
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Jul 25, 2016
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. This commit moves more 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.
tlrx
added a commit
to tlrx/elasticsearch
that referenced
this pull request
Aug 1, 2016
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
synhershko
pushed a commit
to plugind/builder-elasticsearch-gradle-plugin
that referenced
this pull request
Jan 15, 2018
This commit moves back some messy tests that have been placed in lang-groovy module in elastic/elasticsearch#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.
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.
See #13725 for the motivation.
Note that many core tests (approximately 50) depend on groovy. Rather than try to refactor all of these at once, I simply moved them all to this plugin under the
org.elasticsearch.messy.testspackage, so we can deal with them iteratively. This preserves them in their entirety, so we don't lose coverage, or go back and forth on how each one should be fixed: instead this way we can clean up over time.Last time when factoring out expressions, @rjernst stayed up all night to fix a bunch of tests, but we can't do things this way, that is why I did what I did.
The package-info.java has guidelines about what I think we should do, to get them back in core. It also has the list of renames so you know where each file came from (in case its easy to fix and move back to core).
I made no security changes here to not bury in this noise.