Repository: vackosar/gitflow-incremental-builder
Pull request #42 (2018-07-08 18:36:33)

Fix issue #41: Introduce argsForNotImpactedModules

famod: Notes: I cleaned up `UnchangedProjectsRemover` a bit, especially in terms of method names.
I also decided to introduce a new Mockito-based test for `UnchangedProjectsRemover` instead of extending the integration test to have a faster feedback.
famod: Force-pushed the changes from the review.

Patch:
diff --git a/README.md b/README.md
index d8e98eb..ffe6d2d 100644
--- a/README.md
+++ b/README.md
@@ -217,6 +217,7 @@ Maven pom properties configuration with default values is below:
 	<gib.uncommited>true</gib.uncommited>
 	<gib.untracked>true</gib.untracked>
 	<gib.skipTestsForNotImpactedModules>false</gib.skipTestsForNotImpactedModules>
+	<gib.argsForNotImpactedModules>false</gib.argsForNotImpactedModules>
 	<gib.buildAll>false</gib.buildAll>
 	<gib.compareToMergeBase>true</gib.compareToMergeBase>
 	<gib.fetchBaseBranch>false</gib.fetchBaseBranch>
@@ -242,7 +243,23 @@ Detects changed files that have not yet been committed. This does **not** includ
 
 ### gib.untracked
 
-Detects files that are not yet tracked by git (see `git status` manual). This does **not** include _uncommitted_ files. A new file is not _untracked_ anmore after it is added to the index.
+Detects files that are not yet tracked by git (see `git status` manual). This does **not** include _uncommitted_ files. A new file is not _untracked_ anymore after it is added to the index.
+
+### gib.skipTestsForNotImpactedModules
+
+In conjunction with `-am` or `gib.buildAll=true` this property disables the compilation/execution of tests for modules that have *not* been changed by adding `maven.test.skip=true`. In case a not impacted module produces a test jar just the test *execution* is disbled via `skipTests=true`.
+
+Can be combined with `gib.argsForNotImpactedModules`.
+
+### gib.argsForNotImpactedModules
+
+In conjunction with `-am` or `gib.buildAll=true` this property allows adding arguments/properties for modules that have *not* been changed to futher reduce overhead, e.g. skip Checkstyle or Enforcer plugin.
+Arguments have to be sparated with a single space character and values are optional. Example:
+```
+mvn clean install -am -Dgib.argsForNotImpactedModules='-Denforcer.skip -Dcheckstyle.skip=true'
+```
+
+Can be combined with `gib.skipTestsForNotImpactedModules`.
 
 ## Requirements
 
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
index 235a726..cf58c23 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/Configuration.java
@@ -1,24 +1,32 @@
 package com.vackosar.gitflowincrementalbuild.boundary;
 
 import com.vackosar.gitflowincrementalbuild.control.Property;
+
+import org.apache.commons.lang3.tuple.ImmutablePair;
 import org.apache.maven.MavenExecutionException;
+import org.apache.maven.execution.MavenExecutionRequest;
 import org.apache.maven.execution.MavenSession;
 
 import javax.inject.Inject;
 import javax.inject.Singleton;
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.AbstractMap;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 @Singleton
 public class Configuration {
 
-    private static final List<String> alsoMakeBehaviours = Arrays.asList("make-upstream", "make-both");
+    private static final List<String> alsoMakeBehaviours = Arrays.asList(
+            MavenExecutionRequest.REACTOR_MAKE_UPSTREAM, MavenExecutionRequest.REACTOR_MAKE_BOTH);
 
     public final boolean enabled;
     public final Optional<Path> key;
@@ -29,6 +37,7 @@ public class Configuration {
     public final boolean untracked;
     public final boolean makeUpstream;
     public final boolean skipTestsForNotImpactedModules;
+    public final Map<String, String> argsForNotImpactedModules;
     public final boolean buildAll;
     public final boolean compareToMergeBase;
     public final boolean fetchBaseBranch;
@@ -49,6 +58,8 @@ public class Configuration {
             untracked = Boolean.valueOf(Property.untracked.getValue());
             makeUpstream = alsoMakeBehaviours.contains(session.getRequest().getMakeBehavior());
             skipTestsForNotImpactedModules = Boolean.valueOf(Property.skipTestsForNotImpactedModules.getValue());
+            argsForNotImpactedModules = Collections.unmodifiableMap(
+                    parseSpaceDelimitedArgs(Property.argsForNotImpactedModules.getValue()));
             buildAll = Boolean.valueOf(Property.buildAll.getValue());
             compareToMergeBase = Boolean.valueOf(Property.compareToMergeBase.getValue());
             fetchReferenceBranch = Boolean.valueOf(Property.fetchReferenceBranch.getValue());
@@ -60,7 +71,7 @@ public class Configuration {
         }
     }
 
-    private Optional<Path> parseKey(MavenSession session) throws IOException {
+    private static Optional<Path> parseKey(MavenSession session) throws IOException {
         Path pomDir = session.getCurrentProject().getBasedir().toPath();
         String keyOptionValue = Property.repositorySshKey.getValue();
         if (keyOptionValue != null && ! keyOptionValue.isEmpty()) {
@@ -70,7 +81,24 @@ public class Configuration {
         }
     }
 
-    private void checkProperties() throws MavenExecutionException {
+    private static Map<String, String> parseSpaceDelimitedArgs(String value) {
+        return value.isEmpty()
+                ? Collections.emptyMap()
+                : Arrays.stream(value.split(" "))
+                    .map(String::trim)
+                    .filter(s -> !s.isEmpty())
+                    .map(Configuration::keyValueStringToEntry)
+                    .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (a, b) -> a, LinkedHashMap::new));
+    }
+
+    private static Map.Entry<String, String> keyValueStringToEntry(String pair) {
+        int indexOfDelim = pair.indexOf('=');
+        return indexOfDelim > 0
+                ? new AbstractMap.SimpleEntry<>(pair.substring(0, indexOfDelim), pair.substring(indexOfDelim + 1))
+                : new AbstractMap.SimpleEntry<>(pair, "");
+    }
+
+    private static void checkProperties() throws MavenExecutionException {
         try {
             System.getProperties().entrySet().stream().map(Map.Entry::getKey)
                     .filter(o -> o instanceof String).map(o -> (String) o)
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
index c3cadf0..3e2218b 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
@@ -19,20 +19,28 @@ class UnchangedProjectsRemover {
     private static final String MAVEN_TEST_SKIP = "maven.test.skip";
     private static final String MAVEN_TEST_SKIP_EXEC = "skipTests";
     static final String TEST_JAR_DETECTED = "Dependency with test-jar goal detected. Will compile test sources.";
-    private static final String GOAL_TEST_COMPILE = "test-compile";
     private static final String GOAL_TEST_JAR = "test-jar";
 
-    @Inject private Configuration configuration;
-    @Inject private Logger logger;
-    @Inject private ChangedProjects changedProjects;
-    @Inject private MavenSession mavenSession;
+    private Configuration configuration;
+    private Logger logger;
+    private ChangedProjects changedProjects;
+    private MavenSession mavenSession;
+
+    @Inject
+    UnchangedProjectsRemover(Configuration configuration, Logger logger, ChangedProjects changedProjects,
+            MavenSession mavenSession) {
+        this.configuration = configuration;
+        this.logger = logger;
+        this.changedProjects = changedProjects;
+        this.mavenSession = mavenSession;
+    }
 
     void act() throws GitAPIException, IOException {
         Set<MavenProject> changed = changedProjects.get();
         printDelimiter();
         logProjects(changed, "Changed Artifacts:");
         Set<MavenProject> impacted = changed.stream()
-            .flatMap(this::getAllDependents)
+            .flatMap(this::streamProjectWithDependents)
             .collect(Collectors.toCollection(LinkedHashSet::new));
         if (!configuration.buildAll) {
             Set<MavenProject> rebuild = getRebuildProjects(impacted);
@@ -47,7 +55,7 @@ class UnchangedProjectsRemover {
         } else {
             mavenSession.getProjects().stream()
                     .filter(p -> !impacted.contains(p))
-                    .forEach(this::ifSkipDependenciesTest);
+                    .forEach(this::applyNotImpactedModuleArgs);
         }
     }
 
@@ -62,20 +70,22 @@ class UnchangedProjectsRemover {
 
     private Stream<MavenProject> collectDependencies(Set<MavenProject> changedProjects) {
         return changedProjects.stream()
-                .flatMap(this::ifMakeUpstreamGetDependencies)
+                .flatMap(this::streamProjectWithDependencies)
                 .filter(p -> ! changedProjects.contains(p))
-                .map(this::ifSkipDependenciesTest);
+                .map(this::applyNotImpactedModuleArgs);
     }
 
-    private MavenProject ifSkipDependenciesTest(MavenProject mavenProject) {
+    private MavenProject applyNotImpactedModuleArgs(MavenProject mavenProject) {
+        final Properties projectProperties = mavenProject.getProperties();
         if (configuration.skipTestsForNotImpactedModules) {
             if (projectDeclaresTestJarGoal(mavenProject)) {
                 logger.debug(mavenProject.getArtifactId() + ": " + TEST_JAR_DETECTED);
-                mavenProject.getProperties().setProperty(MAVEN_TEST_SKIP_EXEC, Boolean.TRUE.toString());
+                projectProperties.setProperty(MAVEN_TEST_SKIP_EXEC, Boolean.TRUE.toString());
             } else {
-                mavenProject.getProperties().setProperty(MAVEN_TEST_SKIP, Boolean.TRUE.toString());
+                projectProperties.setProperty(MAVEN_TEST_SKIP, Boolean.TRUE.toString());
             }
         }
+        configuration.argsForNotImpactedModules.forEach(projectProperties::setProperty);
         return mavenProject;
     }
 
@@ -97,13 +107,13 @@ class UnchangedProjectsRemover {
         logger.info("------------------------------------------------------------------------");
     }
 
-    private Stream<MavenProject> getAllDependents(MavenProject project) {
+    private Stream<MavenProject> streamProjectWithDependents(MavenProject project) {
         return Stream.concat(
             Stream.of(project),
             mavenSession.getProjectDependencyGraph().getDownstreamProjects(project, true).stream());
     }
 
-    private Stream<MavenProject> ifMakeUpstreamGetDependencies(MavenProject project) {
+    private Stream<MavenProject> streamProjectWithDependencies(MavenProject project) {
         return Stream.concat(
             Stream.of(project),
             mavenSession.getProjectDependencyGraph().getUpstreamProjects(project, true).stream());
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java b/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
index d05c744..994feef 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/control/Property.java
@@ -9,6 +9,7 @@ public enum Property {
     uncommited("true"),
     untracked("true"),
     skipTestsForNotImpactedModules("false"),
+    argsForNotImpactedModules(""),
     buildAll("false"),
     compareToMergeBase("true"),
     fetchBaseBranch("false"),


Test patch:
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemoverTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemoverTest.java
new file mode 100644
index 0000000..225777f
--- /dev/null
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemoverTest.java
@@ -0,0 +1,160 @@
+/**
+ * 
+ */
+package com.vackosar.gitflowincrementalbuild.boundary;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Matchers.anyListOf;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.maven.execution.MavenExecutionRequest;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.execution.ProjectDependencyGraph;
+import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.logging.Logger;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
+
+import com.google.common.collect.ImmutableMap;
+import com.vackosar.gitflowincrementalbuild.control.ChangedProjects;
+import com.vackosar.gitflowincrementalbuild.control.Property;
+
+/**
+ * Tests {@link UnchangedProjectsRemover} with Mockito mocks.
+ * 
+ * @author famod
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class UnchangedProjectsRemoverTest {
+
+    @Mock
+    private MavenProject mavenProjectMock;
+
+    @Mock
+    private MavenExecutionRequest mavenExecutionRequestMock;
+
+    @Mock
+    private MavenSession mavenSessionMock;
+
+    @Mock
+    private ProjectDependencyGraph projectDependencyGraphMock;
+
+    @Mock
+    private Logger loggerMock;
+
+    @Mock
+    private ChangedProjects changedProjectsMock;
+
+    private final List<MavenProject> projects = new ArrayList<>(); 
+    
+    @Before
+    public void setup() {
+        addPropertiesToMock(mavenProjectMock);
+        when(mavenProjectMock.getBasedir()).thenReturn(new File("."));
+        when(mavenSessionMock.getCurrentProject()).thenReturn(mavenProjectMock);
+        when(mavenSessionMock.getRequest()).thenReturn(mavenExecutionRequestMock);
+        projects.add(mavenProjectMock);
+        when(mavenSessionMock.getProjects()).thenReturn(projects);
+        when(mavenSessionMock.getProjectDependencyGraph()).thenReturn(projectDependencyGraphMock);
+    }
+    
+    @Test
+    public void singleChanged() throws GitAPIException, IOException {
+        MavenProject changedProjectMock = Mockito.mock(MavenProject.class);
+        when(changedProjectsMock.get()).thenReturn(Collections.singleton(changedProjectMock));
+        projects.add(changedProjectMock);
+
+        buildUnderTest().act();
+
+        Mockito.verify(mavenSessionMock).setProjects(Collections.singletonList(changedProjectMock));
+    }
+
+    @Test
+    public void singleChanged_alsoMake_argsForNotImpactedModules() throws GitAPIException, IOException {
+        MavenProject changedModuleMock = addChangedModuleMock();
+
+        when(mavenExecutionRequestMock.getMakeBehavior()).thenReturn(MavenExecutionRequest.REACTOR_MAKE_UPSTREAM);
+
+        ImmutableMap<Property, String> propertyMap = ImmutableMap.of(
+                Property.argsForNotImpactedModules, "enforcer.skip=true argWithNoValue");
+
+        buildUnderTest(propertyMap).act();
+
+        Mockito.verify(mavenSessionMock).setProjects(Arrays.asList(changedModuleMock, mavenProjectMock));
+        
+        assertEquals("Unexpected value for enforcer.skip",
+                "true", mavenProjectMock.getProperties().getProperty("enforcer.skip"));
+        assertEquals("Unexpected value for argWithNoValue",
+                "", mavenProjectMock.getProperties().getProperty("argWithNoValue"));
+        assertEquals("Unexpected properties for unchanged project",
+                new Properties(), changedModuleMock.getProperties());
+    }
+
+    @Test
+    public void singleChanged_buildAll_argsForNotImpactedModules() throws GitAPIException, IOException {
+        MavenProject changedModuleMock = addChangedModuleMock();
+
+        ImmutableMap<Property, String> propertyMap = ImmutableMap.of(
+                Property.argsForNotImpactedModules, "enforcer.skip=true argWithNoValue",
+                Property.buildAll, "true");
+
+        buildUnderTest(propertyMap).act();
+
+        Mockito.verify(mavenSessionMock, never()).setProjects(anyListOf(MavenProject.class));
+        
+        assertEquals("Unexpected value for enforcer.skip",
+                "true", mavenProjectMock.getProperties().getProperty("enforcer.skip"));
+        assertEquals("Unexpected value for argWithNoValue",
+                "", mavenProjectMock.getProperties().getProperty("argWithNoValue"));
+        assertEquals("Unexpected properties for unchanged project",
+                new Properties(), changedModuleMock.getProperties());
+    }
+
+    private Properties addPropertiesToMock(MavenProject mavenProjectMock) {
+        Properties properties = new Properties();
+        when(mavenProjectMock.getProperties()).thenReturn(properties);
+        return properties;
+    }
+
+    private MavenProject addChangedModuleMock() throws GitAPIException, IOException {
+        MavenProject changedModuleMock = mock(MavenProject.class);
+        when(changedProjectsMock.get()).thenReturn(Collections.singleton(changedModuleMock));
+        projects.add(changedModuleMock);
+        addPropertiesToMock(changedModuleMock);
+
+        when(projectDependencyGraphMock.getUpstreamProjects(changedModuleMock, true))
+                .thenReturn(Collections.singletonList(mavenProjectMock));
+        return changedModuleMock;
+    }
+
+    private UnchangedProjectsRemover buildUnderTest() throws IOException {
+        return buildUnderTest(ImmutableMap.of());
+    }
+
+    private UnchangedProjectsRemover buildUnderTest(ImmutableMap<Property, String> propertyMap) throws IOException {
+        Configuration configuration;
+        Properties origProps = (Properties) System.getProperties().clone();
+        try {
+            propertyMap.forEach((k, v) -> System.setProperty(k.fullName(), v));
+            configuration = new Configuration(mavenSessionMock);
+        } finally {
+            System.setProperties(origProps);
+        }
+        return new UnchangedProjectsRemover(configuration, loggerMock, changedProjectsMock, mavenSessionMock);
+    }
+}