[TEST] Add mock plugins service loadServiceProviders#88690
[TEST] Add mock plugins service loadServiceProviders#88690grcevski merged 3 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
rjernst
left a comment
There was a problem hiding this comment.
Looks good, a couple minor comments.
| .instance(); | ||
|
|
||
| // We shouldn't find the FooTestService implementation with PluginOther | ||
| assertEquals(List.of(), MockPluginsService.createExtensions(TestService.class, othPlugin)); |
There was a problem hiding this comment.
I find that using the assertThat with hamcrest matchers produces much nicer failure messages for things like this. For example, here I think it would be:
assertThat(MockPluginsService.createExtensions(TestService.class, othPlugin), empty());
|
|
||
| // We should find the FooTestService implementation when we use FooPlugin, because it matches the constructor arg. | ||
| var providers = MockPluginsService.createExtensions(TestService.class, fooPlugin); | ||
| assertEquals(1, providers.size()); |
There was a problem hiding this comment.
Similar to the above comment about hamcrest, I think both this and the next assert can be one statement with hamcrest, asserting the contents of the list:
assertThat(providers, allOf(hasSize(1), everyItem(instanceOf(BarTestService.class))));
| result.addAll(createExtensions(service, pluginTuple.instance())); | ||
| } | ||
|
|
||
| return result.stream().toList(); |
There was a problem hiding this comment.
This could just be List.copyOf(result)
|
|
||
| @SuppressWarnings("unchecked") | ||
| Constructor<T>[] constructors = (Constructor<T>[]) extensionClass.getConstructors(); | ||
| boolean compatible = true; |
There was a problem hiding this comment.
When there's no constructor argument we simply fall though to load the plugin, there's no need to distinguish from which Plugin the load happened. We ensure we de-dup in the caller method by using a set, because the service provider will get loaded for every plugin.
I'll add a comment to this effect.
This PR adds a bit of infrastructure to be able to correctly load service providers with our SPIClassIterator in MockNode. Namely, the mock node loads all plugins under the same classloader in tests, therefore we need special logic to find which is the correct plugin to use when constructing SPIs with a single argument constructor.