Derby
  1. Derby
  2. DERBY-3273

convert derbynet/testconnection.java to junit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.4.1.3
    • Fix Version/s: 10.4.1.3
    • Component/s: Test
    • Labels:
      None
    • Issue & fix info:
      Patch Available

      Description

      convert derbynet/testconnection.java to junit.

      1. derby-3273_diff.txt
        22 kB
        Kathey Marsden
      2. derby-3273_diff2.txt
        24 kB
        Kathey Marsden

        Issue Links

          Activity

          Hide
          Kathey Marsden added a comment -

          Attached is the conversion of derbynet/testconnection.java to junit.

          Added ping testing with the api to NetworkServerControlApiTest

          Created a new test NetworkServerControlClientCommandTest to test the command line ping testing. Command line testing for runtimeinfo, sysinfo etc can be added to this file when converted.

          I have a question about portability. I think that because the process input stream will use the platform default encoding as will the String constructor, my code comparing the command output will be ok, but I would appreciate a second opinion.

          Kathey

          Show
          Kathey Marsden added a comment - Attached is the conversion of derbynet/testconnection.java to junit. Added ping testing with the api to NetworkServerControlApiTest Created a new test NetworkServerControlClientCommandTest to test the command line ping testing. Command line testing for runtimeinfo, sysinfo etc can be added to this file when converted. I have a question about portability. I think that because the process input stream will use the platform default encoding as will the String constructor, my code comparing the command output will be ok, but I would appreciate a second opinion. Kathey
          Hide
          Myrna van Lunteren added a comment -

          I have no reply to your question (looks ok to me, but then I'm working on English systems also), but I have two comments:

          • although it's negative testing, maybe you should use the InetAddress.getByName(
            TestConfiguration.getCurrent().getHostName()),
            TestConfiguration.getCurrent().getPort());
            instead of hard-coded localhost and 1527.
          • although the jvm executable for J2ME may not be 'java' I wouldn't say it's "not right". Just different.
          Show
          Myrna van Lunteren added a comment - I have no reply to your question (looks ok to me, but then I'm working on English systems also), but I have two comments: although it's negative testing, maybe you should use the InetAddress.getByName( TestConfiguration.getCurrent().getHostName()), TestConfiguration.getCurrent().getPort()); instead of hard-coded localhost and 1527. although the jvm executable for J2ME may not be 'java' I wouldn't say it's "not right". Just different.
          Hide
          Daniel John Debrunner added a comment -

          Or even:

          NetworkServerTestSetup.getNetworkServerControl()

          which returns a correctly setup NetworkServerControl object for the current configuration including handling SSL.

          Show
          Daniel John Debrunner added a comment - Or even: NetworkServerTestSetup.getNetworkServerControl() which returns a correctly setup NetworkServerControl object for the current configuration including handling SSL.
          Hide
          Daniel John Debrunner added a comment -

          In the Utilities.execCmd() if Runtime.getRuntime().exec(command) throws an exception a NPE will result.

          I think the exception should simply be thrown which will cause the test to fail.

          In the same method:

          + if (pr.exitValue() != expectedExitCode)

          { + BaseTestCase.fail("FAIL: expected exit code of " + expectedExitCode + + ", got exit code of " + pr.exitValue()); + }

          any reason not to use Assert.assertEquals()?

          and

          + int j = 0;
          + for (int i = 3; i < totalSize; i++)

          { + cmd[i] = args[j++]; + }

          any reason not to use System.arraycopy()?

          Putting this exec in Utilities is good, it could be the base to fix DERBY-3250 since the new method handles multiple arguments correctly.

          Show
          Daniel John Debrunner added a comment - In the Utilities.execCmd() if Runtime.getRuntime().exec(command) throws an exception a NPE will result. I think the exception should simply be thrown which will cause the test to fail. In the same method: + if (pr.exitValue() != expectedExitCode) { + BaseTestCase.fail("FAIL: expected exit code of " + expectedExitCode + + ", got exit code of " + pr.exitValue()); + } any reason not to use Assert.assertEquals()? and + int j = 0; + for (int i = 3; i < totalSize; i++) { + cmd[i] = args[j++]; + } any reason not to use System.arraycopy()? Putting this exec in Utilities is good, it could be the base to fix DERBY-3250 since the new method handles multiple arguments correctly.
          Hide
          Daniel John Debrunner added a comment -

          In NetworkServerControlApiTest

          + public void testPing() throws Exception
          + {
          + NetworkServerControl nsctrl = new NetworkServerControl();
          + nsctrl.ping();
          +

          This code has the problem that it assumes the default of localhost and 1527 matches the current configuration of the test which may not be true.
          Existing tests in NetworkServerControlApiTest have this problem, as Myrna points out the NetworkServerControl should be pulled based on the configuration.

          + try {
          + nsctrl = new NetworkServerControl(InetAddress.getByName("notthere"), 1527);
          + nsctrl.ping();
          + fail("Should not have been able to ping host \"notthere\"");

          I think this is a dangerous coding style for tests, which method call are you expecting to fail? Only the method expected to fail should be in the try catch block.

          Show
          Daniel John Debrunner added a comment - In NetworkServerControlApiTest + public void testPing() throws Exception + { + NetworkServerControl nsctrl = new NetworkServerControl(); + nsctrl.ping(); + This code has the problem that it assumes the default of localhost and 1527 matches the current configuration of the test which may not be true. Existing tests in NetworkServerControlApiTest have this problem, as Myrna points out the NetworkServerControl should be pulled based on the configuration. + try { + nsctrl = new NetworkServerControl(InetAddress.getByName("notthere"), 1527); + nsctrl.ping(); + fail("Should not have been able to ping host \"notthere\""); I think this is a dangerous coding style for tests, which method call are you expecting to fail? Only the method expected to fail should be in the try catch block.
          Hide
          Kathey Marsden added a comment -

          Dan said...
          >+ try {
          >+ nsctrl = new >NetworkServerControl(InetAddress.getByName("notthere"), 1527);
          >+ nsctrl.ping();
          >+ fail("Should not have been able to ping host >\"notthere\"");

          >I think this is a dangerous coding style for tests, which >method call are you expecting to fail? Only the method >expected to fail should be in the try catch block

          In fact the failure was happening in the InetAddress.getByName("notthere"), call, so we never got to the ping. It was a security exception, but when I added the permissions and privilege block, it failed with an UnknownHost exception, so the test itself does not make much sense for ping with the API. I'll take the unknown host test out of testPing.

          Show
          Kathey Marsden added a comment - Dan said... >+ try { >+ nsctrl = new >NetworkServerControl(InetAddress.getByName("notthere"), 1527); >+ nsctrl.ping(); >+ fail("Should not have been able to ping host >\"notthere\""); >I think this is a dangerous coding style for tests, which >method call are you expecting to fail? Only the method >expected to fail should be in the try catch block In fact the failure was happening in the InetAddress.getByName("notthere"), call, so we never got to the ping. It was a security exception, but when I added the permissions and privilege block, it failed with an UnknownHost exception, so the test itself does not make much sense for ping with the API. I'll take the unknown host test out of testPing.
          Hide
          Kathey Marsden added a comment -

          A new patch addressing Myrna's and Dan's comments.

          Show
          Kathey Marsden added a comment - A new patch addressing Myrna's and Dan's comments.
          Hide
          Daniel John Debrunner added a comment -

          Can you comment what's going on here?

          + String currentHost = TestConfiguration.getCurrent().getHostName();
          + int currentPort = TestConfiguration.getCurrent().getPort();
          + NetworkServerControl nsctrl = NetworkServerTestSetup.getNetworkServerControl();
          + nsctrl.ping();
          + nsctrl = new NetworkServerControl(privInetAddressGetByName(currentHost),
          + currentPort);
          + nsctrl.ping();

          What's different about the second ping that requires additional testing?

          Seems to me that if SSL is not enabled then the two pings are testing the same logic, if SSL is enabled then won't the second ping fail?

          Show
          Daniel John Debrunner added a comment - Can you comment what's going on here? + String currentHost = TestConfiguration.getCurrent().getHostName(); + int currentPort = TestConfiguration.getCurrent().getPort(); + NetworkServerControl nsctrl = NetworkServerTestSetup.getNetworkServerControl(); + nsctrl.ping(); + nsctrl = new NetworkServerControl(privInetAddressGetByName(currentHost), + currentPort); + nsctrl.ping(); What's different about the second ping that requires additional testing? Seems to me that if SSL is not enabled then the two pings are testing the same logic, if SSL is enabled then won't the second ping fail?
          Hide
          Kathey Marsden added a comment -

          of course you are right, the two commands end up testing the same thing. I will remove the second.

          Show
          Kathey Marsden added a comment - of course you are right, the two commands end up testing the same thing. I will remove the second.
          Hide
          Ole Solberg added a comment -

          Looks like the 604074 checkin was incomplete: old test still referenced in derbynetmats and j9derbynetmats:

          ./org/apache/derbyTesting/functionTests/suites/derbynetmats.runall:10:derbynet/testconnection.java
          ./org/apache/derbyTesting/functionTests/suites/j9derbynetmats.runall:6:derbynet/testconnection.java

          See
          http://dbtg.thresher.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/604075-derbyall_diff.txt

          > Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/derbyTesting/functionTests/tests/derbynet/testconnection

          Show
          Ole Solberg added a comment - Looks like the 604074 checkin was incomplete: old test still referenced in derbynetmats and j9derbynetmats: ./org/apache/derbyTesting/functionTests/suites/derbynetmats.runall:10:derbynet/testconnection.java ./org/apache/derbyTesting/functionTests/suites/j9derbynetmats.runall:6:derbynet/testconnection.java See http://dbtg.thresher.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/604075-derbyall_diff.txt > Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/derbyTesting/functionTests/tests/derbynet/testconnection
          Hide
          Andrew McIntyre added a comment -

          Removed testconnection from old harness suites with revision 604123.

          Show
          Andrew McIntyre added a comment - Removed testconnection from old harness suites with revision 604123.

            People

            • Assignee:
              Kathey Marsden
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development