[SUREFIRE-1385] Add new parameter "promoteUserPropertiesToSystemProperties" (#762)

This allows to disable merging of user properties into effective system properties
Log overwritten properties
Clarify effective properties merging order

Author: kwin

Diff for: maven-failsafe-plugin/pom.xml

@@ -23,7 +23,7 @@
   <parent>
     <groupId>org.apache.maven.surefire</groupId>
     <artifactId>surefire</artifactId>
-    <version>3.3.2-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <groupId>org.apache.maven.plugins</groupId>

Diff for: maven-surefire-common/pom.xml

@@ -23,7 +23,7 @@
   <parent>
     <groupId>org.apache.maven.surefire</groupId>
     <artifactId>surefire</artifactId>
-    <version>3.3.2-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>maven-surefire-common</artifactId>

Diff for: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java

@@ -24,6 +24,7 @@
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.nio.file.Files;
+import java.text.ChoiceFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -319,25 +320,45 @@ public abstract class AbstractSurefireMojo extends AbstractMojo implements Suref
      */
     @Deprecated
     @Parameter
-    private Properties systemProperties;
+    Properties systemProperties;
 
     /**
      * List of System properties to pass to a provider.
      * The effective system properties given to a provider are contributed from several sources:
      * <ol>
+     * <li>properties set via {@link #argLine} with {@code -D} (only for forked executions)</li>
      * <li>{@link #systemProperties}</li>
      * <li>{@link AbstractSurefireMojo#getSystemPropertiesFile()} (set via parameter {@code systemPropertiesFile} on some goals)</li>
      * <li>{@link #systemPropertyVariables}</li>
-     * <li>User properties from {@link MavenSession#getUserProperties()}, usually set via CLI options given with {@code -D}</li>
+     * <li>User properties from {@link MavenSession#getUserProperties()}, usually set via CLI options given with {@code -D} on the current Maven process (only used as source if {@link #promoteUserPropertiesToSystemProperties} is {@code true})</li>
      * </ol>
      * Later sources may overwrite same named properties from earlier sources, that means for example that one cannot overwrite user properties with either
-     * {@link #systemProperties}, {@link AbstractSurefireMojo#getSystemPropertiesFile()} or {@link #systemPropertyVariables}.
+     * {@link #systemProperties}, {@link #getSystemPropertiesFile()} or {@link #systemPropertyVariables}.
+     * <p>
+     * Certain properties may only be overwritten via {@link #argLine} (applicable only for forked executions) because their values are cached and only evaluated at the start of the JRE.
+     * Those include:
+     * <ul>
+     * <li>{@code java.library.path}</li>
+     * <li>{@code file.encoding}</li>
+     * <li>{@code jdk.map.althashing.threshold}</li>
+     * <li>{@code line.separator}</li>
+     * </ul>
      *
      * @since 2.5
      * @see #systemProperties
      */
     @Parameter
-    private Map<String, String> systemPropertyVariables;
+    Map<String, String> systemPropertyVariables;
+
+    /**
+     * If set to {@code true} will also pass all user properties exposed via {@link MavenSession#getUserProperties()} as system properties to a provider.
+     * Those always take precedence over same named system properties set via any other means.
+     *
+     * @since 3.4.0
+     * @see #systemPropertyVariables
+     */
+    @Parameter(defaultValue = "true")
+    boolean promoteUserPropertiesToSystemProperties;
 
     /**
      * List of properties for configuring the testing provider. This is the preferred method of
@@ -425,7 +446,7 @@ public abstract class AbstractSurefireMojo extends AbstractMojo implements Suref
     private String jvm;
 
     /**
-     * Arbitrary JVM options to set on the command line.
+     * Arbitrary JVM options to set on the command line. Only effective for forked executions.
      * <br>
      * <br>
      * Since the Version 2.17 using an alternate syntax for {@code argLine}, <b>@{...}</b> allows late replacement
@@ -438,6 +459,7 @@ public abstract class AbstractSurefireMojo extends AbstractMojo implements Suref
      * <a href="http://maven.apache.org/surefire/maven-failsafe-plugin/faq.html">
      *     http://maven.apache.org/surefire/maven-failsafe-plugin/faq.html</a>
      *
+     * @see #forkCount
      * @since 2.1
      */
     @Parameter(property = "argLine")
@@ -543,7 +565,7 @@ public abstract class AbstractSurefireMojo extends AbstractMojo implements Suref
      * @since 2.14
      */
     @Parameter(property = "forkCount", defaultValue = "1")
-    private String forkCount;
+    String forkCount;
 
     /**
      * Indicates if forked VMs can be reused. If set to "false", a new VM is forked for each test class to be executed.
@@ -1139,10 +1161,10 @@ protected List<ProviderInfo> createProviders(TestClassPath testClasspath) throws
                 new JUnit3ProviderInfo());
     }
 
-    private SurefireProperties setupProperties() {
-        SurefireProperties sysProps = null;
+    SurefireProperties setupProperties() {
+        SurefireProperties sysPropsFromFile = null;
         try {
-            sysProps = SurefireProperties.loadProperties(getSystemPropertiesFile());
+            sysPropsFromFile = SurefireProperties.loadProperties(getSystemPropertiesFile());
         } catch (IOException e) {
             String msg = "The file '" + getSystemPropertiesFile().getAbsolutePath() + "' can't be read.";
             if (getConsoleLogger().isDebugEnabled()) {
@@ -1152,8 +1174,11 @@ private SurefireProperties setupProperties() {
             }
         }
 
-        SurefireProperties result = SurefireProperties.calculateEffectiveProperties(
-                getSystemProperties(), getSystemPropertyVariables(), getUserProperties(), sysProps);
+        SurefireProperties result = calculateEffectiveProperties(
+                getSystemProperties(),
+                getSystemPropertyVariables(),
+                promoteUserPropertiesToSystemProperties ? getUserProperties() : new Properties(),
+                sysPropsFromFile);
 
         result.setProperty("basedir", getBasedir().getAbsolutePath());
         result.setProperty("localRepository", getLocalRepositoryPath());
@@ -1184,6 +1209,38 @@ private SurefireProperties setupProperties() {
         return result;
     }
 
+    private SurefireProperties calculateEffectiveProperties(
+            Properties systemProperties,
+            Map<String, String> systemPropertyVariables,
+            Properties userProperties,
+            SurefireProperties sysPropsFromFile) {
+        SurefireProperties result = new SurefireProperties();
+        result.copyPropertiesFrom(systemProperties);
+
+        Collection<String> overwrittenProperties = result.copyPropertiesFrom(sysPropsFromFile);
+        if (!overwrittenProperties.isEmpty() && getConsoleLogger().isDebugEnabled()) {
+            getConsoleLogger().debug(getOverwrittenPropertiesLogMessage(overwrittenProperties, "systemPropertiesFile"));
+        }
+        overwrittenProperties = result.copyPropertiesFrom(systemPropertyVariables);
+        if (!overwrittenProperties.isEmpty() && getConsoleLogger().isDebugEnabled()) {
+            getConsoleLogger()
+                    .debug(getOverwrittenPropertiesLogMessage(overwrittenProperties, "systemPropertyVariables"));
+        }
+        // We used to take all of our system properties and dump them in with the
+        // user specified properties for SUREFIRE-121, causing SUREFIRE-491.
+        // Not gonna do THAT any more... instead, we only propagate those system properties
+        // that have been explicitly specified by the user via -Dkey=value on the CLI
+        if (!userProperties.isEmpty()) {
+            overwrittenProperties = result.copyPropertiesFrom(userProperties);
+            if (!overwrittenProperties.isEmpty()) {
+                getConsoleLogger()
+                        .warning(getOverwrittenPropertiesLogMessage(
+                                overwrittenProperties, "user properties from Maven session"));
+            }
+        }
+        return result;
+    }
+
     private Set<Object> systemPropertiesMatchingArgLine(SurefireProperties result) {
         Set<Object> intersection = new HashSet<>();
         if (isNotBlank(getArgLine())) {
@@ -1199,6 +1256,20 @@ private Set<Object> systemPropertiesMatchingArgLine(SurefireProperties result) {
         return intersection;
     }
 
+    private String getOverwrittenPropertiesLogMessage(
+            Collection<String> overwrittenProperties, String overwrittenBySource) {
+        if (overwrittenProperties.isEmpty()) {
+            throw new IllegalArgumentException("overwrittenProperties must not be empty");
+        }
+        // one or multiple?
+        ChoiceFormat propertyChoice = new ChoiceFormat("1#property|1>properties");
+        StringBuilder message = new StringBuilder("System ");
+        message.append(propertyChoice.format(overwrittenProperties.size())).append(" ");
+        message.append(overwrittenProperties.stream().collect(Collectors.joining("], [", "[", "]")));
+        message.append(" overwritten by ").append(overwrittenBySource);
+        return message.toString();
+    }
+
     private void showToLog(SurefireProperties props, ConsoleLogger log) {
         for (Object key : props.getStringKeySet()) {
             String value = props.getProperty((String) key);
@@ -2718,7 +2789,7 @@ private Classpath getArtifactClasspath(Artifact surefireArtifact) throws MojoExe
         return existing;
     }
 
-    private Properties getUserProperties() {
+    Properties getUserProperties() {
         return getSession().getUserProperties();
     }
 

Diff for: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java

@@ -27,6 +27,7 @@
 import java.util.Enumeration;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -40,7 +41,7 @@
 import static java.util.Map.Entry;
 
 /**
- * A properties implementation that preserves insertion order.
+ * A {@link Properties} implementation that preserves insertion order.
  */
 public class SurefireProperties extends Properties implements KeyValueSource {
     private static final Collection<String> KEYS_THAT_CANNOT_BE_USED_AS_SYSTEM_PROPERTIES =
@@ -64,9 +65,17 @@ public SurefireProperties(KeyValueSource source) {
 
     @Override
     public synchronized void putAll(Map<?, ?> t) {
-        for (Entry<?, ?> entry : t.entrySet()) {
-            put(entry.getKey(), entry.getValue());
+        putAllInternal(t);
+    }
+
+    private Collection<String> putAllInternal(Map<?, ?> source) {
+        Collection<String> overwrittenProperties = new LinkedList<>();
+        for (Entry<?, ?> entry : source.entrySet()) {
+            if (put(entry.getKey(), entry.getValue()) != null) {
+                overwrittenProperties.add(entry.getKey().toString());
+            }
         }
+        return overwrittenProperties;
     }
 
     @Override
@@ -92,12 +101,28 @@ public synchronized Enumeration<Object> keys() {
         return Collections.enumeration(items);
     }
 
-    public void copyPropertiesFrom(Properties source) {
+    /**
+     * Copies all keys and values from source to these properties, overwriting existing properties with same name
+     * @param source
+     * @return all overwritten property names (may be empty if there was no property name clash)
+     */
+    public Collection<String> copyPropertiesFrom(Properties source) {
         if (source != null) {
-            putAll(source);
+            return putAllInternal(source);
+        } else {
+            return Collections.emptyList();
         }
     }
 
+    /**
+     * Copies all keys and values from source to these properties, overwriting existing properties with same name
+     * @param source
+     * @return all overwritten property names (may be empty if there was no property name clash)
+     */
+    public Collection<String> copyPropertiesFrom(Map<String, String> source) {
+        return copyProperties(this, source);
+    }
+
     public Iterable<Object> getStringKeySet() {
         return keySet();
     }
@@ -121,34 +146,17 @@ public void copyToSystemProperties() {
         }
     }
 
-    static SurefireProperties calculateEffectiveProperties(
-            Properties systemProperties,
-            Map<String, String> systemPropertyVariables,
-            Properties userProperties,
-            SurefireProperties props) {
-        SurefireProperties result = new SurefireProperties();
-        result.copyPropertiesFrom(systemProperties);
-
-        result.copyPropertiesFrom(props);
-
-        copyProperties(result, systemPropertyVariables);
-
-        // We used to take all of our system properties and dump them in with the
-        // user specified properties for SUREFIRE-121, causing SUREFIRE-491.
-        // Not gonna do THAT any more... instead, we only propagate those system properties
-        // that have been explicitly specified by the user via -Dkey=value on the CLI
-
-        result.copyPropertiesFrom(userProperties);
-        return result;
-    }
-
-    private static void copyProperties(Properties target, Map<String, String> source) {
+    private static Collection<String> copyProperties(Properties target, Map<String, String> source) {
+        Collection<String> overwrittenProperties = new LinkedList<>();
         if (source != null) {
             for (String key : source.keySet()) {
                 String value = source.get(key);
-                target.setProperty(key, value == null ? "" : value);
+                if (target.setProperty(key, value == null ? "" : value) != null) {
+                    overwrittenProperties.add(key);
+                }
             }
         }
+        return overwrittenProperties;
     }
 
     @Override