Repository: vackosar/gitflow-incremental-builder
Pull request #288 (2020-12-13 23:35:34)

Retain project order also when no forceBuildModules are present
Introduces a comparator to be used wherever the order is important.


Patch:
diff --git a/pom.xml b/pom.xml
index 06dd6be..92ffae5 100644
--- a/pom.xml
+++ b/pom.xml
@@ -639,7 +639,7 @@
                                         <limit>
                                             <counter>BRANCH</counter>
                                             <value>COVEREDRATIO</value>
-                                            <minimum>0.82</minimum>
+                                            <minimum>0.83</minimum>
                                         </limit>
                                         <limit>
                                             <counter>CLASS</counter>
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
index 57fc560..02bbc45 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/boundary/UnchangedProjectsRemover.java
@@ -9,6 +9,7 @@ import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -19,6 +20,7 @@ import java.util.Properties;
 import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import java.util.stream.Stream;
 
 import javax.inject.Inject;
@@ -69,8 +71,9 @@ class UnchangedProjectsRemover {
     }
 
     private void doAct(Configuration config) throws GitAPIException, IOException {
+        LazyMavenProjectComparator projectComparator = new LazyMavenProjectComparator(config.mavenSession);
         // ensure to write logfile for impaced (even if just empty)
-        config.logImpactedTo.ifPresent(logFilePath -> writeImpactedLogFile(Collections.emptySet(), logFilePath, config.mavenSession));
+        config.logImpactedTo.ifPresent(logFilePath -> writeImpactedLogFile(Collections.emptySet(), logFilePath, projectComparator));
 
         final Set<MavenProject> selected;
         if (config.disableSelectedProjectsHandling) {
@@ -100,17 +103,17 @@ class UnchangedProjectsRemover {
         final Set<MavenProject> changed = changedProjects.get(config);
         printDelimiter();
         if (changed.isEmpty()) {
-            handleNoChangesDetected(selected, config);
+            handleNoChangesDetected(selected, projectComparator, config);
             return;
         }
-        logProjects(changed, "Changed Artifacts:", config.mavenSession);
+        logProjects(changed, "Changed Artifacts:", projectComparator, config.mavenSession);
 
         final Set<MavenProject> impacted = calculateImpactedProjects(selected, changed, config);
 
-        config.logImpactedTo.ifPresent(logFilePath -> writeImpactedLogFile(impacted, logFilePath, config.mavenSession));
+        config.logImpactedTo.ifPresent(logFilePath -> writeImpactedLogFile(impacted, logFilePath, projectComparator));
 
         if (!config.buildAll) {
-            modifyProjectList(selected, changed, impacted, config);
+            modifyProjectList(selected, changed, impacted, projectComparator, config);
         } else {
             config.mavenSession.getProjects().stream()
                     .filter(proj -> !impacted.contains(proj))
@@ -127,7 +130,7 @@ class UnchangedProjectsRemover {
         return config.mavenSession.getProjects().size() == 1 && config.currentProject.getModel().getModules().isEmpty();
     }
 
-    private void handleNoChangesDetected(Set<MavenProject> selected, Configuration config) {
+    private void handleNoChangesDetected(Set<MavenProject> selected, LazyMavenProjectComparator projectComparator, Configuration config) {
         if (!selected.isEmpty()) {
             logger.info("No changed artifacts detected: Building just explicitly selected projects (and their upstream and/or downstream, if requested).");
             // note: "only selected" case was handled before, so we have up- and/or downstream projects in the session as well
@@ -138,8 +141,8 @@ class UnchangedProjectsRemover {
             if (Configuration.isMakeBehaviourActive(MavenExecutionRequest.REACTOR_MAKE_UPSTREAM, config.mavenSession)) {
                 if (config.buildUpstreamMode == BuildUpstreamMode.NONE) {
                     // only retain selected and downstream
-                    config.mavenSession.setProjects(config.mavenSession.getProjects().stream()  // retain order
-                            .filter(selectedAndDownstream::contains)
+                    config.mavenSession.setProjects(selectedAndDownstream.stream()
+                            .sorted(projectComparator)
                             .collect(Collectors.toList()));
                 } else {
                     // applyUpstreamModuleArgs
@@ -151,7 +154,7 @@ class UnchangedProjectsRemover {
             // handle downstream (remove downstream projects if configured)
             if (Configuration.isMakeBehaviourActive(MavenExecutionRequest.REACTOR_MAKE_DOWNSTREAM, config.mavenSession)
                     && !config.buildDownstream) {
-                config.mavenSession.setProjects(config.mavenSession.getProjects().stream()  // retain order
+                config.mavenSession.setProjects(config.mavenSession.getProjects().stream()  // retain order, without projectComparator
                         .filter(proj -> selected.contains(proj) || !selectedAndDownstream.contains(proj))
                         .collect(Collectors.toList()));
             }
@@ -179,11 +182,11 @@ class UnchangedProjectsRemover {
                 .collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
-    private void writeImpactedLogFile(Set<MavenProject> impacted, Path logFilePath, MavenSession mavenSession) {
+    private void writeImpactedLogFile(Set<MavenProject> impacted, Path logFilePath, LazyMavenProjectComparator projectComparator) {
         List<String> projectsToLog = impacted.isEmpty()
                 ? Collections.emptyList()
-                : mavenSession.getProjects().stream()   // write in proper order
-                        .filter(impacted::contains)
+                : impacted.stream()
+                        .sorted(projectComparator)
                         .map(proj -> proj.getBasedir().getPath())
                         .collect(Collectors.toList());
         logger.debug("Writing impacted projects to {}: {}", logFilePath, projectsToLog);
@@ -194,21 +197,23 @@ class UnchangedProjectsRemover {
         }
     }
 
-    private void modifyProjectList(Set<MavenProject> selected, Set<MavenProject> changed, Set<MavenProject> impacted, Configuration config) {
+    private void modifyProjectList(Set<MavenProject> selected, Set<MavenProject> changed, Set<MavenProject> impacted,
+            LazyMavenProjectComparator projectComparator, Configuration config) {
         Set<MavenProject> rebuild = calculateRebuildProjects(selected, changed, impacted, config);
         if (rebuild.isEmpty()) {
-            handleNoChangesDetected(selected, config);
-        } else if (!config.forceBuildModules.isEmpty()) {
-            Set<MavenProject> forceBuildModules = config.mavenSession.getProjects().stream()
-                    .filter(proj -> !rebuild.contains(proj))
-                    .filter(proj -> matchesAny(proj.getArtifactId(), config.forceBuildModules))
-                    .map(proj -> applyUpstreamModuleArgs(proj, config))
-                    .collect(Collectors.toCollection(LinkedHashSet::new));
-            config.mavenSession.setProjects(config.mavenSession.getProjects().stream()
-                    .filter(proj -> forceBuildModules.contains(proj) || rebuild.contains(proj))
-                    .collect(Collectors.toList()));
+            handleNoChangesDetected(selected, projectComparator, config);
         } else {
-            config.mavenSession.setProjects(new ArrayList<>(rebuild));
+            if (!config.forceBuildModules.isEmpty()) {
+                Set<MavenProject> forceBuildModules = config.mavenSession.getProjects().stream()
+                        .filter(proj -> !rebuild.contains(proj))
+                        .filter(proj -> matchesAny(proj.getArtifactId(), config.forceBuildModules))
+                        .map(proj -> applyUpstreamModuleArgs(proj, config))
+                        .collect(Collectors.toCollection(LinkedHashSet::new));
+                rebuild.addAll(forceBuildModules);
+            }
+            config.mavenSession.setProjects(rebuild.stream()
+                    .sorted(projectComparator)
+                    .collect(Collectors.toList()));
         }
     }
 
@@ -262,10 +267,11 @@ class UnchangedProjectsRemover {
                 .anyMatch(GOAL_TEST_JAR::equals);
     }
 
-    private void logProjects(Set<MavenProject> projects, String title, MavenSession mavenSession) {
+    private void logProjects(Set<MavenProject> projects, String title, LazyMavenProjectComparator projectComparator, MavenSession mavenSession) {
         logger.info(title);
         logger.info("");
         projects.stream()
+                .sorted(projectComparator)
                 .map(proj -> {
                     String entry = proj.getArtifactId();
                     if (!mavenSession.getProjects().contains(proj)) {
@@ -369,6 +375,39 @@ class UnchangedProjectsRemover {
         }
     }
 
+    /**
+     * Compares projects by their position/index in the reactor ({@link MavenSession#getProjects()}) or secondarily in {@link MavenSession#getAllProjects()}.
+     */
+    static class LazyMavenProjectComparator implements Comparator<MavenProject> {
+
+        private final MavenSession mavenSession;
+
+        private Map<MavenProject, Integer> indexMap;
+
+        public LazyMavenProjectComparator(MavenSession mavenSession) {
+            this.mavenSession = mavenSession;
+        }
+
+        @Override
+        public int compare(MavenProject proj1, MavenProject proj2) {
+            if (indexMap == null) {
+                List<MavenProject> projects = mavenSession.getProjects();
+                indexMap = IntStream.range(0, projects.size()).boxed().collect(Collectors.toMap(projects::get, i -> i));
+                // projects might be a subset of all projects (e.g. when -pl is used)
+                List<MavenProject> allProjects = mavenSession.getAllProjects();
+                if (allProjects.size() > projects.size()) {
+                    for (int i = 0; i < allProjects.size(); i++) {
+                        MavenProject proj = allProjects.get(i);
+                        if (!indexMap.containsKey(proj)) {
+                            indexMap.put(proj, i + 100_000);  // non-reactor project receives a penalty offset (sorted to the back)
+                        }
+                    }
+                }
+            }
+            return indexMap.getOrDefault(proj1, Integer.MAX_VALUE).compareTo(indexMap.getOrDefault(proj2, Integer.MAX_VALUE));
+        }
+    }
+
     private static class LazyExpressionEvaluator {
 
         private static final Logger LOGGER = LoggerFactory.getLogger(LazyExpressionEvaluator.class);
diff --git a/src/main/java/com/vackosar/gitflowincrementalbuild/control/ChangedProjects.java b/src/main/java/com/vackosar/gitflowincrementalbuild/control/ChangedProjects.java
index 7921196..1013765 100644
--- a/src/main/java/com/vackosar/gitflowincrementalbuild/control/ChangedProjects.java
+++ b/src/main/java/com/vackosar/gitflowincrementalbuild/control/ChangedProjects.java
@@ -39,7 +39,7 @@ public class ChangedProjects {
     private MavenProject findProject(Path diffPath, Map<Path, MavenProject> modulesPathMap) {
         Path path = diffPath;
         // Files.exist() to spot changes in non-reactor module (path will then yield a null changedReactorProject).
-        // Without this check, the changed would be wrongly mapped to the "closest" reactor module (which hasn't changed at all!).
+        // Without this check, the changed path would be wrongly mapped to the "closest" reactor module (which might not have changed at all!).
         while (path != null && !modulesPathMap.containsKey(path) && !Files.exists(path.resolve("pom.xml"))) {
             path = path.getParent();
         }


Test patch:
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/BaseUnchangedProjectsRemoverTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/BaseUnchangedProjectsRemoverTest.java
index 0a92405..6195ae1 100644
--- a/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/BaseUnchangedProjectsRemoverTest.java
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/BaseUnchangedProjectsRemoverTest.java
@@ -77,6 +77,11 @@ public abstract class BaseUnchangedProjectsRemoverTest {
      */
     private final List<MavenProject> projects = new ArrayList<>();
 
+    /**
+     * Value for {@code mavenSessionMock.getAllProjects()}.
+     */
+    private final List<MavenProject> allProjects = new ArrayList<>();
+
     // gibProperties to be applied to _every_ moduleMock
     // note: at regular runtime (via Maven), each module will automatically contain the properties of its parent(s),
     //       which has to be emulated here, even though this test does not (yet) have an explicit root project
@@ -93,6 +98,7 @@ public abstract class BaseUnchangedProjectsRemoverTest {
         when(mavenExecutionRequestMock.isRecursive()).thenReturn(true);
         when(mavenSessionMock.getRequest()).thenReturn(mavenExecutionRequestMock);
         when(mavenSessionMock.getProjects()).thenReturn(projects);
+        when(mavenSessionMock.getAllProjects()).thenReturn(allProjects);
         when(mavenSessionMock.getProjectDependencyGraph()).thenReturn(projectDependencyGraphMock);
         when(changedProjectsMock.get(any(Configuration.class))).thenReturn(changedProjects);
 
@@ -115,6 +121,7 @@ public abstract class BaseUnchangedProjectsRemoverTest {
             changedProjects.add(newModuleMock);
         }
         projects.add(newModuleMock);    // add to projects that are seen by the session, which can be changed afterwards via overrideProjects()
+        allProjects.add(newModuleMock);
 
         when(newModuleMock.getProperties()).thenReturn(new Properties());
         newModuleMock.getProperties().putAll(gibProperties);
@@ -152,6 +159,7 @@ public abstract class BaseUnchangedProjectsRemoverTest {
     protected void overrideProjects(MavenProject... moduleMocks) {
         projects.clear();
         projects.addAll(Arrays.asList(moduleMocks));
+        // note: allProject always contains _all_ modules!
 
         // Maven shifts currentProject to the first(!) project (as per Maven 3.6.3 with -pl and -f)
         MavenProject firstProject = moduleMocks[0];
diff --git a/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/LazyMavenProjectComparatorTest.java b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/LazyMavenProjectComparatorTest.java
new file mode 100644
index 0000000..5b19560
--- /dev/null
+++ b/src/test/java/com/vackosar/gitflowincrementalbuild/boundary/LazyMavenProjectComparatorTest.java
@@ -0,0 +1,112 @@
+package com.vackosar.gitflowincrementalbuild.boundary;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.Collections;
+
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.project.MavenProject;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import com.vackosar.gitflowincrementalbuild.boundary.UnchangedProjectsRemover.LazyMavenProjectComparator;
+
+@ExtendWith(MockitoExtension.class)
+public class LazyMavenProjectComparatorTest {
+
+    @Mock
+    private MavenSession mavenSessionMock;
+
+    @InjectMocks
+    private LazyMavenProjectComparator underTest;
+
+    @Test
+    public void bothNull() {
+        assertThat(underTest.compare(null, null)).isEqualTo(0);
+    }
+
+    @Test
+    public void firstNull() {
+        MavenProject second = new MavenProject();
+        when(mavenSessionMock.getProjects()).thenReturn(Collections.singletonList(second));
+
+        assertThat(underTest.compare(null, second)).isEqualTo(1);
+    }
+
+    @Test
+    public void secondNull() {
+        MavenProject first = new MavenProject();
+        when(mavenSessionMock.getProjects()).thenReturn(Collections.singletonList(first));
+
+        assertThat(underTest.compare(first, null)).isEqualTo(-1);
+    }
+
+    @Test
+    public void firstBeforeSecond() {
+        MavenProject first = projWithAid("first");
+        MavenProject second = projWithAid("second");
+        when(mavenSessionMock.getProjects()).thenReturn(Arrays.asList(first, second));
+
+        assertThat(underTest.compare(first, second)).isEqualTo(-1);
+    }
+
+    @Test
+    public void secondAfterFirst() {
+        MavenProject first = projWithAid("first");
+        MavenProject second = projWithAid("second");
+        when(mavenSessionMock.getProjects()).thenReturn(Arrays.asList(first, second));
+
+        assertThat(underTest.compare(second, first)).isEqualTo(1);
+    }
+
+    @Test
+    public void same() {
+        MavenProject first = projWithAid("first");
+        MavenProject second = projWithAid("second");
+        when(mavenSessionMock.getProjects()).thenReturn(Arrays.asList(first, second));
+
+        assertThat(underTest.compare(first, first)).isEqualTo(0);
+    }
+
+    @Test
+    public void indexMapIsRetained() {
+        assertThat(underTest.compare(null, null)).isEqualTo(0);
+
+        verify(mavenSessionMock).getProjects();
+
+        assertThat(underTest.compare(null, null)).isEqualTo(0);
+
+        verify(mavenSessionMock).getProjects(); // still only once, from the first call!
+    }
+
+    @Test
+    public void nonReactorAfterReactor() {
+        MavenProject nonReactor = projWithAid("nonReactor");
+        MavenProject reactor = projWithAid("reactor");
+        when(mavenSessionMock.getProjects()).thenReturn(Collections.singletonList(reactor));
+        when(mavenSessionMock.getAllProjects()).thenReturn(Arrays.asList(nonReactor, reactor));
+
+        assertThat(underTest.compare(nonReactor, reactor)).isEqualTo(1);
+    }
+
+    @Test
+    public void secondNonReactoAfterFirstNonReactor() {
+        MavenProject first = projWithAid("first");
+        MavenProject second = projWithAid("second");
+        when(mavenSessionMock.getAllProjects()).thenReturn(Arrays.asList(first, second));
+
+        assertThat(underTest.compare(second, first)).isEqualTo(1);
+    }
+
+    private MavenProject projWithAid(String artifactId) {
+        MavenProject proj = new MavenProject();
+        proj.setArtifactId(artifactId);
+        return proj;
+    }
+}