[GitHub] [pulsar] nicoloboschi commented on a change in pull request #14098: [test] Replace PowerMockito#mockStatic usages with mockito-inline

[GitBox <gi...@apache.org>]

nicoloboschi commented on a change in pull request #14098:
URL: https://github.com/apache/pulsar/pull/14098#discussion_r801806585



##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarClassLoader.java
##########
@@ -127,6 +127,25 @@
 @Slf4j
 public class NarClassLoader extends URLClassLoader {
 
+    public static class Factory {

Review comment:
       The important thing is that you don't have to put the starting method on NarClassLoader (because you can't mock it as well)
   
   So, solutions might be:
   
   1) First level class factory: NarClassLoaderFactory. Same as in this PR, but as first level which is more readable (`NarClassLoaderFactory.create(...)`)
   
   2) Use a builder pattern NarClassLoaderBuilder and use (`NarClassLoaderBuilder.newBuilder().withArchive()....build()`) and then mock the builder in the test
   
   3) Have a inner class inside NarClassLoader responsible for the NarClassLoader creation. Same as 1) but you won't change the public API. You still have to make the InternalFactory public in order to be mocked. However, it's a weird round-trip inside the NarClassLoader. Something like this:
   ```
   class InternalFactory {
     NarClassLoader createNarClassLoader(..) {
       return ... // createNarClassloader
   }
   }
   
   public static NarClassLoader getFromArchive(..) {
     return InternalFactory.createNarClassLoader(...)
   }
   ```
   IMHO all the three options are valid 
   WDYT? 
   
   




-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org