-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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] Add mock plugins service loadServiceProviders #88690
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this leniency?
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.
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.