Derby
  1. Derby
  2. DERBY-858

ijConnName test output order is non-deterministic.

    Details

      Description

      The test loads four initial connections using these ij.connection properties in the test's _app.properties file.

      ij.connection.connOne=jdbc:derby:wombat;create=true
      ij.connection.connTwo=jdbc:derby:lemming;create=true
      ij.connection.connThree=jdbc:noone:fruitfly;create=true
      ij.connection.connFour=jdbc:derby:nevercreated

      The order these connections are created, and hence the order of the test output is only determined by the hash table ordering of the properties file. Making small changes in ijConnName_app.properties files can change the order, making the test appear to fail.

      E.g. I tried to enable the test with a security manager by modifying the noSecurityManger to be set to false, and the order changed.

      1. DERBY-858_105.diff
        27 kB
        Myrna van Lunteren
      2. DERBY-858.diff2
        28 kB
        Myrna van Lunteren
      3. DERBY-858.diff
        28 kB
        Myrna van Lunteren

        Activity

        Hide
        Myrna van Lunteren added a comment -

        ij5.sql has the same problem.
        If the hash table ordering is different, this may result in ConnTwo being the first connection made, and so the active connection.

        I think it's ok for these two tests to not have the 2nd valid pre-defined connection. Does anyone disagree?

        Show
        Myrna van Lunteren added a comment - ij5.sql has the same problem. If the hash table ordering is different, this may result in ConnTwo being the first connection made, and so the active connection. I think it's ok for these two tests to not have the 2nd valid pre-defined connection. Does anyone disagree?
        Hide
        Myrna van Lunteren added a comment -

        Attaching a patch for this issue.

        I removed the second connection from the _app.properties file in ijConnName.sql and ij5.sql, (and adjusted comments) and added ijConnName_sed.properties to remove the 08001 and XJ004 errors resulting from connThree and connFour...As a result, I could remove the extra ijConnName and ij5 masters.

        We do lose a tiny bit of information from the test by doing this: an extra 08001 or XJ004 error would go unnoticed, and we don't see the connection 'name' show up in the ij prompt. I think there are other tests that would catch a change in behavior that would cause an extra 08001 or XJ004.

        I found 2 junit tests that had depencies on hashtable ordering: derbynet.SysinfoTest and derbynet.ServerPropertiesTest.
        So I changed the way those tests check for expected values.

        Show
        Myrna van Lunteren added a comment - Attaching a patch for this issue. I removed the second connection from the _app.properties file in ijConnName.sql and ij5.sql, (and adjusted comments) and added ijConnName_sed.properties to remove the 08001 and XJ004 errors resulting from connThree and connFour...As a result, I could remove the extra ijConnName and ij5 masters. We do lose a tiny bit of information from the test by doing this: an extra 08001 or XJ004 error would go unnoticed, and we don't see the connection 'name' show up in the ij prompt. I think there are other tests that would catch a change in behavior that would cause an extra 08001 or XJ004. I found 2 junit tests that had depencies on hashtable ordering: derbynet.SysinfoTest and derbynet.ServerPropertiesTest. So I changed the way those tests check for expected values.
        Hide
        Knut Anders Hatlen added a comment -

        The patch looks fine to me. A couple of tiny issues, though:

        1) I noticed that SysinfoTest.searchMatchingString() uses
        String.matches(String), which takes a regex argument. It looks to me
        as if we're actually looking for exact match here, and not a regex
        match, so it's probably better to use equals() instead.

        2) The error message generated here can be wrong:

        + String ns = actualOutputArray[i];
        + assertTrue("Expected String: " + OUTPUT[i] +
        + "doesn't match actual output: " + ns,
        + searchMatchingString(ns));
        +

        OUTPUT[i] is not necessarily the string that's supposed to match
        actualOutputArray[i], given that the order is JVM dependent (which is
        also why we use searchMatchingString() here and not just
        ns.equals(OUTPUT[i])).

        3) It looks like you forgot to remove one line of code that was
        commented out:

        + String [] OUTPUT1 =

        { + //String OUTPUT1 = + "--------- Derby Network Server Information --------" , 4) A couple of instances in SysinfoTest where the closing braces have been moved to an odd position: - assertEquals(OUTPUT, s); - }


        + assertMatchingStringExists(s); }

        Show
        Knut Anders Hatlen added a comment - The patch looks fine to me. A couple of tiny issues, though: 1) I noticed that SysinfoTest.searchMatchingString() uses String.matches(String), which takes a regex argument. It looks to me as if we're actually looking for exact match here, and not a regex match, so it's probably better to use equals() instead. 2) The error message generated here can be wrong: + String ns = actualOutputArray [i] ; + assertTrue("Expected String: " + OUTPUT [i] + + "doesn't match actual output: " + ns, + searchMatchingString(ns)); + OUTPUT [i] is not necessarily the string that's supposed to match actualOutputArray [i] , given that the order is JVM dependent (which is also why we use searchMatchingString() here and not just ns.equals(OUTPUT [i] )). 3) It looks like you forgot to remove one line of code that was commented out: + String [] OUTPUT1 = { + //String OUTPUT1 = + "--------- Derby Network Server Information --------" , 4) A couple of instances in SysinfoTest where the closing braces have been moved to an odd position: - assertEquals(OUTPUT, s); - } + assertMatchingStringExists(s); }
        Hide
        Myrna van Lunteren added a comment -

        Thanks Knut for the review.
        Attaching a second patch; I believe this addresses your comments.
        I plan to commit this to trunk and backport to 10.5.

        Show
        Myrna van Lunteren added a comment - Thanks Knut for the review. Attaching a second patch; I believe this addresses your comments. I plan to commit this to trunk and backport to 10.5.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks, Myrna, for addressing my comments. The latest patch looks fine.

        Show
        Knut Anders Hatlen added a comment - Thanks, Myrna, for addressing my comments. The latest patch looks fine.
        Hide
        Myrna van Lunteren added a comment -

        Merge to 10.5 was not clean for ServerPropertiesTest and SysinfoTest (I think because of DERBY-4217), so I'm attaching a patch that does the backport. I intend to commit in a couple of hours. tests ran clean.

        Show
        Myrna van Lunteren added a comment - Merge to 10.5 was not clean for ServerPropertiesTest and SysinfoTest (I think because of DERBY-4217 ), so I'm attaching a patch that does the backport. I intend to commit in a couple of hours. tests ran clean.
        Hide
        Myrna van Lunteren added a comment -

        committed to trunk with revision 835153 and backported to 10.5 with revision 835482.
        Interesting svn quirk: the merged added file didn't show up when I did svn diff on 10.5. But it was there with svn stat and it seems to have been correctly added.

        Show
        Myrna van Lunteren added a comment - committed to trunk with revision 835153 and backported to 10.5 with revision 835482. Interesting svn quirk: the merged added file didn't show up when I did svn diff on 10.5. But it was there with svn stat and it seems to have been correctly added.
        Hide
        Myrna van Lunteren added a comment -

        Reopening to address an issue with the SysinfoTest on DOS (i.e. Windows but not using a shell environment like cygwin); it will give a diff like so:
        1) testSysinfoLocale(org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest)junit.framework.AssertionFailedError: expected:<14> but was:<1>
        at org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest.assertMatchingStringExists(SysinfoTest.java:322)
        at org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest.testSysinfoLocale(SysinfoTest.java:249)

        Show
        Myrna van Lunteren added a comment - Reopening to address an issue with the SysinfoTest on DOS (i.e. Windows but not using a shell environment like cygwin); it will give a diff like so: 1) testSysinfoLocale(org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest)junit.framework.AssertionFailedError: expected:<14> but was:<1> at org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest.assertMatchingStringExists(SysinfoTest.java:322) at org.apache.derbyTesting.functionTests.tests.derbynet.SysinfoTest.testSysinfoLocale(SysinfoTest.java:249)
        Hide
        Myrna van Lunteren added a comment -

        closing again; the error for which I reopened looks the same to me as DERBY-4617.
        Marking also closed in 10.3 and 10.4 as I've backported the fix for DERBY-4536.

        Show
        Myrna van Lunteren added a comment - closing again; the error for which I reopened looks the same to me as DERBY-4617 . Marking also closed in 10.3 and 10.4 as I've backported the fix for DERBY-4536 .

          People

          • Assignee:
            Myrna van Lunteren
            Reporter:
            Daniel John Debrunner
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development