[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...

Tcharl
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 <[hidden email]>
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...

Tcharl
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...

Tcharl
In reply to this post by Tcharl
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #171: [SUREFIRE-1445] Explicitly define SurefirePropert...

Tcharl
In reply to this post by Tcharl
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #171: [SUREFIRE-1445] Explicitly define Surefire...

Tcharl
In reply to this post by Tcharl
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #171: [SUREFIRE-1445] Explicitly define SurefirePropert...

Tcharl
In reply to this post by Tcharl
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: [hidden email]
For additional commands, e-mail: [hidden email]