[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user asfgit closed the pull request at: https://github.com/apache/commons-lang/pull/269 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120550160
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +317,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new
ArrayList<>(arraylistInitialCapacity);
--- End diff --
Roger that. Will add a note to myself to fix the other final member
variables later... trying to be concise, but I'm clearly missing the point here
:-) was supposedly to be a very simple fix for this issue. Pushing a new commit
in a few minutes, just finishing to review commons-fileupload vote.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user britter commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120549733
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +317,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new
ArrayList<>(arraylistInitialCapacity);
--- End diff --
Sorry to be nitpicking, but this should be a constant and wie should add a
comment referencing JIRA-1337 with an explanation why we need to pass the
initial capacity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120546260
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +316,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new ArrayList<>(10);
--- End diff --
>If the test fails when the initial size arg is omitted, does that not also
affect the behaviour of the method being tested?
Not really. The test simply checks the string built for an arraylist
through reflection. The issue was caused for believing that the lazy
initialization (as @andyklimczak) would work in the same independent of the JVM.
What the test is verifying is correct, the current approach could be
improved to make the test less flaky.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user kinow commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120545692
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +316,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new ArrayList<>(10);
--- End diff --
Fair enough on the magic number. I'd thought about that, then noticed a few
other tests with numbers. But one broken window doesn't mean I can break
another one :-) fixing in another commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user sebbASF commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120390348
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +316,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new ArrayList<>(10);
--- End diff --
Thanks!
If the test fails when the initial size arg is omitted, does that not also
affect the behaviour of the method being tested? i.e. do apps also have to
ensure that they specify the min size when using
ToStringBuilder.reflectionToString() ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user andyklimczak commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120386094
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +316,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new ArrayList<>(10);
--- End diff --
probably related to [this](https://stackoverflow.com/a/34250231)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
Github user sebbASF commented on a diff in the pull request:
https://github.com/apache/commons-lang/pull/269#discussion_r120385458
--- Diff:
src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java ---
@@ -316,7 +316,7 @@ public void testReflectionHierarchyArrayList() {
// representation different for IBM JDK 1.6.0, LANG-727
assumeFalse("IBM Corporation".equals(SystemUtils.JAVA_VENDOR) &&
"1.6".equals(SystemUtils.JAVA_SPECIFICATION_VERSION));
assumeFalse("Oracle Corporation".equals(SystemUtils.JAVA_VENDOR)
&& "1.6".compareTo(SystemUtils.JAVA_SPECIFICATION_VERSION) < 0);
-final List list = new ArrayList<>();
+final List list = new ArrayList<>(10);
--- End diff --
I think that needs a comment.
Is the magic number 10 significant?
If so, what determines the value?
Could it ever change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] commons-lang pull request #269: LANG-1337: Fix test failures in IBM JDK 8 fo...
GitHub user kinow opened a pull request: https://github.com/apache/commons-lang/pull/269 LANG-1337: Fix test failures in IBM JDK 8 for ToStringBuilderTest by specifying the ArrayList initial capacity. See https://issues.apache.org/jira/browse/LANG-1337 for issue description and explanation of the changes here. In summary, we specify the ArrayList's initial capacity, this way the String built through reflection has the expected value. Tested with Oracle JDK 7 and 8, and IBM JDK 8. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kinow/commons-lang LANG-1337 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/commons-lang/pull/269.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 #269 commit 65b08c4f91a4e5d78eb645b46d06d7a46f8c62dd Author: Bruno P. Kinoshita Date: 2017-06-06T11:41:31Z LANG-1337: Fix test failures in IBM JDK 8 for ToStringBuilderTest by specifying the ArrayList initial capacity. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
