Cleaner provider logic (#275)

uber-archive · Sep 24, 2018 · 1d5c38b · 1d5c38b
1 parent db36325
commit 1d5c38b
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 245 deletions.
diff --git a/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java b/src/main/java/com/uber/jenkins/phabricator/PhabricatorNotifier.java
@@ -24,14 +24,17 @@
 import com.uber.jenkins.phabricator.conduit.ConduitAPIException;
 import com.uber.jenkins.phabricator.conduit.Differential;
 import com.uber.jenkins.phabricator.conduit.DifferentialClient;
+import com.uber.jenkins.phabricator.coverage.CoberturaCoverageProvider;
 import com.uber.jenkins.phabricator.coverage.CodeCoverageMetrics;
 import com.uber.jenkins.phabricator.coverage.CoverageProvider;
+import com.uber.jenkins.phabricator.coverage.JacocoCoverageProvider;
 import com.uber.jenkins.phabricator.credentials.ConduitCredentials;
 import com.uber.jenkins.phabricator.provider.InstanceProvider;
 import com.uber.jenkins.phabricator.tasks.NonDifferentialBuildTask;
 import com.uber.jenkins.phabricator.tasks.NonDifferentialHarbormasterTask;
 import com.uber.jenkins.phabricator.tasks.Task;
 import com.uber.jenkins.phabricator.uberalls.UberallsClient;
+import com.uber.jenkins.phabricator.unit.JUnitTestProvider;
 import com.uber.jenkins.phabricator.unit.UnitTestProvider;
 import com.uber.jenkins.phabricator.utils.CommonUtils;
 import com.uber.jenkins.phabricator.utils.Logger;
@@ -63,13 +66,6 @@
 import java.util.Set;
 
 public class PhabricatorNotifier extends Notifier implements SimpleBuildStep {
-    public static final String COBERTURA_CLASS_NAME = "com.uber.jenkins.phabricator.coverage.CoberturaCoverageProvider";
-
-    private static final String JUNIT_PLUGIN_NAME = "junit";
-    private static final String JUNIT_CLASS_NAME = "com.uber.jenkins.phabricator.unit.JUnitTestProvider";
-    private static final String COBERTURA_PLUGIN_NAME = "cobertura";
-    private static final String JACOCO_PLUGIN_NAME = "jacoco";
-    private static final String JACOCO_CLASS_NAME = "com.uber.jenkins.phabricator.coverage.JacocoCoverageProvider";
     private static final String ABORT_TAG = "abort";
     private static final String UBERALLS_TAG = "uberalls";
     private static final String CONDUIT_TAG = "conduit";
@@ -327,12 +323,12 @@ private CoverageProvider getCoverageProvider(Run<?, ?> build, FilePath workspace
         Logger logger = new Logger(listener.getLogger());
         List<CoverageProvider> coverageProviders = new ArrayList<CoverageProvider>();
 
-        CoverageProvider coberturaCoverage = makeProvider(COBERTURA_PLUGIN_NAME, COBERTURA_CLASS_NAME, logger);
+        CoverageProvider coberturaCoverage = InstanceProvider.getCoberturaCoverageProvider(logger);
         if (coberturaCoverage != null) {
             coverageProviders.add(coberturaCoverage);
         }
 
-        CoverageProvider jacocoCoverage = makeProvider(JACOCO_PLUGIN_NAME, JACOCO_CLASS_NAME, logger);
+        CoverageProvider jacocoCoverage = InstanceProvider.getJacocoCoverageProvider(logger);
         if (jacocoCoverage != null) {
             coverageProviders.add(jacocoCoverage);
         }
@@ -361,24 +357,14 @@ private CoverageProvider getCoverageProvider(Run<?, ?> build, FilePath workspace
     private UnitTestProvider getUnitProvider(Run<?, ?> build, TaskListener listener) {
         Logger logger = new Logger(listener.getLogger());
 
-        UnitTestProvider unitProvider = makeProvider(JUNIT_PLUGIN_NAME, JUNIT_CLASS_NAME, logger);
+        UnitTestProvider unitProvider = InstanceProvider.getUnitTestProvider(logger);
         if (unitProvider == null) {
             return null;
         }
         unitProvider.setBuild(build);
         return unitProvider;
     }
 
-    private <T> T makeProvider(String pluginName, String className, Logger logger) {
-        InstanceProvider<T> instanceProvider = new InstanceProvider<T>(
-                Jenkins.getInstance(),
-                pluginName,
-                className,
-                logger
-        );
-        return instanceProvider.getInstance();
-    }
-
     @SuppressWarnings("UnusedDeclaration")
     public boolean isCommentOnSuccess() {
         return commentOnSuccess;

diff --git a/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java b/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java
@@ -31,11 +31,11 @@
 import java.util.List;
 import java.util.Map;
 
-public class PathResolver {
+class PathResolver {
     private final FilePath root;
     private final List<String> candidates;
 
-    public PathResolver(FilePath root, List<String> candidates) {
+    PathResolver(FilePath root, List<String> candidates) {
         this.root = root;
         this.candidates = candidates;
     }
@@ -50,7 +50,7 @@ public PathResolver(FilePath root, List<String> candidates) {
      * Returns map where key is filename and value - sourceDirectory
      */
 
-    public Map<String, String> choose(List<String> filenames) {
+    Map<String, String> choose(List<String> filenames) {
         try {
             if (candidates.size() > 0) {
                 return root.act(new PathResolverChooseMultiCallable(candidates, filenames));

diff --git a/src/main/java/com/uber/jenkins/phabricator/provider/BaseProvider.java b/src/main/java/com/uber/jenkins/phabricator/provider/BaseProvider.java
diff --git a/src/main/java/com/uber/jenkins/phabricator/provider/InstanceProvider.java b/src/main/java/com/uber/jenkins/phabricator/provider/InstanceProvider.java
@@ -20,48 +20,80 @@
 
 package com.uber.jenkins.phabricator.provider;
 
+import com.uber.jenkins.phabricator.coverage.CoberturaCoverageProvider;
+import com.uber.jenkins.phabricator.coverage.CoverageProvider;
+import com.uber.jenkins.phabricator.coverage.JacocoCoverageProvider;
+import com.uber.jenkins.phabricator.unit.JUnitTestProvider;
+import com.uber.jenkins.phabricator.unit.UnitTestProvider;
 import com.uber.jenkins.phabricator.utils.Logger;
+
 import jenkins.model.Jenkins;
 
-public class InstanceProvider<T> {
+public abstract class InstanceProvider<T> {
+
+    private static final String COBERTURA_PLUGIN_NAME = "cobertura";
+    private static final String JACOCO_PLUGIN_NAME = "jacoco";
+    private static final String JUNIT_PLUGIN_NAME = "junit";
+
     private static final String LOGGER_TAG = "plugin-provider";
-    private final Provider<T> provider;
-    private final String className;
-    private final Logger logger;
+    private final Jenkins jenkins;
     private final String pluginName;
+    private final Logger logger;
 
     /**
      * Encapsulate lazily loading a concrete implementation when a plugin is available
+     *
      * @param jenkins the instance of Jenkins
      * @param pluginName the name of the plugin, e.g. "cobertura" or "junit" (maven name)
-     * @param className the concrete class name (com.uber.phabricator...)
-     * @param logger the logger to use
      */
-    public InstanceProvider(Jenkins jenkins, String pluginName, String className, Logger logger) {
-        this.provider = new BaseProvider<T>(
-                jenkins,
-                pluginName,
-                logger
-        );
+    private InstanceProvider(Jenkins jenkins, String pluginName, Logger logger) {
+        this.jenkins = jenkins;
         this.pluginName = pluginName;
-        this.className = className;
         this.logger = logger;
     }
 
     /**
      * Get an instance of the desired implementation, if available
+     *
      * @return the class desired
      */
-    public T getInstance() {
-        if (!provider.isAvailable()) {
+    final T getInstance() {
+        if (jenkins.getPlugin(pluginName) == null) {
             logger.info(LOGGER_TAG, String.format("'%s' plugin not installed.", pluginName));
             return null;
         }
-        T instance = provider.getInstance(className);
-        if (instance == null) {
-            logger.warn(LOGGER_TAG,
-                    String.format("Unable to instantiate plugin provider for '%s'. This should not happen.", pluginName));
-        }
-        return instance;
+        return makeInstance();
+    }
+
+    abstract T makeInstance();
+
+    public static CoverageProvider getCoberturaCoverageProvider(Logger logger) {
+        return new InstanceProvider<CoverageProvider>(Jenkins.getInstance(),
+                COBERTURA_PLUGIN_NAME, logger) {
+            @Override
+            protected CoverageProvider makeInstance() {
+                return new CoberturaCoverageProvider();
+            }
+        }.getInstance();
+    }
+
+    public static CoverageProvider getJacocoCoverageProvider(Logger logger) {
+        return new InstanceProvider<CoverageProvider>(Jenkins.getInstance(),
+                JACOCO_PLUGIN_NAME, logger) {
+            @Override
+            protected CoverageProvider makeInstance() {
+                return new JacocoCoverageProvider();
+            }
+        }.getInstance();
+    }
+
+    public static UnitTestProvider getUnitTestProvider(Logger logger) {
+        return new InstanceProvider<UnitTestProvider>(Jenkins.getInstance(),
+                JUNIT_PLUGIN_NAME, logger) {
+            @Override
+            protected UnitTestProvider makeInstance() {
+                return new JUnitTestProvider();
+            }
+        }.getInstance();
     }
 }
diff --git a/src/main/java/com/uber/jenkins/phabricator/provider/Provider.java b/src/main/java/com/uber/jenkins/phabricator/provider/Provider.java
diff --git a/src/test/java/com/uber/jenkins/phabricator/provider/BaseProviderTest.java b/src/test/java/com/uber/jenkins/phabricator/provider/BaseProviderTest.java