[
https://issues.apache.org/jira/browse/SUREFIRE-1385?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17868179#comment-17868179
]
ASF GitHub Bot commented on SUREFIRE-1385:
------------------------------------------
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`
> System properties defined in the Surefire and Failsafe plugin configuration
> should override user properties
> -----------------------------------------------------------------------------------------------------------
>
> Key: SUREFIRE-1385
> URL: https://issues.apache.org/jira/browse/SUREFIRE-1385
> Project: Maven Surefire
> Issue Type: Improvement
> Components: Maven Failsafe Plugin, Maven Surefire Plugin
> Affects Versions: 2.20
> Reporter: Guillaume Boué
> Assignee: Konrad Windszus
> Priority: Major
>
> Consider a build with the following POM configuration for the Maven Failsafe
> Plugin:
> {code:xml}
> <configuration>
> <systemPropertyVariables>
> <prop>foo</prop>
> </systemPropertyVariables>
> </configuration>
> {code}
> When running the build with the command line {{mvn -Dprop=bar ...}}, the
> tests would be passed a system property with a value of {{bar}} instead of
> {{foo}}.
> This is counter-intuitive since direct configuration of the plugin is
> overriden by the more general properties passed on the command line. I would
> have expected the closer definition in the POM to override the one passed
> with the CLI. Furthermore, in the case of the above sample, it would not be
> possible for the tests run by the Failsafe Plugin to have a system property
> {{prop}} with a value of {{foo}} if the build happens to already define a
> system property with the same name. While using a different name to avoid a
> clash is possible, it still doesn't make the test self-contained and
> consistent since anyone could run Maven with that other name and compromise
> the test that really relies on the system property having a value of {{foo}}.
> The proposal is thus to make the {{systemPropertyVariables}} and
> {{systemPropertiesFile}} configuration elements of the Surefire and Failsafe
> Plugin take precedence over user properties passed on the command line.
> Proposed commit
> [4de017b38b101b0b28f9fbed135eae3921b99d0d|http://git-wip-us.apache.org/repos/asf/maven-surefire/commit/4de017b3]
> on SUREFIRE-1385 branch.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
