Derby
  1. Derby
  2. DERBY-3294

Convert demo/checkToursDB.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

      Description

      Place holder for the junit conversion of demo/checkToursDB.java

      1. DERBY-3294_stat_12_28.txt
        0.3 kB
        Manjula Kutty
      2. DERBY-3294_diff_12_28.txt
        14 kB
        Manjula Kutty
      3. DERBY_3294_stat_01_02.txt
        0.3 kB
        Manjula Kutty
      4. DERBY_3294_diff_01_02.txt
        15 kB
        Manjula Kutty
      5. DERBY_3294_stat_1_4.txt
        0.3 kB
        Manjula Kutty
      6. DERBY_3294_diff_1_4.txt
        15 kB
        Manjula Kutty
      7. DERBY_3294_stat_1_7.txt
        0.3 kB
        Manjula Kutty
      8. DERBY_3294_diff_1_7.txt
        15 kB
        Manjula Kutty
      9. DERBY-3294_stat_1_14.txt
        0.6 kB
        Myrna van Lunteren
      10. DERBY-3294_diff_1_14.txt
        17 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Hide
          Manjula Kutty added a comment -

          Attaching the converted CheckToursDBTest and related files. I didn't delete the original checkToursDB.java from the derbyall. Will do that once this one gets commited.

          Please review and if looks good, please commit

          Show
          Manjula Kutty added a comment - Attaching the converted CheckToursDBTest and related files. I didn't delete the original checkToursDB.java from the derbyall. Will do that once this one gets commited. Please review and if looks good, please commit
          Hide
          John H. Embretsen added a comment -

          The patch DERBY-3294_diff_12_28.txt breaks the build - the file checkToursDBTest.java
          should be renamed to match the casing used in the source (CheckToursDBTest).
          The source file's copyright statement should also be updated if such a
          change is made.

          Other than that, I have the following comments (mostly nitpicking):

          @demo._Suite.java:

          • two unused imports

          @demo.checkToursDBTest.java:

          • typo in class Javadoc: TourDB --> ToursDB
          • some methods throw both SQLException and Exception, which seems
            unnecessary.
          • methods that can only throw SQLExeption should not declare that
            they may throw any Exception (doDelete(), doUpdate())
          • Javadoc for tearDown() method says "Tear-down the fixture by
            removing the tables", but the tables are not removed.
          • Other existing Javadoc is incomplete (e.g. missing/wrong @throws,
            @param tags), while some methods have no Javadoc altogether.
          • some comments on why the different parts of the test are not run as
            individual test fixtures would be good for future reference.
          Show
          John H. Embretsen added a comment - The patch DERBY-3294 _diff_12_28.txt breaks the build - the file checkToursDBTest.java should be renamed to match the casing used in the source (CheckToursDBTest). The source file's copyright statement should also be updated if such a change is made. Other than that, I have the following comments (mostly nitpicking): @demo._Suite.java: two unused imports @demo.checkToursDBTest.java: typo in class Javadoc: TourDB --> ToursDB some methods throw both SQLException and Exception, which seems unnecessary. methods that can only throw SQLExeption should not declare that they may throw any Exception (doDelete(), doUpdate()) Javadoc for tearDown() method says "Tear-down the fixture by removing the tables", but the tables are not removed. Other existing Javadoc is incomplete (e.g. missing/wrong @throws, @param tags), while some methods have no Javadoc altogether. some comments on why the different parts of the test are not run as individual test fixtures would be good for future reference.
          Hide
          Manjula Kutty added a comment -

          Thanks John for the comments. My replies are in line. Also please find the updated patches. Please review them.

          >The patch DERBY-3294_diff_12_28.txt breaks the build - the file >checkToursDBTest.java
          >should be renamed to match the casing used in the source
          >(CheckToursDBTest).
          >The source file's copyright statement should also be updated if such a
          >change is made.

          It was my mistake. I created the test first with "checkToursDBTest.java" and then moved it to the correct name format. But forgot to delete the older one, and that followed every where.

          >@demo._Suite.java:

          >- two unused imports

          Removed them

          >@demo.checkToursDBTest.java:

          >- typo in class Javadoc: TourDB --> ToursDB
          Corrected

          >- some methods throw both SQLException and Exception, which seems
          >unnecessary.
          Fixed
          > - methods that can only throw SQLExeption should not declare that
          > they may throw any Exception (doDelete(), doUpdate())
          Done
          > - Javadoc for tearDown() method says "Tear-down the fixture by
          > removing the tables", but the tables are not removed.
          Included dropping tables
          > - Other existing Javadoc is incomplete (e.g. missing/wrong @throws,
          > @param tags), while some methods have no Javadoc altogether.
          Fixed
          >- some comments on why the different parts of the test are not run as
          > individual test fixtures would be good for future reference.
          First I made each of the insert, delete and update in different fixtures. Then since the order of running the fixtures are not fixed the test may fail in some scenarios. So made all the methods in the same one test.

          Show
          Manjula Kutty added a comment - Thanks John for the comments. My replies are in line. Also please find the updated patches. Please review them. >The patch DERBY-3294 _diff_12_28.txt breaks the build - the file >checkToursDBTest.java >should be renamed to match the casing used in the source >(CheckToursDBTest). >The source file's copyright statement should also be updated if such a >change is made. It was my mistake. I created the test first with "checkToursDBTest.java" and then moved it to the correct name format. But forgot to delete the older one, and that followed every where. >@demo._Suite.java: >- two unused imports Removed them >@demo.checkToursDBTest.java: >- typo in class Javadoc: TourDB --> ToursDB Corrected >- some methods throw both SQLException and Exception, which seems >unnecessary. Fixed > - methods that can only throw SQLExeption should not declare that > they may throw any Exception (doDelete(), doUpdate()) Done > - Javadoc for tearDown() method says "Tear-down the fixture by > removing the tables", but the tables are not removed. Included dropping tables > - Other existing Javadoc is incomplete (e.g. missing/wrong @throws, > @param tags), while some methods have no Javadoc altogether. Fixed >- some comments on why the different parts of the test are not run as > individual test fixtures would be good for future reference. First I made each of the insert, delete and update in different fixtures. Then since the order of running the fixtures are not fixed the test may fail in some scenarios. So made all the methods in the same one test.
          Hide
          John H. Embretsen added a comment -

          Thanks for the update. Some follow-up comments, having considered the
          second patch:

          • I think it may be possible to implement the doDelete(), doUpdate()
            and doSelect() methods as independent fixtures (by utilizing rollback(),
            setUp()/tearDown() and possibly a new test setup class) or to specify
            a specific ordering of fixtures in the suite() method, but the current
            scheme works quite well, so keeping it is ok with me.
          • I think it is confusing to have both the methods insertMaps() and
            InsertMaps(). One suggestion is to comply with common Java naming
            conventions and use a lowercase letter in the beginning of method names,
            for example by renaming InsertMaps() to insertMapsPrivileged().
          • the non-JUnit version of this test tests that select count works (not
            sure of the intended purpose of doing this) from all the tables both
            before and after deletes/updates, while the new version only selects
            before doUpdate()/doDelete(). Is that intentional?
            (I'm not saying it should be different, I just think the reasoning
            behind this decision should be on record.)
          • testToursDB() method's Javadoc specifies a wrong exception in the
            @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc.
          • tearDown() lacks @throws tag in Javadoc.
          Show
          John H. Embretsen added a comment - Thanks for the update. Some follow-up comments, having considered the second patch: I think it may be possible to implement the doDelete(), doUpdate() and doSelect() methods as independent fixtures (by utilizing rollback(), setUp()/tearDown() and possibly a new test setup class) or to specify a specific ordering of fixtures in the suite() method, but the current scheme works quite well, so keeping it is ok with me. I think it is confusing to have both the methods insertMaps() and InsertMaps(). One suggestion is to comply with common Java naming conventions and use a lowercase letter in the beginning of method names, for example by renaming InsertMaps() to insertMapsPrivileged(). the non-JUnit version of this test tests that select count works (not sure of the intended purpose of doing this) from all the tables both before and after deletes/updates, while the new version only selects before doUpdate()/doDelete(). Is that intentional? (I'm not saying it should be different, I just think the reasoning behind this decision should be on record.) testToursDB() method's Javadoc specifies a wrong exception in the @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc. tearDown() lacks @throws tag in Javadoc.
          Hide
          Manjula Kutty added a comment -

          Thanks for the comments, John, Here is my reply and the new patch. Please review.

          • I think it may be possible to implement the doDelete(), doUpdate()
            and doSelect() methods as independent fixtures (by utilizing rollback(),
            setUp()/tearDown() and possibly a new test setup class) or to specify
            a specific ordering of fixtures in the suite() method, but the current
            scheme works quite well, so keeping it is ok with me.

          I would like to keep like that now and will change once I figure out how to do it.

          • I think it is confusing to have both the methods insertMaps() and
            InsertMaps(). One suggestion is to comply with common Java naming
            conventions and use a lowercase letter in the beginning of method names,
            for example by renaming InsertMaps() to insertMapsPrivileged().

          Done!

          • the non-JUnit version of this test tests that select count works (not
            sure of the intended purpose of doing this) from all the tables both
            before and after deletes/updates, while the new version only selects
            before doUpdate()/doDelete(). Is that intentional?
            (I'm not saying it should be different, I just think the reasoning
            behind this decision should be on record.)

          -The doSelect() asserts with the original number of rows. But after deleting the rows, those asserts will fail. So I thought not doing since the assertions will make sure the rows are deleted.

          • testToursDB() method's Javadoc specifies a wrong exception in the
            @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc.

          Fixed

          • tearDown() lacks @throws tag in Javadoc.

          Fixed

          Show
          Manjula Kutty added a comment - Thanks for the comments, John, Here is my reply and the new patch. Please review. I think it may be possible to implement the doDelete(), doUpdate() and doSelect() methods as independent fixtures (by utilizing rollback(), setUp()/tearDown() and possibly a new test setup class) or to specify a specific ordering of fixtures in the suite() method, but the current scheme works quite well, so keeping it is ok with me. I would like to keep like that now and will change once I figure out how to do it. I think it is confusing to have both the methods insertMaps() and InsertMaps(). One suggestion is to comply with common Java naming conventions and use a lowercase letter in the beginning of method names, for example by renaming InsertMaps() to insertMapsPrivileged(). Done! the non-JUnit version of this test tests that select count works (not sure of the intended purpose of doing this) from all the tables both before and after deletes/updates, while the new version only selects before doUpdate()/doDelete(). Is that intentional? (I'm not saying it should be different, I just think the reasoning behind this decision should be on record.) -The doSelect() asserts with the original number of rows. But after deleting the rows, those asserts will fail. So I thought not doing since the assertions will make sure the rows are deleted. testToursDB() method's Javadoc specifies a wrong exception in the @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc. Fixed tearDown() lacks @throws tag in Javadoc. Fixed
          Hide
          John H. Embretsen added a comment -

          Manjula Kutty wrote:

          > -The doSelect() asserts with the original number of rows. But after deleting the rows, those asserts will fail. So I thought not doing since the assertions will make sure the rows are deleted.

          OK. I think what you are saying is that the number of rows deleted is asserted in the doDelete() method, so there is no need to check this again by doing another select count after the deletes.

          > - testToursDB() method's Javadoc specifies a wrong exception in the
          > @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc.
          >
          > Fixed

          The typo is fixed, but it still specifies the wrong exception after the @throws tag.

          (If you are going to provide a new patch, it would be good to add a @param tag for the CheckToursDBTest constructor as well.)

          I have no further comments at this time, so I guess I'll leave the rest to a committer...

          Show
          John H. Embretsen added a comment - Manjula Kutty wrote: > -The doSelect() asserts with the original number of rows. But after deleting the rows, those asserts will fail. So I thought not doing since the assertions will make sure the rows are deleted. OK. I think what you are saying is that the number of rows deleted is asserted in the doDelete() method, so there is no need to check this again by doing another select count after the deletes. > - testToursDB() method's Javadoc specifies a wrong exception in the > @throws tag. There is also a typo (tourdb --> ToursDB) in the Javadoc. > > Fixed The typo is fixed, but it still specifies the wrong exception after the @throws tag. (If you are going to provide a new patch, it would be good to add a @param tag for the CheckToursDBTest constructor as well.) I have no further comments at this time, so I guess I'll leave the rest to a committer...
          Hide
          Manjula Kutty added a comment -

          Thanks John for the review. Here is the modified patch. Please review and commit.

          Show
          Manjula Kutty added a comment - Thanks John for the review. Here is the modified patch. Please review and commit.
          Hide
          Myrna van Lunteren added a comment -

          I'm looking at committing this - and on the whole it looks good.

          However, I wonder why the method insertMaps was introduced. Was it not possible to use the insertMaps.main() method like the original test does?

          I think it should...

          The point of this test is to ensure that the toursDB created during compile is correct/working. Using the toursdb created would be one way to do this. The original test took another approach, and worked with the files in the demo directory (the .sql files get copied to the built functionTests dir from the java/demo source), mimicking the toursdb creation during the build using the identical files as the build. Using insertMaps.main() as the original test does, is identical to what happens to populate the MAPS table in the toursDB database during the build process. (The java/demo/toursdb.build.xml does an ant java call to insertMaps, which executes the main method).

          However, in the conversion now, you've duplicated the code, rather than actually using the available class, and thus, you're not actually testing the code...

          Show
          Myrna van Lunteren added a comment - I'm looking at committing this - and on the whole it looks good. However, I wonder why the method insertMaps was introduced. Was it not possible to use the insertMaps.main() method like the original test does? I think it should... The point of this test is to ensure that the toursDB created during compile is correct/working. Using the toursdb created would be one way to do this. The original test took another approach, and worked with the files in the demo directory (the .sql files get copied to the built functionTests dir from the java/demo source), mimicking the toursdb creation during the build using the identical files as the build. Using insertMaps.main() as the original test does, is identical to what happens to populate the MAPS table in the toursDB database during the build process. (The java/demo/toursdb.build.xml does an ant java call to insertMaps, which executes the main method). However, in the conversion now, you've duplicated the code, rather than actually using the available class, and thus, you're not actually testing the code...
          Hide
          Manjula Kutty added a comment -

          Thank you for your comments Myrna. I tried running the test with the available insertMaps class. Since it is a pre-created class, it has System.out.println statements. Also I have to change the File names to specify the full path (in the avilable insertMaps class). For this to happen I had changed the TestConfiguration.java to have a new decorator which accepts the database name. Even after all these the test has not finished and is hanging. So I would like to have the test like this. If you are OK with this please commit

          Show
          Manjula Kutty added a comment - Thank you for your comments Myrna. I tried running the test with the available insertMaps class. Since it is a pre-created class, it has System.out.println statements. Also I have to change the File names to specify the full path (in the avilable insertMaps class). For this to happen I had changed the TestConfiguration.java to have a new decorator which accepts the database name. Even after all these the test has not finished and is hanging. So I would like to have the test like this. If you are OK with this please commit
          Hide
          Myrna van Lunteren added a comment -

          I just do not like that duplication of code, I think it defeats the purpose of adding that test.

          So, I took your patch, then modified java/demo/toursdb/insertMaps.java so that the main() method calls a new method - insertRows() - which does all the work, but prints nothing, and returns the number of rows inserted.
          This insertRows() method is then called from the test.

          It then became necessary to allow for a 'null' path as well as the path used by the test; I wasn't sure whether - and if so, how - to document the purpose of this in insertMaps.java...

          In addition, it became necessary to add fileIn.close for the gif files in insertMaps.java, or the junit test cleanup wouldn't work.

          Finally, I noticed that the LightRail row inserts BART.gif. I thought this logically wrong, but inserting LightRail.gif hits an error 22001 because the create table statement uses a blob size smaller than LightRail.gif. I didn't want to increase the size of toursDB in the distributions, so instead I added a comment to insertMaps.

          The alternatives were to enlarge the database and insert LightRail.gif, or to not add the third row, but those two would possibly require changing any documentation references, and I didn't want to go there. Or I could try to make a smaller LightRail.gif, but again, I felt that to be getting sidetracked too much.

          demo/toursdb/insertMaps.java itself is not intended as a demo, but to populate the database (used in many doc examples) so I think it would be ok to make these changes...

          If I hear nothing on this, I'll commit this latest patch tomorrow...

          Show
          Myrna van Lunteren added a comment - I just do not like that duplication of code, I think it defeats the purpose of adding that test. So, I took your patch, then modified java/demo/toursdb/insertMaps.java so that the main() method calls a new method - insertRows() - which does all the work, but prints nothing, and returns the number of rows inserted. This insertRows() method is then called from the test. It then became necessary to allow for a 'null' path as well as the path used by the test; I wasn't sure whether - and if so, how - to document the purpose of this in insertMaps.java... In addition, it became necessary to add fileIn.close for the gif files in insertMaps.java, or the junit test cleanup wouldn't work. Finally, I noticed that the LightRail row inserts BART.gif. I thought this logically wrong, but inserting LightRail.gif hits an error 22001 because the create table statement uses a blob size smaller than LightRail.gif. I didn't want to increase the size of toursDB in the distributions, so instead I added a comment to insertMaps. The alternatives were to enlarge the database and insert LightRail.gif, or to not add the third row, but those two would possibly require changing any documentation references, and I didn't want to go there. Or I could try to make a smaller LightRail.gif, but again, I felt that to be getting sidetracked too much. demo/toursdb/insertMaps.java itself is not intended as a demo, but to populate the database (used in many doc examples) so I think it would be ok to make these changes... If I hear nothing on this, I'll commit this latest patch tomorrow...
          Hide
          Manjula Kutty added a comment -

          Thanks Myrna..This patch looks good to me.

          Show
          Manjula Kutty added a comment - Thanks Myrna..This patch looks good to me.
          Hide
          Myrna van Lunteren added a comment -

          committed the last patch, as well as removing demo (entire suite, as checkToursDB was the only test in it) from the old test harness with revision 612218.

          Show
          Myrna van Lunteren added a comment - committed the last patch, as well as removing demo (entire suite, as checkToursDB was the only test in it) from the old test harness with revision 612218.
          Hide
          Manjula Kutty added a comment -

          Thanks Myrna

          Show
          Manjula Kutty added a comment - Thanks Myrna
          Hide
          Myrna van Lunteren added a comment -

          As of revision 613201 this test should no longer run with JSR169; it is executing PreparedStatement.setBigDecimal() which is not supported in that environment.

          Show
          Myrna van Lunteren added a comment - As of revision 613201 this test should no longer run with JSR169; it is executing PreparedStatement.setBigDecimal() which is not supported in that environment.

            People

            • Assignee:
              Manjula Kutty
              Reporter:
              Manjula Kutty
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development