[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite #762

kwin · 2024-07-15T13:03:52Z

user properties

Log overwritten properties
Clarify effective properties merging order

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

user properties Log overwritten properties Clarify effective properties merging order

michael-o

Did you have if we have any documentation in site which needs updating?

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

michael-o · 2024-07-15T16:05:30Z

Will happily review...

slawekjaranowski · 2024-07-15T16:22:47Z

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

+     * @see #systemPropertyVariables
+     */
+    @Parameter
+    Map<String, String> userPropertyVariables;


From test perspective there are system properties - not a user properties.

Next parameter like this can confusing user and will be used in random.

I would like to consider changing a priority for systemPropertyVariables,
even more user can take old behavior by settings properties in project and use it in configuration.

Maybe we should don't pass a MavenSession#getUserProperties() by default to tests ....

I would like to consider changing a priority for systemPropertyVariables,
...
Maybe we should don't pass a MavenSession#getUserProperties() by default to tests ....

Both approaches will potentially trigger backwards compatibility issues for certain edge cases. I don't think we can do that for a 3.x release.

Any suggestion for another name? As I said above, I don't think we can fix this in a backwards-compatible way without introducing another parameter...

@kwin The let's make it right -- after all -- with a deprecated param with retains old behavior. Surefire 4 is far away, this issue has been open for too long.

let's make it right

What is right? As outlined in https://issues.apache.org/jira/browse/SUREFIRE-1385?focusedCommentId=17864170&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17864170 there is no simple solution IMHO:
The default should still be to allow overwrites with CLI arguments. Overwriting CLI arguments with plugin parameters is the edge case which we need to support on an exceptional base. I don't see a simple solution without adding a param. Please make a proposal what you consider right here.

Not sure how this helps here. Let us focus on how you think it should work!

What about a boolean parameter includeUserPropertiesInSystemProperties which is true by default (for backwards compatibility). If set to false this will prevent passing on the user properties to the provider at all. This will make the addition of the userPropertyVariables parameter obsolete from my perspective.

I implemented the aforementioned approach now in f8b0f57.

OK, I will try to dedicate some to it soon.

@slawekjaranowski I agree with you that passing user properties by default is a bad idea.

the default is true for being backwards compatible

kwin · 2024-07-19T15:59:31Z

Did you have if we have any documentation in site which needs updating?

Maybe https://maven.apache.org/surefire/maven-surefire-plugin/examples/system-properties.html, but nothing in it becomes incorrect, but maybe worth to mention there the additional parameter.

michael-o · 2024-07-23T20:12:49Z

I read the JIRA description and this PR. My first problem: Where is the documentation that user properties are passed to Surefire at all? Here: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#systemPropertyVariables I see zero of the Javadoc. That is a serious problem for starters...

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

kwin · 2024-07-23T20:28:41Z

I already extended the javadoc in cd72eaa (you reviewed and approved) but for me this is only remotely related to this PR.

michael-o · 2024-07-23T20:30:20Z

I already extended the javadoc in cd72eaa (you reviewed and approved) but for me this is only remotely related to this PR.

Ah, yes now I see. This makes sense. It was merely a side comment.

michael-o

One nit, but the rest is fine. At someone else should have a look at this too.

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

michael-o · 2024-08-02T14:16:06Z

I guess @slawekjaranowski is on vacation. Let's leave him sometime to review.

slawekjaranowski

Looks ok, thanks

kwin requested review from michael-o and slawekjaranowski July 15, 2024 13:03

[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite

476b18a

user properties Log overwritten properties Clarify effective properties merging order

kwin force-pushed the feature/override-user-properties branch from 865e46d to 476b18a Compare July 15, 2024 15:57

michael-o requested changes Jul 15, 2024

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java Outdated Show resolved Hide resolved

slawekjaranowski reviewed Jul 15, 2024

View reviewed changes

switch to boolean toggle to use user properties as system properties

f8b0f57

the default is true for being backwards compatible

michael-o requested changes Jul 23, 2024

View reviewed changes

kwin added 2 commits July 30, 2024 15:26

Apply suggestions from PR review

f21ac96

Extend documentation

1afafc0

kwin force-pushed the feature/override-user-properties branch from 09ec17a to 1afafc0 Compare July 30, 2024 13:53

michael-o approved these changes Jul 30, 2024

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java Show resolved Hide resolved

Add missing newline

4883617

slawekjaranowski approved these changes Aug 6, 2024

View reviewed changes

kwin merged commit 4bd36a1 into master Aug 7, 2024
39 checks passed

kwin deleted the feature/override-user-properties branch August 7, 2024 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite #762

[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite #762

kwin commented Jul 15, 2024

michael-o left a comment

michael-o commented Jul 15, 2024

slawekjaranowski Jul 15, 2024

kwin Jul 15, 2024 •

edited

Loading

kwin Jul 15, 2024

michael-o Jul 15, 2024

kwin Jul 15, 2024 •

edited

Loading

kwin Jul 15, 2024

kwin Jul 19, 2024 •

edited

Loading

kwin Jul 19, 2024

michael-o Jul 20, 2024

michael-o Jul 23, 2024

kwin commented Jul 19, 2024

michael-o commented Jul 23, 2024

kwin commented Jul 23, 2024

michael-o commented Jul 23, 2024

michael-o left a comment

michael-o commented Aug 2, 2024

slawekjaranowski left a comment

[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite #762

[SUREFIRE-1385] Add new parameter "userPropertyVariables" to overwrite #762

Conversation

kwin commented Jul 15, 2024

michael-o left a comment

Choose a reason for hiding this comment

michael-o commented Jul 15, 2024

slawekjaranowski Jul 15, 2024

Choose a reason for hiding this comment

kwin Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

kwin Jul 15, 2024

Choose a reason for hiding this comment

michael-o Jul 15, 2024

Choose a reason for hiding this comment

kwin Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

kwin Jul 15, 2024

Choose a reason for hiding this comment

kwin Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

kwin Jul 19, 2024

Choose a reason for hiding this comment

michael-o Jul 20, 2024

Choose a reason for hiding this comment

michael-o Jul 23, 2024

Choose a reason for hiding this comment

kwin commented Jul 19, 2024

michael-o commented Jul 23, 2024

kwin commented Jul 23, 2024

michael-o commented Jul 23, 2024

michael-o left a comment

Choose a reason for hiding this comment

michael-o commented Aug 2, 2024

slawekjaranowski left a comment

Choose a reason for hiding this comment

kwin Jul 15, 2024 •

edited

Loading

kwin Jul 15, 2024 •

edited

Loading

kwin Jul 19, 2024 •

edited

Loading