You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/03/07 16:11:01 UTC
[GitHub] [maven-surefire] rmannibucau opened a new pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
rmannibucau opened a new pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r590002068
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
Yep got it but have to admit I'm mixed about it, I like your setter proposal because it makes the abstract data consistent for any consumer whereas copyProperties can let subclasses/consumers see wrong data which is worse IMHO.
About the utility I'm tempted to say the gain is very light and mainly envisionned because we can't use lambda yet. Since maven master is on java 8 and plugins already started to move I suspect this will drop as soon as we move to java 8 so let's keep it simple maybe?
Wdyt?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589395119
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
Review comment:
It can only for duplicates - it is system properties ;) - and here it is not allowed so all good
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r593718690
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
I don't know what you mean since we said many things.
j8 is in the project due to the JUnit5 but the compiler has target j7, and all the [core plugins](https://maven.apache.org/plugins/) are at j7. We all know that there will be one day when Maven and the core plugins will be alligned with the same java version.
IMO the refactiring where you **reuse and safely move** an important method [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168) to your place where you need to have the fix can be freely done as a good fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#issuecomment-792721126
Pls see the CI logs and fix the chceckstyle:
```
Error: src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:[3628,13] (whitespace) ParenPad: '(' is not followed by whitespace.
Error: src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java:[3628,44] (whitespace) ParenPad: ')' is not preceded with whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2008,41] (whitespace) ParenPad: '(' is not followed by whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2008,54] (whitespace) ParenPad: '(' is not followed by whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2008,69] (whitespace) ParenPad: ')' is not preceded with whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2008,70] (whitespace) ParenPad: ')' is not preceded with whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2009,22] (whitespace) ParenPad: '(' is not followed by whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2009,35] (whitespace) ParenPad: '(' is not followed by whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2009,52] (whitespace) ParenPad: ')' is not preceded with whitespace.
Error: src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java:[2009,88] (whitespace) ParenPad: ')' is not preceded with whitespace.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r593720594
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
About java 8: it is maven 4 time since it is already set to 8 but not v3, agree.
About why setter is saner than any other "flow" method: because class is public+abstract and getter as well so it is part of the contract of the class to convert it in the setter - as you said on slack - and not later on in the flow which can be missed by the contract. So we actually don't have much choice. We can create an utility but as mentionned I think it is worse than duplicating the inline test IMHO so think we are good.
Now if you want to fix it differently I'll not fight while it behaves the same.
side note: i'll not get time this week end/start of next week to modify this PR and can be slow to answer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589386968
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
Review comment:
Does the order of the entries matter? Let's see what the ITs would say.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589384582
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
See how the getter is used and how the null check looks like. I think it is something like `val == null ? "" : val`. It would be nice to extract this treatment or even more code to a private static method and call it from both places.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r593718690
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
I don't know what you mean since we said many things.
j8 is in the project due to the JUnit5 but the compiler ha target j7, and all the [core plugins](https://maven.apache.org/plugins/) are at j7. We all know that there will be one day when Maven and the core plugins will be alligned with the same java version.
IMO the refactiring where you **reuse and safely move** an important method [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168) to your place where you need to have the fix can be freely done as a good fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589778798
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
@rmannibucau
I was talking [about](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L175).
Due to the method [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168) is called only in [calculateEffectiveProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L148) which is called in ASM, then the solution is to move both methods to ASM because they are used only there, and then your method can call [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168). You have to only change `Properties` to `Map` without generics. The method `copyProperties` takes your `systemPropertyVariables` to the second parameter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589394853
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
+ }
+ }
+ else
Review comment:
it is an abstract class and a public method, we can't assume it I guess so even if you are supposed to be true, there is no reason it is true in practise so thought being defensive was saner.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r592500005
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
Right, but once again it is not the main reason to not fix it there as explained.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r592486479
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
Even if the utility class is public, still the method `calculateEffectiveProperties` is package private, and `copyProperties` is private.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589385444
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
+ }
+ }
+ else
Review comment:
The "else" is not needed. The setter is called and not the field, so there is only one entry point, i.e. the setter. Or?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r592172934
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
Understood but it does not fix the issue that the mojo exposed view is wrong for some time in between until the utility is public which is worse than these 2 lines IMHO so not a bad compromise probably.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r592161135
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
Both methods will be private and they have a commpon purpose - moving properties from one Map to second Map. Of course j8 may turn loops to streams but that's a big hammer for this little refactoring.
I would be glad when Maven aligns all default Maven plugins to j8 after Maven 4.0, and then all the stack would be on the same j8. IMO j9 would be even a bigger benefit for the Maven but not for users yet ;-).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] rmannibucau closed pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
rmannibucau closed pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340
--
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-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #340: [SUREFIRE-1892] ensure systemPropertyVariables values are stringified
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #340:
URL: https://github.com/apache/maven-surefire/pull/340#discussion_r589778798
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -3623,9 +3623,20 @@ public void setSystemProperties( Properties systemProperties )
}
@SuppressWarnings( "UnusedDeclaration" )
- public void setSystemPropertyVariables( Map<String, String> systemPropertyVariables )
+ public void setSystemPropertyVariables( Map<String, ?> systemPropertyVariables )
{
- this.systemPropertyVariables = systemPropertyVariables;
+ if (systemPropertyVariables != null)
+ {
+ this.systemPropertyVariables = new HashMap<>();
+ for ( final Map.Entry<String, ?> entry : systemPropertyVariables.entrySet() )
+ {
+ this.systemPropertyVariables.put( entry.getKey(), String.valueOf( entry.getValue() ) );
Review comment:
@rmannibucau
I was talking [about](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L175).
Due to the method [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168) is called only in [calculateEffectiveProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L148) which is called in ASM, then the solution is to move both methods to ASM because they are used only there, and then your method can call [copyProperties](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java#L168). You have to only change `Properties` to `Map` withou generics. The method `copyProperties` takes your `systemPropertyVariables` to the second parameter.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org