-
Notifications
You must be signed in to change notification settings - Fork 267
Test framework support for loading plugin instances #6138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test framework support for loading plugin instances #6138
Conversation
Also, added integration tests for drop_events and parse_json processors. Signed-off-by: David Venable <[email protected]>
…or loading plugin instances. Not all features are supported, so some tests are still using the classic approach. Signed-off-by: David Venable <[email protected]>
Signed-off-by: David Venable <[email protected]>
Signed-off-by: David Venable <[email protected]>
final Map<String, Object> messageMap = Map.of( | ||
"stringKey", UUID.randomUUID().toString(), | ||
"integerKey", random.nextInt(10_000) + 10, | ||
"objectKey", Map.of( | ||
"nestedKey", UUID.randomUUID().toString() | ||
), | ||
"arrayKey", List.of( | ||
UUID.randomUUID().toString(), | ||
UUID.randomUUID().toString() | ||
) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like we were discussing, we can avoid these lines repeating between the test methods, if we can generalize this part as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added #5907 to track this idea. I think it would be a nice addition.
|
||
final PluginSetting pluginSetting = generatePluginSetting(testClass, configurationFile); | ||
|
||
final PluginFactory pluginFactory = getOrCreatePluginFactory(extensionContext); | ||
|
||
final Class<?> pluginType = annotation.pluginType(); | ||
|
||
return pluginFactory.loadPlugin(pluginType, pluginSetting); | ||
} | ||
|
||
private PluginSetting generatePluginSetting(final Class<?> testClass, final String configurationFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope we can't avoid this method returning PluginSetting
as some of the processors still depend on it! Or we shouldn't add this and convert those processors before we support test framework on them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin framework still takes PluginSetting
. So until that is removed we need it here. But, this is kept private so that it doesn't go into the plugins themselves.
|
||
final Class<?> pluginType = annotation.pluginType(); | ||
|
||
return pluginFactory.loadPlugin(pluginType, pluginSetting); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be not required for this PR but we should test this with one of the plugin that has its PluginConfig injected. We are not explicitly registering PluginConfig object as a bean here but I think it is not required since we are scanning the entire DefaultPluginFactory.class
package where are already initializing PluginConfig bean.
|
||
private static PipelinesDataFlowModel readPipelinesDataFlowModel(final Class<?> testClass, final String configurationFile) { | ||
final PipelinesDataFlowModel pipelinesDataFlowModel; | ||
try(final InputStream resourceStream = testClass.getResourceAsStream(configurationFile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are expecting these files to be in the same package as the Plugin class. It may not be a problem but if the number of scenarios goes high, there may be a need to organize these into folders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, it loads relative to the test class actually. So there is some additional flexibility. It should also support sub-directories, though I've not tested that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice addition 👍
Description
This adds a new feature to the Data Prepper plugin test framework. It provides the ability for plugin authors to write tests that will load plugin instances from a YAML file. The core feature is the addition of the
@PluginConfigurationFile
annotation that can be applied to method parameters. It uses JUnit's support for parameter injection to load the actual plugin and provide it to the plugin author's unit test.The biggest new code change is the addition of
PluginInstanceParameterResolver
which perform the actual loading.This also provides support for plugin tests to get an injected
EventFactory
orEventKeyFactory
to help with authoring tests.This PR also adds plugin test framework support to the
drop_events
andparse_json
plugins.Additionally, I updated the
GrokProcessorIT
to make as many tests as possible use this new approach. These tests are now using the framework to load plugin configurations rather than mutatingPluginSetting
directly. However, there were some tests that I couldn't yet update. These were:I do plan to look into this in a future PR.
Issues Resolved
Resolves #5917
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.