michael-o commented on code in PR #762: URL: https://github.com/apache/maven-surefire/pull/762#discussion_r1688670224
########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java: ########## @@ -1184,6 +1208,39 @@ 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, "sysPropsFile")); Review Comment: The parameter name is `systemPropertiesFile` and this is what I would pass here to avoid confusion ########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java: ########## @@ -1184,6 +1208,39 @@ 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, "sysPropsFile")); + } + 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 + // and only if useUserPropertiesAsSystemProperty is set to true Review Comment: I believe that this comment is redundant because wether or not user properties have been passed is at the discretion of the caller. This method shouldn' care about the decision logic. ########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java: ########## @@ -1152,8 +1173,11 @@ private SurefireProperties setupProperties() { } } - SurefireProperties result = SurefireProperties.calculateEffectiveProperties( - getSystemProperties(), getSystemPropertyVariables(), getUserProperties(), sysProps); + SurefireProperties result = calculateEffectiveProperties( + getSystemProperties(), + getSystemPropertyVariables(), + useUserPropertiesAsSystemProperty ? getUserProperties() : null, Review Comment: Wouldn't it be better, for consistency reasons, to pass empty properties here instead of `null`. That makes the code more consistent with the other sources. ########## maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java: ########## @@ -319,25 +320,44 @@ 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 #useUserPropertiesAsSystemProperty} 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 useUserPropertiesAsSystemProperty; Review Comment: `useUserPropertiesAsSystemProperties` -- 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: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org