gharris1727 commented on PR #13165:
URL: https://github.com/apache/kafka/pull/13165#issuecomment-1560240995
> I'm wondering if we can get better coverage for
DelegatingClassLoader::scanPluginPath. Right now we verify in
PluginsTest::newConnectorShouldInstantiateWithPluginClassLoader that if we've
initialized a Plugins instance, and we invoke Plugins::newConnector, the
constructor for that connector is called with the correct context classloader.
But it seems like this isn't very powerful since, if the constructor is invoked
multiple times, the last invocation's classloader will be recorded--so in this
case, we're really testing Plugins::newConnector and not the instantiations
that are performed during plugin discovery.
Yeah this is a blind-spot in the existing tests. The "sampling" paradigm
requires an instance of the object in order to perform the assertions, and the
scanPluginPath implementation throws away the objects that it creates. The test
does not and cannot assert that the TCCL is correct for the first version()
call, for example.
In this specific case the regression test is still sensitive, because the
static initialization happens when the plugin constructor is first called (not
when the Class object is created). This means that we can assert the TCCL
used in the first constructor via the staticClassloader inspection.
I think the alternative would involve mocking/spying part of the
scanPluginPath (such as versionFor), or keeping track of instantiated objects
in SamplingTestPlugins, both of which seem messy, and would make this harder to
refactor in the near future. Do you think this should be addressed now, or can
it wait until the plugin path scanning refactor is landed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org