Derby
  1. Derby
  2. DERBY-5084

convert ijConnName.sql to a ScriptTest junit test

    Details

    • Urgency:
      Low

      Description

      In DERBY-3089 it is mentioned that tools/ijConnName.sql should get converted to a junit test, probably using ScriptTest.

      Making a separate issue so that that issue can be closed.

      1. ijConnName-1.tmp
        2 kB
        Houx Zhang
      2. ijConnName-1.out
        3 kB
        Houx Zhang
      3. DERBY-5084-9.patch
        4 kB
        Houx Zhang
      4. derby-5084-8.patch
        4 kB
        Houx Zhang
      5. derby-5084-7.patch
        3 kB
        Houx Zhang
      6. derby-5084-6.patch
        3 kB
        Houx Zhang
      7. DERBY-5084-3.stat
        11 kB
        Houx Zhang
      8. DERBY-5084-3.patch
        11 kB
        Houx Zhang
      9. DERBY-5084-2.patch
        10 kB
        Houx Zhang
      10. DERBY-5084-1.patch
        2 kB
        Houx Zhang
      11. DERBY-5084.patch
        3 kB
        Houx Zhang
      12. DERBY-5084_5.diff
        11 kB
        Myrna van Lunteren
      13. DERBY-5084_4.diff
        11 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Hide
          Houx Zhang added a comment -

          I'm interested in this issue, and I think it's a good point for a new comer. I will read some infomation about Derby testing, and hope to attach a patch some days later.

          Show
          Houx Zhang added a comment - I'm interested in this issue, and I think it's a good point for a new comer. I will read some infomation about Derby testing, and hope to attach a patch some days later.
          Hide
          Houx Zhang added a comment -

          Ask for help!

          I have provided an attach for this issue. However, it hasn't passed yet. I have some questions about it:

          1. I have changed ijConnName.out by adding a empty line, as it's obviously right. It seems, the sql test hasn't been runned for a long time. Is it so?

          2. When running my test case extending ScriptTestcase.java, the test failed in "CONNONE* - jdbc:derby:wombat" by "CONNECTION1* - jdbc:derby:wombat". I

          have setting system prompties with SystemPropertyTestSetup, however it seems making no effect. So, I wonder how to set the properties in junit Testcase just

          like in ijConnName_app.properties.

          Thanks for your advice!

          Show
          Houx Zhang added a comment - Ask for help! I have provided an attach for this issue. However, it hasn't passed yet. I have some questions about it: 1. I have changed ijConnName.out by adding a empty line, as it's obviously right. It seems, the sql test hasn't been runned for a long time. Is it so? 2. When running my test case extending ScriptTestcase.java, the test failed in "CONNONE* - jdbc:derby:wombat" by "CONNECTION1* - jdbc:derby:wombat". I have setting system prompties with SystemPropertyTestSetup, however it seems making no effect. So, I wonder how to set the properties in junit Testcase just like in ijConnName_app.properties. Thanks for your advice!
          Hide
          Kathey Marsden added a comment -

          Hello Houx. Thank you for contributing to derby.

          For 1) Adding the blank line is fine. This is covered in the http://wiki.apache.org/db-derby/ConvertSqlScriptTestsToJunit page under Test fails. Because the old harness did a sed of the output it can have white space differences.

          Part of the final patch will need to be to take the test out of the old harness which will mean at least taking it out of suites/tools.runall and taking out the ijConnName*properties files.

          I am not sure about the SystemProperty problem. I wonder if ScriptTestCase sets up its own connection that interferes?

          Show
          Kathey Marsden added a comment - Hello Houx. Thank you for contributing to derby. For 1) Adding the blank line is fine. This is covered in the http://wiki.apache.org/db-derby/ConvertSqlScriptTestsToJunit page under Test fails. Because the old harness did a sed of the output it can have white space differences. Part of the final patch will need to be to take the test out of the old harness which will mean at least taking it out of suites/tools.runall and taking out the ijConnName*properties files. I am not sure about the SystemProperty problem. I wonder if ScriptTestCase sets up its own connection that interferes?
          Hide
          Kathey Marsden added a comment -

          I just noticed there is a ScriptTestCase(String script, boolean useSystemProperties) constructor. Maybe us that one instead?

          Show
          Kathey Marsden added a comment - I just noticed there is a ScriptTestCase(String script, boolean useSystemProperties) constructor. Maybe us that one instead?
          Hide
          Houx Zhang added a comment -

          Thanks very much, Kathey. Your advice is helpful.

          ScriptTestCase(String script, boolean useSystemProperties) constructor can work as you advised. With setting it true, the System Properties are set up OK.

          However, my junit test case can not pass, while the harness.RunTest can pass. I have attached the file "ijConnName-1.tmp", which is created by harness.RunTest, and the file "ijConnName-1.out", created by my junit test case. In the file "ijConnName-1.out", we can see,

          "
          show connections;
          CONNECTION0* - jdbc:derby:wombat
          CONNONE - jdbc:derby:wombat
          "

          A connection with the name "CONNECTION0" has been created before the testing. Maybe it's the connection created in BaseJDBCTestCase, however, I have tried to override the setup() method to close the connection, but it can not work, too.

          How can I go ahead? I doubt this .sql is not fit to converted into a junit test case, in other word, harness is it's only way to work.

          Comments are welcome. Thanks!

          Show
          Houx Zhang added a comment - Thanks very much, Kathey. Your advice is helpful. ScriptTestCase(String script, boolean useSystemProperties) constructor can work as you advised. With setting it true, the System Properties are set up OK. However, my junit test case can not pass, while the harness.RunTest can pass. I have attached the file "ijConnName-1.tmp", which is created by harness.RunTest, and the file "ijConnName-1.out", created by my junit test case. In the file "ijConnName-1.out", we can see, " show connections; CONNECTION0* - jdbc:derby:wombat CONNONE - jdbc:derby:wombat " A connection with the name "CONNECTION0" has been created before the testing. Maybe it's the connection created in BaseJDBCTestCase, however, I have tried to override the setup() method to close the connection, but it can not work, too. How can I go ahead? I doubt this .sql is not fit to converted into a junit test case, in other word, harness is it's only way to work. Comments are welcome. Thanks!
          Hide
          Myrna van Lunteren added a comment -

          Hi,

          I don't see a .out nor .tmp file attached, did you forget that step (you wouldn't be the first, nor, I'm sure, the last) or did it fail?

          Though tempting to convert this to a straight junit test, where you have more control over the output, the name indicates this is a test intended to test the ij commands to manage connections. So converting it to straight junit would defeat the purpose of the test - it has to activate ij.

          Perhaps you can add a 'disconnect all' to the top of the sql script and get closer to the current output?

          The 'master' file (the expected output, which is saved in <trunk>/java/testing/org/apache/derbyTesting/functionTests/master/ijConnName.out) can be updated. As long as the result is equivalent, makes sense, doesn't hide any bugs, and is always the same (i.e. for all jvms, for embedded and client/server if the test runs both ways, for all operating systems).

          Show
          Myrna van Lunteren added a comment - Hi, I don't see a .out nor .tmp file attached, did you forget that step (you wouldn't be the first, nor, I'm sure, the last) or did it fail? Though tempting to convert this to a straight junit test, where you have more control over the output, the name indicates this is a test intended to test the ij commands to manage connections. So converting it to straight junit would defeat the purpose of the test - it has to activate ij. Perhaps you can add a 'disconnect all' to the top of the sql script and get closer to the current output? The 'master' file (the expected output, which is saved in <trunk>/java/testing/org/apache/derbyTesting/functionTests/master/ijConnName.out) can be updated. As long as the result is equivalent, makes sense, doesn't hide any bugs, and is always the same (i.e. for all jvms, for embedded and client/server if the test runs both ways, for all operating systems).
          Hide
          Houx Zhang added a comment -

          Oh, Myrna, I'm sorry for my mistake, let me post the patch first.

          Show
          Houx Zhang added a comment - Oh, Myrna, I'm sorry for my mistake, let me post the patch first.
          Hide
          Houx Zhang added a comment -

          Myrna, thanks for your advice. I have update the .sql file and the .out file by keeping the original opinion for testing. Now it can run well.

          However, as two new databases are created after one run, the second run will failed. Is there any TestSetup tool class that can remove the temporary databases, please? Thanks!

          Show
          Houx Zhang added a comment - Myrna, thanks for your advice. I have update the .sql file and the .out file by keeping the original opinion for testing. Now it can run well. However, as two new databases are created after one run, the second run will failed. Is there any TestSetup tool class that can remove the temporary databases, please? Thanks!
          Hide
          Houx Zhang added a comment -

          I have also tried to use "TestConfiguration.singleUseDatabaseDecorator" to decorate the test case, but the second run failed still.

          public class IjConnNameTest extends ScriptTestCase {

          private static String test_script = "ijConnName";

          public IjConnNameTest(String name)

          { super(name, true); }

          public static Test suite()

          { TestSuite suite = new TestSuite("IjConnNameTest"); suite.addTest(getTestCase()); suite.addTest(TestConfiguration.clientServerDecorator(getTestCase())); return suite; }

          private static Test getTestCase()

          { return TestConfiguration.singleUseDatabaseDecorator(new IjConnNameTest(test_script)); }

          }

          Show
          Houx Zhang added a comment - I have also tried to use "TestConfiguration.singleUseDatabaseDecorator" to decorate the test case, but the second run failed still. public class IjConnNameTest extends ScriptTestCase { private static String test_script = "ijConnName"; public IjConnNameTest(String name) { super(name, true); } public static Test suite() { TestSuite suite = new TestSuite("IjConnNameTest"); suite.addTest(getTestCase()); suite.addTest(TestConfiguration.clientServerDecorator(getTestCase())); return suite; } private static Test getTestCase() { return TestConfiguration.singleUseDatabaseDecorator(new IjConnNameTest(test_script)); } }
          Hide
          Myrna van Lunteren added a comment -

          Firstly, thank you for working on this issue.

          I took a closer look at your patch, and I think the problem you're seeing - the second run (probably the second one is the network server run, but the order depends on the jvm) fails with 'connection made to existing database instead' is because the sql script creates its own database using the connect statement with ";create=true". Most tests don't test database creation, and can rely on methods in TestConfiguration such as CleanDatabaseSetup or singleUseDatabaseDecorator...But we cannot do that in this case.

          Using TestConfiguration.singleUseDatabaseDecorator was a good attempt, but - as you can see by reading the javadoc description of the method - it's creating databases with a specific naming scheme.

          CleanDatabaseSetup assumes it will create the database to be used and the name will be 'wombat'.

          I think I ran into these same problems before, and ended up creating a separate teardown method in at least one instance, see org.apache.derbyTesting.functionTests.tests.i18n.LocalizedAttributeScriptTest.java.
          Note also that the .sql script for that test has a shutdown of the system to ensure the database directories can be removed; ijConnName.sql will need something similar.

          Note finally, that as this test uses some hard-coded urls that only reflect embedded, it would be pointless to run this with networkserver; it'll only connect using the embedded driver but start network server for no reason. Perhaps a similar test (.sql file) can be made to use network server urls and run that through NetworkServerTestSetup, but I'd say that would be an improvement beyond the scope of this JIRA.

          Apart from the database creation and cleanup issues, I don't think the current test is testing the same as the original one.
          Although it certainly manages to create a database using the same (or similar) URL, the original tests some ij specific properties which were set in the _app.properties file. You've rolled those properties into the url in the connect statement in the .sql file, which is not the same thing...

          I think you will need to wrap the test in a SystemPropertyTestSetup of some sort to set up those ij properties (which would unset them automatically afterwards).

          Finally, you should run the suite through the getIJConfig() method. This ensures that the test only runs when ij is available in the classpath.

          Show
          Myrna van Lunteren added a comment - Firstly, thank you for working on this issue. I took a closer look at your patch, and I think the problem you're seeing - the second run (probably the second one is the network server run, but the order depends on the jvm) fails with 'connection made to existing database instead' is because the sql script creates its own database using the connect statement with ";create=true". Most tests don't test database creation, and can rely on methods in TestConfiguration such as CleanDatabaseSetup or singleUseDatabaseDecorator...But we cannot do that in this case. Using TestConfiguration.singleUseDatabaseDecorator was a good attempt, but - as you can see by reading the javadoc description of the method - it's creating databases with a specific naming scheme. CleanDatabaseSetup assumes it will create the database to be used and the name will be 'wombat'. I think I ran into these same problems before, and ended up creating a separate teardown method in at least one instance, see org.apache.derbyTesting.functionTests.tests.i18n.LocalizedAttributeScriptTest.java. Note also that the .sql script for that test has a shutdown of the system to ensure the database directories can be removed; ijConnName.sql will need something similar. Note finally, that as this test uses some hard-coded urls that only reflect embedded, it would be pointless to run this with networkserver; it'll only connect using the embedded driver but start network server for no reason. Perhaps a similar test (.sql file) can be made to use network server urls and run that through NetworkServerTestSetup, but I'd say that would be an improvement beyond the scope of this JIRA. Apart from the database creation and cleanup issues, I don't think the current test is testing the same as the original one. Although it certainly manages to create a database using the same (or similar) URL, the original tests some ij specific properties which were set in the _app.properties file. You've rolled those properties into the url in the connect statement in the .sql file, which is not the same thing... I think you will need to wrap the test in a SystemPropertyTestSetup of some sort to set up those ij properties (which would unset them automatically afterwards). Finally, you should run the suite through the getIJConfig() method. This ensures that the test only runs when ij is available in the classpath.
          Hide
          Houx Zhang added a comment -

          Myrna, thanks very much for your detailed insctruction, it's valuale to me.

          Following your advice, I have finished DERBY-5084-3.patch. In it, removal of the lemming database is resolved well, and SystemPropertySetUp is used to perform the original aim in ijConnName_app.properties. In the beginning of the .sql script, I added:

          ---disconnect connection created by ScriptTestCase
          show connections;
          disconnect CONNECTION0;
          set connection connOne;

          I suppose, CONNECTION0(wombat) is created by the Derby JUnit framework, our connOne connected to the same database in fact, so "disconnect CONNECTION0" and "set connection connOne" can give a clear view.

          Please check the patch, thanks!

          Besides, if drop a temporary database is common, why not extract out a util class to do this? Maybe a new issue can be created to deal with it. There is already DropDatabaseSetup which can do this, but it's not visible, and just able to remove a specially named database.

          Regards

          Show
          Houx Zhang added a comment - Myrna, thanks very much for your detailed insctruction, it's valuale to me. Following your advice, I have finished DERBY-5084 -3.patch. In it, removal of the lemming database is resolved well, and SystemPropertySetUp is used to perform the original aim in ijConnName_app.properties. In the beginning of the .sql script, I added: ---disconnect connection created by ScriptTestCase show connections; disconnect CONNECTION0; set connection connOne; I suppose, CONNECTION0(wombat) is created by the Derby JUnit framework, our connOne connected to the same database in fact, so "disconnect CONNECTION0" and "set connection connOne" can give a clear view. Please check the patch, thanks! Besides, if drop a temporary database is common, why not extract out a util class to do this? Maybe a new issue can be created to deal with it. There is already DropDatabaseSetup which can do this, but it's not visible, and just able to remove a specially named database. Regards
          Hide
          Myrna van Lunteren added a comment -

          Attaching a modified patch.
          It differs from the _3.patch in that:

          • the last line in the suite() method returns the test, rather than the suite. When returning the suite, I got no (0) tests run. I don't understand how you could've gotten that to work, but it works well enough when the 'test' is returned:
            < + return getIJConfig(test);

            > + return getIJConfig(suite);
          • I've commented out the noSecurityManager call. It seemed unnecessary. If you agree, we could just remove it altogether.
            < + //test = SecurityManagerSetup.noSecurityManager(test);

            > + test = SecurityManagerSetup.noSecurityManager(test);
          • I changed the eol-style property for the new file to native (by doing svn propset svn:eol-style native. There's instructions on the web how to do this automatically for svn).
          • (nit I removed one of the dashes from the comment you added; normally a comment in ij is two dashes.

          However, there is still an issue with this test conversion.

          When I ran the test with these changes with ibm 1.6 (both on windows and linux, with classes or jars), I did get a mostly successful run, but the .out file was slightly different; instead of:
          +ERROR XJ004: Database 'nevercreated' not found.
          +ERROR 08001: No suitable driver found for jdbc:noone:fruitfly;create=true
          I got:
          +ERROR 08001: No suitable driver
          +ERROR XJ004: Database 'nevercreated' not found.

          My patch has the ibm jvm output.
          The original test hid this difference by using the _sed.properties file. I'm not sure what the solution for this is...

          Show
          Myrna van Lunteren added a comment - Attaching a modified patch. It differs from the _3.patch in that: the last line in the suite() method returns the test, rather than the suite. When returning the suite, I got no (0) tests run. I don't understand how you could've gotten that to work, but it works well enough when the 'test' is returned: < + return getIJConfig(test); — > + return getIJConfig(suite); I've commented out the noSecurityManager call. It seemed unnecessary. If you agree, we could just remove it altogether. < + //test = SecurityManagerSetup.noSecurityManager(test); — > + test = SecurityManagerSetup.noSecurityManager(test); I changed the eol-style property for the new file to native (by doing svn propset svn:eol-style native. There's instructions on the web how to do this automatically for svn). (nit I removed one of the dashes from the comment you added; normally a comment in ij is two dashes. However, there is still an issue with this test conversion. When I ran the test with these changes with ibm 1.6 (both on windows and linux, with classes or jars), I did get a mostly successful run, but the .out file was slightly different; instead of: +ERROR XJ004: Database 'nevercreated' not found. +ERROR 08001: No suitable driver found for jdbc:noone:fruitfly;create=true I got: +ERROR 08001: No suitable driver +ERROR XJ004: Database 'nevercreated' not found. My patch has the ibm jvm output. The original test hid this difference by using the _sed.properties file. I'm not sure what the solution for this is...
          Hide
          Kathey Marsden added a comment -

          Well, one thing to consider is that neither of these cases were actually tested with the old test because it had in the ijConnName_sed.properties:

          delete=ERROR 08001:,ERROR XJ004:

          but both are perfectly valid cases that should be tested.
          So I would say for the converted ijConnName test, keep the property ij.connection.connFour=jdbc:derby:nevercreated

          and test that case.

          For ij.connection.connThree=jdbc:noone:fruitfly;create=true , since the JVM messages differ I think we will need to think of a different way to test that one outside of the master based ScriptTestCase. I don't think it would be a major loss now with the conversion of this test as we are currrently deleting both these lines when run in the old harness.

          Show
          Kathey Marsden added a comment - Well, one thing to consider is that neither of these cases were actually tested with the old test because it had in the ijConnName_sed.properties: – delete=ERROR 08001: ,ERROR XJ004: but both are perfectly valid cases that should be tested. So I would say for the converted ijConnName test, keep the property ij.connection.connFour=jdbc:derby:nevercreated and test that case. For ij.connection.connThree=jdbc:noone:fruitfly;create=true , since the JVM messages differ I think we will need to think of a different way to test that one outside of the master based ScriptTestCase. I don't think it would be a major loss now with the conversion of this test as we are currrently deleting both these lines when run in the old harness.
          Hide
          Myrna van Lunteren added a comment -

          What Kathey said is true. I've modified the test once more, to eliminate the noone protocol property; and adjusted the master/ijConnName.out file accordingly.
          If there are no comments I'll check that part of the converted test in.

          I think the remaining property could get tested by making a separate test in a similar way to the SysinfoCPCheckTest and SysinfoAPITest. There is no need for a .sql script as there can be no connection; but if we catch the output in that mechanism we can check for the string '08001' and 'Driver not found' in the output.

          Show
          Myrna van Lunteren added a comment - What Kathey said is true. I've modified the test once more, to eliminate the noone protocol property; and adjusted the master/ijConnName.out file accordingly. If there are no comments I'll check that part of the converted test in. I think the remaining property could get tested by making a separate test in a similar way to the SysinfoCPCheckTest and SysinfoAPITest. There is no need for a .sql script as there can be no connection; but if we catch the output in that mechanism we can check for the string '08001' and 'Driver not found' in the output.
          Hide
          Houx Zhang added a comment -

          Myrna and Kathey, thanks for your working and advice on this issue.

          @Myrna
          As to "the last line in the suite() method returns the test, rather than the suite", I'm very sorry to have submitted a temporary patch by mistake. I will be more careful next time.

          As to "ERROR 08001: No suitable driver", I wil provide another patch on it later.

          Thanks again!

          Show
          Houx Zhang added a comment - Myrna and Kathey, thanks for your working and advice on this issue. @Myrna As to "the last line in the suite() method returns the test, rather than the suite", I'm very sorry to have submitted a temporary patch by mistake. I will be more careful next time. As to "ERROR 08001: No suitable driver", I wil provide another patch on it later. Thanks again!
          Hide
          Myrna van Lunteren added a comment -

          committed the patch after the changes as per DERBY-5084_5.diff with revision 1087433.
          I'll leave this open for the follow up patch for testing the ij property with a url with bad protocol.

          Show
          Myrna van Lunteren added a comment - committed the patch after the changes as per DERBY-5084 _5.diff with revision 1087433. I'll leave this open for the follow up patch for testing the ij property with a url with bad protocol.
          Hide
          Houx Zhang added a comment - - edited

          Hi, Myrna, I have submitted derby-5084-6.patch, which tests 'jdbc:noone:fruitfly;create=true'. Please check it, thanks!

          Show
          Houx Zhang added a comment - - edited Hi, Myrna, I have submitted derby-5084-6.patch, which tests 'jdbc:noone:fruitfly;create=true'. Please check it, thanks!
          Hide
          Bryan Pendleton added a comment -

          When I try to build patch 6 in my environment I get a compilation error (see below).

          I think that String.contains() is new with JDK 1.5, and this section of the code is by
          default compiled with JDK 1.4 compatibility, so we can't use that method.

          compilet1:
          [javac] Compiling 2 source files to /home/bpendleton/src/derby/trunk/classes
          [javac] /home/bpendleton/src/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/tools/IjConnNameWithWrongDriverTest.java:31: cannot find symbol
          [javac] symbol : method contains(java.lang.String)
          [javac] location: class java.lang.String
          [javac] assertTrue(ijResult.contains("08001"));
          [javac] ^
          [javac] /home/bpendleton/src/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/tools/IjConnNameWithWrongDriverTest.java:32: cannot find symbol
          [javac] symbol : method contains(java.lang.String)
          [javac] location: class java.lang.String
          [javac] assertTrue(ijResult.contains("No suitable driver"));
          [javac] ^
          [javac] 2 errors

          BUILD FAILED

          Show
          Bryan Pendleton added a comment - When I try to build patch 6 in my environment I get a compilation error (see below). I think that String.contains() is new with JDK 1.5, and this section of the code is by default compiled with JDK 1.4 compatibility, so we can't use that method. compilet1: [javac] Compiling 2 source files to /home/bpendleton/src/derby/trunk/classes [javac] /home/bpendleton/src/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/tools/IjConnNameWithWrongDriverTest.java:31: cannot find symbol [javac] symbol : method contains(java.lang.String) [javac] location: class java.lang.String [javac] assertTrue(ijResult.contains("08001")); [javac] ^ [javac] /home/bpendleton/src/derby/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/tools/IjConnNameWithWrongDriverTest.java:32: cannot find symbol [javac] symbol : method contains(java.lang.String) [javac] location: class java.lang.String [javac] assertTrue(ijResult.contains("No suitable driver")); [javac] ^ [javac] 2 errors BUILD FAILED
          Hide
          Myrna van Lunteren added a comment -

          I agree Bryan. I had made a similar comment - but my session had expired.

          I think on the whole the patch looks good, I have just 2 comments:

          1. You cannot use contains(String). This only works as of java 1.5; if you compile with 1.4.2 libraries, you'd get the build error Bryan described.
          We support jvm 1.4.2, so you cannot use this construct unless the class is explicitly getting built with 1.5 or higher (see the tools/build.xml; you can see source and target are defined as 1.4).
          To prevent this kind of issue with your patches, you should obtain a 1.4.2 jre, see trunk/<BUILDING.html>.
          There's also been discussions on this in the past, you can search the list archives. I think typically we use a construct like: assertTrue((ijResult.indexOf("08001")>1);

          2. As there is no protocol specified, there's no point running this test with both embedded and client, the only thing it would do with client is starting and shutting down NetworkServer for no reason. I suggest you use EmbeddedSuite() instead of defaultSuite().

          Did you run suites.All? You didn't say...

          Show
          Myrna van Lunteren added a comment - I agree Bryan. I had made a similar comment - but my session had expired. I think on the whole the patch looks good, I have just 2 comments: 1. You cannot use contains(String). This only works as of java 1.5; if you compile with 1.4.2 libraries, you'd get the build error Bryan described. We support jvm 1.4.2, so you cannot use this construct unless the class is explicitly getting built with 1.5 or higher (see the tools/build.xml; you can see source and target are defined as 1.4). To prevent this kind of issue with your patches, you should obtain a 1.4.2 jre, see trunk/<BUILDING.html>. There's also been discussions on this in the past, you can search the list archives. I think typically we use a construct like: assertTrue((ijResult.indexOf("08001")>1); 2. As there is no protocol specified, there's no point running this test with both embedded and client, the only thing it would do with client is starting and shutting down NetworkServer for no reason. I suggest you use EmbeddedSuite() instead of defaultSuite(). Did you run suites.All? You didn't say...
          Hide
          Houx Zhang added a comment -

          Thanks, Bryan and Myrna. I have summitted the new patch by your advice.

          @Myrna
          I have only run the tools.Suite, but not suites.All. And now, I have run the complete test, and I'm sure the new patch doesn't make bad effect on old testing.

          Besides, I'm not sure whether it's neccesary to run the complete for this patch, as it's just to have added a new test class, not changed any product code, maybe it's safe even not run the complete test suite. It have cost me over 5 hour in my PC for one run and I have to run again when testing was broken down by accident.

          Show
          Houx Zhang added a comment - Thanks, Bryan and Myrna. I have summitted the new patch by your advice. @Myrna I have only run the tools.Suite, but not suites.All. And now, I have run the complete test, and I'm sure the new patch doesn't make bad effect on old testing. Besides, I'm not sure whether it's neccesary to run the complete for this patch, as it's just to have added a new test class, not changed any product code, maybe it's safe even not run the complete test suite. It have cost me over 5 hour in my PC for one run and I have to run again when testing was broken down by accident.
          Hide
          Myrna van Lunteren added a comment -

          There's indeed no need to run the full suites.All for such a small test as with this patch.

          I took a look at your new patch, and find I didn't check the first patch (-6.patch) properly, I apologize for that.

          I don't think the new test is doing the same thing as the original test. The original test was setting the connection with the bad subprotocol ('jdbc:noone:', it should be jdbc:derby, of course) using an ij property. The new test is using a simple ij connect statement with a bad subprotocol.

          I did a simple 'grep' and didn't find another test where a bad subprotocol was lobbed at ij...(jdbcapi/DriverTest and management.JDBCMBeanTest do test incorrect subprotocol). So I guess this test does test something useful, but it's not the same as the original test.

          Also, but that's more of a nit, the name is not really right, because we're not even specifying a driver, just a bad subprotocol (which results in derby not finding any driver to use).

          Can you add the testing of passing in the bad subprotocol with a system property?

          Show
          Myrna van Lunteren added a comment - There's indeed no need to run the full suites.All for such a small test as with this patch. I took a look at your new patch, and find I didn't check the first patch (-6.patch) properly, I apologize for that. I don't think the new test is doing the same thing as the original test. The original test was setting the connection with the bad subprotocol ('jdbc:noone:', it should be jdbc:derby, of course) using an ij property. The new test is using a simple ij connect statement with a bad subprotocol. I did a simple 'grep' and didn't find another test where a bad subprotocol was lobbed at ij...(jdbcapi/DriverTest and management.JDBCMBeanTest do test incorrect subprotocol). So I guess this test does test something useful, but it's not the same as the original test. Also, but that's more of a nit, the name is not really right, because we're not even specifying a driver, just a bad subprotocol (which results in derby not finding any driver to use). Can you add the testing of passing in the bad subprotocol with a system property?
          Hide
          Houx Zhang added a comment -

          Hi, Myrna. Thanks for your advice. I have summitted 5084-8.patch, and it should meet all demands.

          Show
          Houx Zhang added a comment - Hi, Myrna. Thanks for your advice. I have summitted 5084-8.patch, and it should meet all demands.
          Hide
          Houx Zhang added a comment -

          I have added checking Derby.hasTools() in DERBY-5084-9.patch. Please check it, thanks!

          Show
          Houx Zhang added a comment - I have added checking Derby.hasTools() in DERBY-5084 -9.patch. Please check it, thanks!
          Hide
          Myrna van Lunteren added a comment -

          I committed patch -9 to trunk with revision 1097460, with 2 minor modifications:

          • this test - because there is no pre-decided use of client or embedded driver - will cause java.sql.Driver to be invoked, which isn't available with JSR169. So I added a section at the top of the suite method:
            if (JDBC.vmSupportsJSR169())
            return new TestSuite("empty: no support for Driver.sql.Manager with jsr 169");
          • I changed the name of the method that does the actual asserts from 'testConnectWrongSubprotocol' to 'checkConnectWrongSubprotocol'. I find it easier to identify the fixtures when they start with 'test' when I look at the code. But this was more a personal preference, no technical reason.

          Then I noticed that we missed adding the apache licence header to IjConnNameTest and the new test, and added them to trunk with revision 1097469.
          (Then I realized I forgot to change the name after cut-and-paste in one of the headers and fixed that up with reivision 1097471).

          I backported these changed to 10.8 with revision 1097477.

          Thank you for your work on this issue!

          Show
          Myrna van Lunteren added a comment - I committed patch -9 to trunk with revision 1097460, with 2 minor modifications: this test - because there is no pre-decided use of client or embedded driver - will cause java.sql.Driver to be invoked, which isn't available with JSR169. So I added a section at the top of the suite method: if (JDBC.vmSupportsJSR169()) return new TestSuite("empty: no support for Driver.sql.Manager with jsr 169"); I changed the name of the method that does the actual asserts from 'testConnectWrongSubprotocol' to 'checkConnectWrongSubprotocol'. I find it easier to identify the fixtures when they start with 'test' when I look at the code. But this was more a personal preference, no technical reason. Then I noticed that we missed adding the apache licence header to IjConnNameTest and the new test, and added them to trunk with revision 1097469. (Then I realized I forgot to change the name after cut-and-paste in one of the headers and fixed that up with reivision 1097471). I backported these changed to 10.8 with revision 1097477. Thank you for your work on this issue!
          Hide
          Houx Zhang added a comment -

          Thanks for your help on this issue, Myrna. I have harvested a lot. Thanks!

          Show
          Houx Zhang added a comment - Thanks for your help on this issue, Myrna. I have harvested a lot. Thanks!

            People

            • Assignee:
              Houx Zhang
              Reporter:
              Myrna van Lunteren
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development