You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@maven.apache.org by acogoluegnes <gi...@git.apache.org> on 2017/11/27 13:57:53 UTC
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
GitHub user acogoluegnes opened a pull request:
https://github.com/apache/maven-surefire/pull/171
[SUREFIRE-1445] Explicitly define SurefireProperties#putAll
This ensures the overriden put method is called and the items property is updated.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/acogoluegnes/maven-surefire SUREFIRE-1445
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/maven-surefire/pull/171.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #171
----
commit 163077f3b9844b94f931ee8cfe573be97b8437bc
Author: Arnaud Cogoluègnes <ac...@gmail.com>
Date: 2017-11-27T13:54:30Z
[SUREFIRE-1445] Explicitly define SurefireProperties#putAll
This ensures the overriden put method is called and the items
property is updated.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by acogoluegnes <gi...@git.apache.org>.
Github user acogoluegnes commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r154307933
--- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefirePropertiesTest.java ---
@@ -70,9 +70,35 @@ public void testConstructWithOther()
src.setProperty( "b", "2" );
SurefireProperties orderedProperties = new SurefireProperties( src );
// Cannot make assumptions about insertion order
- assertEquals( 2, orderedProperties.size() );
-
+ int expectedCount = 0;
+ // keys() uses the items property, more reliable to test than size(),
+ // which is based on the Properties class
+ // see https://issues.apache.org/jira/browse/SUREFIRE-1445
+ Enumeration<Object> keys = orderedProperties.keys();
+ while ( keys.hasMoreElements() ) {
+ keys.nextElement();
+ expectedCount++;
+ }
+ assertEquals( 2, expectedCount );
--- End diff --
Got it, done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153652056
--- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java ---
@@ -71,13 +71,22 @@ public SurefireProperties( KeyValueSource source )
}
}
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
+ for (Map.Entry<?, ?> entry : t.entrySet()) {
+ this.put(entry.getKey(), entry.getValue());
--- End diff --
The constructor also has "this.". Please remove it as well. Thx.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153654392
--- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java ---
@@ -71,13 +71,22 @@ public SurefireProperties( KeyValueSource source )
}
}
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
+ for (Map.Entry<?, ?> entry : t.entrySet()) {
+ this.put(entry.getKey(), entry.getValue());
+ }
+ }
+
@Override
public synchronized Object put( Object key, Object value )
{
items.add( key );
return super.put( key, value );
}
+
+
--- End diff --
Here only one empty line between two methods. Feel free to remove these two you have added. Thx.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153652515
--- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefirePropertiesTest.java ---
@@ -70,9 +70,16 @@ public void testConstructWithOther()
src.setProperty( "b", "2" );
SurefireProperties orderedProperties = new SurefireProperties( src );
// Cannot make assumptions about insertion order
- assertEquals( 2, orderedProperties.size() );
-
-
+ int expectedCount = 0;
+ // keys() uses the items property, more reliable to test than size(),
+ // which is based on the Properties class
+ // see https://issues.apache.org/jira/browse/SUREFIRE-1445
+ Enumeration<Object> keys = orderedProperties.keys();
+ while (keys.hasMoreElements()) {
+ keys.nextElement();
+ expectedCount++;
+ }
+ assertEquals( 2, expectedCount );
--- End diff --
Can you do the same assert with `getStringKeySet()`?
This is fine you added but can you add a new test explicitly calling putAll()?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire issue #171: [SUREFIRE-1445] Explicitly define SurefirePropert...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on the issue:
https://github.com/apache/maven-surefire/pull/171
@acogoluegnes
Thx for support regarding Java 9.
Please close this PR. I extended the test and already pushed to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r154193802
--- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefirePropertiesTest.java ---
@@ -70,9 +70,35 @@ public void testConstructWithOther()
src.setProperty( "b", "2" );
SurefireProperties orderedProperties = new SurefireProperties( src );
// Cannot make assumptions about insertion order
- assertEquals( 2, orderedProperties.size() );
-
+ int expectedCount = 0;
+ // keys() uses the items property, more reliable to test than size(),
+ // which is based on the Properties class
+ // see https://issues.apache.org/jira/browse/SUREFIRE-1445
+ Enumeration<Object> keys = orderedProperties.keys();
+ while ( keys.hasMoreElements() ) {
+ keys.nextElement();
+ expectedCount++;
+ }
+ assertEquals( 2, expectedCount );
--- End diff --
We missed one method I mentioned before. Can you add an assert that method `getStringKeySet()` returns same elements like it is in `keys()`. Maybe this will help:
`java.util.Collections#list(Enumeration): List`
We can do it in both tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire issue #171: [SUREFIRE-1445] Explicitly define SurefirePropert...
Posted by acogoluegnes <gi...@git.apache.org>.
Github user acogoluegnes commented on the issue:
https://github.com/apache/maven-surefire/pull/171
@Tibor17 Thanks for reviewing! I pushed a new version.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153650439
--- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java ---
@@ -71,13 +71,22 @@ public SurefireProperties( KeyValueSource source )
}
}
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
--- End diff --
We have different conventions. Please see the spaces around brackets and new lines.
The checkstyle plugin would crash the build. Did you try to run "mvn install -P run-its"?
It would take quite long time to complete the build, cca one hour.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by acogoluegnes <gi...@git.apache.org>.
Github user acogoluegnes commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153745052
--- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/SurefirePropertiesTest.java ---
@@ -70,9 +70,16 @@ public void testConstructWithOther()
src.setProperty( "b", "2" );
SurefireProperties orderedProperties = new SurefireProperties( src );
// Cannot make assumptions about insertion order
- assertEquals( 2, orderedProperties.size() );
-
-
+ int expectedCount = 0;
+ // keys() uses the items property, more reliable to test than size(),
+ // which is based on the Properties class
+ // see https://issues.apache.org/jira/browse/SUREFIRE-1445
+ Enumeration<Object> keys = orderedProperties.keys();
+ while (keys.hasMoreElements()) {
+ keys.nextElement();
+ expectedCount++;
+ }
+ assertEquals( 2, expectedCount );
--- End diff --
I don't get it. I used `keys()` on purpose because it relies on `items`, which was not in-sync in the first place. `getStringKeySet()` relies `keySet()`, which uses the base class. Both would work, but `getStringKeySet()` doesn't catch the bug. Does that make sense?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153650568
--- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java ---
@@ -71,13 +71,22 @@ public SurefireProperties( KeyValueSource source )
}
}
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
+ for (Map.Entry<?, ?> entry : t.entrySet()) {
+ this.put(entry.getKey(), entry.getValue());
--- End diff --
"this." is not necessary.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by Tibor17 <gi...@git.apache.org>.
Github user Tibor17 commented on a diff in the pull request:
https://github.com/apache/maven-surefire/pull/171#discussion_r153653384
--- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireProperties.java ---
@@ -71,13 +71,22 @@ public SurefireProperties( KeyValueSource source )
}
}
+ @Override
+ public synchronized void putAll(Map<?, ?> t) {
--- End diff --
Please keep using one commir in PR.
I recommend you to do `git commit--amend` and then forced push `git push origin +master`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org
[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...
Posted by acogoluegnes <gi...@git.apache.org>.
Github user acogoluegnes closed the pull request at:
https://github.com/apache/maven-surefire/pull/171
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@maven.apache.org
For additional commands, e-mail: dev-help@maven.apache.org