Commons Lang
  1. Commons Lang
  2. LANG-727

ToStringBuilderTest.testReflectionHierarchyArrayList fails with IBM JDK 6

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 3.0
    • Fix Version/s: 3.0.1
    • Component/s: lang.builder.*
    • Labels:
      None

      Description

      The unit test fails when running with IBM JDK 6:

      Failed tests: 
        testReflectionHierarchyArrayList(org.apache.commons.lang3.builder.ToStringBuilderTest): null 
      expected:<....ArrayList@115b115b[[elementData={<null>,<null>,<null>,<null>,<null>,<null>,<null>,null>,null>,null>},size=0],modCount=0]>
      but was:<....ArrayList@115b115b[[firstIndex=0,lastIndex=0,array={<null>,<null>,<null>,<null>,<null>,<null>,<null>,null>,null>,null>}],modCount=0]>
      

      Actually the test is wrong, because it makes wrong assumptions about the implementation of ArrayList in the runtime.

        Activity

        Hide
        Henri Yandell added a comment -

        Looking at the code:

                assertEquals(baseStr + "[elementData={<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>},size=0,modCount=0]", ToStringBuilder.reflectionToString(base, null, true));
                assertEquals(baseStr + "[size=0]", ToStringBuilder.reflectionToString(base, null, false));
        

        I think the 2nd test should pass as there's only one item. So just the first one to fix. I think we can use StringUtils.contains, ie:

                String result = ToStringBuilder.reflectionToString(base, null, true);
                assertTrue( StringUtils.startsWith(result, baseStr + "[") );
                assertTrue( StringUtils.contains(result, "[elementData={<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>,<null>}" ) );
                assertTrue( StringUtils.contains(result, "size=0") );
                assertTrue( StringUtils.contains(result, "modCount=0") );
                assertTrue( StringUtils.endsWith(result, "]") );
                assertEquals(baseStr + "[size=0]", ToStringBuilder.reflectionToString(base, null, false));
        

        Is that something you could easily test in JDK 6 and commit?

        I also suspect that we should hard code the elementData size by doing ArrayList(10).

        Show
        Henri Yandell added a comment - Looking at the code: assertEquals(baseStr + "[elementData={< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >},size=0,modCount=0]" , ToStringBuilder.reflectionToString(base, null , true )); assertEquals(baseStr + "[size=0]" , ToStringBuilder.reflectionToString(base, null , false )); I think the 2nd test should pass as there's only one item. So just the first one to fix. I think we can use StringUtils.contains, ie: String result = ToStringBuilder.reflectionToString(base, null , true ); assertTrue( StringUtils.startsWith(result, baseStr + "[" ) ); assertTrue( StringUtils.contains(result, "[elementData={< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >,< null >}" ) ); assertTrue( StringUtils.contains(result, "size=0" ) ); assertTrue( StringUtils.contains(result, "modCount=0" ) ); assertTrue( StringUtils.endsWith(result, "]" ) ); assertEquals(baseStr + "[size=0]" , ToStringBuilder.reflectionToString(base, null , false )); Is that something you could easily test in JDK 6 and commit? I also suspect that we should hard code the elementData size by doing ArrayList(10).
        Hide
        Matt Benson added a comment -

        Alternatively, when we know that we're making assumptions about a class's inner implementation, we could upgrade the test to JUnit 4 and add an assumption at the beginning of the test wrt the particular JVM vendor.

        Show
        Matt Benson added a comment - Alternatively, when we know that we're making assumptions about a class's inner implementation, we could upgrade the test to JUnit 4 and add an assumption at the beginning of the test wrt the particular JVM vendor.
        Hide
        Henri Yandell added a comment -

        It's a bad assumption

        Show
        Henri Yandell added a comment - It's a bad assumption
        Hide
        Matt Benson added a comment -

        right, so the test will be skipped whenever the assumption proves to have been incorrect...

        Show
        Matt Benson added a comment - right, so the test will be skipped whenever the assumption proves to have been incorrect...
        Hide
        Henri Yandell added a comment -

        I've applied the code change I proposed.

        svn ci -m "Rearranging the testReflectionHierarchyArrayList test per LANG-727; shouldn't make assumptions about ArrayList now"
        Sending src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java
        Transmitting file data .
        Committed revision 1153037.

        Show
        Henri Yandell added a comment - I've applied the code change I proposed. svn ci -m "Rearranging the testReflectionHierarchyArrayList test per LANG-727 ; shouldn't make assumptions about ArrayList now" Sending src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java Transmitting file data . Committed revision 1153037.
        Hide
        Joerg Schaible added a comment -

        Sorry to say, but it will still fail. See the error message. On IBM JDK there is no "elementData" member at all.

        Show
        Joerg Schaible added a comment - Sorry to say, but it will still fail. See the error message. On IBM JDK there is no "elementData" member at all.
        Hide
        Joerg Schaible added a comment -

        Actually I am not sure what the test shall demonstrate. Is it a special type construction that is arbitrarily present in ArrayList or does it demonstrate that it can also handle an ArrayList. If it is the latter then we have to check for a different String representation with IBM JDK 6 runtime, because it exposes implementation details of the type.

        Show
        Joerg Schaible added a comment - Actually I am not sure what the test shall demonstrate. Is it a special type construction that is arbitrarily present in ArrayList or does it demonstrate that it can also handle an ArrayList. If it is the latter then we have to check for a different String representation with IBM JDK 6 runtime, because it exposes implementation details of the type.
        Hide
        Matt Benson added a comment -

        I maintain that this test just verifies expectations of the code when running under a certain set of assumptions, including "the ArrayList implementation in use has an elementData member." If we know that that means the Sun JDK, we should just turn the test off under other circumstances. It's not like we don't have the vendor info readily available; this is Commons Lang, after all.

        Show
        Matt Benson added a comment - I maintain that this test just verifies expectations of the code when running under a certain set of assumptions, including "the ArrayList implementation in use has an elementData member." If we know that that means the Sun JDK, we should just turn the test off under other circumstances. It's not like we don't have the vendor info readily available; this is Commons Lang, after all.
        Hide
        Henri Yandell added a comment -

        1) Doh I'll revert the commit.
        2) Happy with your Sun JDK only approach Matt.
        3) I assume the test is to show it can handle ArrayList too.

        Show
        Henri Yandell added a comment - 1) Doh I'll revert the commit. 2) Happy with your Sun JDK only approach Matt. 3) I assume the test is to show it can handle ArrayList too.
        Hide
        Henri Yandell added a comment -

        svn ci -m "Rolling back r1153037 and r1153038 - the changes didn't help"
        Sending src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java
        Transmitting file data .
        Committed revision 1153343.

        Show
        Henri Yandell added a comment - svn ci -m "Rolling back r1153037 and r1153038 - the changes didn't help" Sending src/test/java/org/apache/commons/lang3/builder/ToStringBuilderTest.java Transmitting file data . Committed revision 1153343.
        Hide
        Joerg Schaible added a comment -

        If you postpone this issue, don't forget to drop it again from changes.xml also

        Show
        Joerg Schaible added a comment - If you postpone this issue, don't forget to drop it again from changes.xml also
        Hide
        Matt Benson added a comment -

        I presume the property we'd need to check would be java.vendor as opposed to java.vm.vendor, but I'd be glad for a second opinion. Note that the test also passes on an Apple JDK, so I'd prefer we disable it only on IBM for now until/unless we encounter another classlib that gives us the same problem.

        Joerg, can you give us the java.vendor values for whichever JDKs fail? Or, for example, does the test pass on an earlier IBM JDK? If so, can you give me whatever property values I need to distinguish the environment in which we expect a failure?

        Thanks!

        Show
        Matt Benson added a comment - I presume the property we'd need to check would be java.vendor as opposed to java.vm.vendor , but I'd be glad for a second opinion. Note that the test also passes on an Apple JDK, so I'd prefer we disable it only on IBM for now until/unless we encounter another classlib that gives us the same problem. Joerg, can you give us the java.vendor values for whichever JDKs fail? Or, for example, does the test pass on an earlier IBM JDK? If so, can you give me whatever property values I need to distinguish the environment in which we expect a failure? Thanks!
        Hide
        Joerg Schaible added a comment -

        java.specification.version: 1.6
        java.vm.vendor: IBM Corporation

        IBM JDK 1.5 works. Maybe we can compare the strings and if they're not equal, we look for the JDK. For this one we can print a known issue to sysout, for all others we should fail.

        Show
        Joerg Schaible added a comment - java.specification.version: 1.6 java.vm.vendor: IBM Corporation IBM JDK 1.5 works. Maybe we can compare the strings and if they're not equal, we look for the JDK. For this one we can print a known issue to sysout, for all others we should fail.
        Hide
        Matt Benson added a comment -

        We could do it that way, though it would at least make the test less concise. So is it your opinion that the java.vm.vendor is the important thing here, rather than the java.vendor? I would have thought that java would signify the class library in use while vm would signify only that (the VM). WDYT? What is the value of java.vendor?

        Show
        Matt Benson added a comment - We could do it that way, though it would at least make the test less concise. So is it your opinion that the java.vm.vendor is the important thing here, rather than the java.vendor? I would have thought that java would signify the class library in use while vm would signify only that (the VM). WDYT? What is the value of java.vendor?
        Hide
        Joerg Schaible added a comment -

        java.vendor is the same. Actually we don't have to take this issue too serious. The implementation can handle the ArrayList also for IBM JDK 6. The internals are different but it provides proper information. It's the test that is flaky with its assumptions. Therefore I'd add a comment to state the fact that the result depends on the implementation details of ArrayList that is vendor/version specific. IBM JDK 6 is known for the difference and is currently the only known one with different implementation.

        Show
        Joerg Schaible added a comment - java.vendor is the same. Actually we don't have to take this issue too serious. The implementation can handle the ArrayList also for IBM JDK 6. The internals are different but it provides proper information. It's the test that is flaky with its assumptions. Therefore I'd add a comment to state the fact that the result depends on the implementation details of ArrayList that is vendor/version specific. IBM JDK 6 is known for the difference and is currently the only known one with different implementation.
        Hide
        Joerg Schaible added a comment -

        Committed revision 1154530.

        Show
        Joerg Schaible added a comment - Committed revision 1154530.

          People

          • Assignee:
            Joerg Schaible
            Reporter:
            Joerg Schaible
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development