Skip to content

Conversation

tflobbe
Copy link
Member

@tflobbe tflobbe commented Oct 6, 2025

Description

This draft is a possible solution for #15292. I implemented the getName() as the simple class name, but could also be just a string that represents the class, like default/panama.
With this new method, the initialization of the Holder.INSTANCE could happen from a caller that's not in the VALID_CALLERS. It would not be returned though, I'm not sure if that's a problem. getInstance would still require valid callers.
An alternative approach could be to refactor the lookup method in a way that the getImplementationName could use to determine the implementation without actually initializing it.

}

public void testGetProviderName() {
assertEquals("DefaultVectorizationProvider", VectorizationProvider.getImplementationName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all makes sense to me, but I'm wondering under what circumstances we would have an alternate provider and whether we can test that. Is that something that only happens in the MRJAR code? If that's right, do we have MRJAR-specific tests where we could test that this also works when we have a non-default provider?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, under what circumstances we could get VectorizationProvider.getImplementationName() return something other than DefaultVectorizationProvider (and thus, make the test fail?). If so, I was thinking something along those lines too, like, if someone runs the tests with the right conditions, this could return the Panama implementation and make the test fail.
I guess an option to handle this case is to relax the test and make the return name be any of the existing implementations. Still, not ideal because it would break in the future if a new implementation is added but not included in this list. We could make the test more complex by looking for all implementations dynamically, but at that point we aren't testing anything that valuable and the test becomes way more complex than the code itself.
Is that what you were thinking? or did I misunderstood your comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants