Details

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

      Description

      Convert madhare.sql to JUnit

      1. d5127.diff
        8 kB
        Siddharth Srivastava
      2. derby5127.diff
        8 kB
        Siddharth Srivastava
      3. derby5127.diff
        8 kB
        Siddharth Srivastava
      4. derby5127.diff
        8 kB
        Siddharth Srivastava
      5. derby5127.diff
        8 kB
        Siddharth Srivastava
      6. Derby5127.diff
        5 kB
        Siddharth Srivastava
      7. Derby5127.diff
        8 kB
        Siddharth Srivastava
      8. derby5127(2).diff
        8 kB
        Siddharth Srivastava

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Myrna van Lunteren added a comment -

          I'm marking this as resolved, judging by the last communication between Kathey and Knut Anders there's nothing further needed here. Please reopen if I misread.

          Show
          Myrna van Lunteren added a comment - I'm marking this as resolved, judging by the last communication between Kathey and Knut Anders there's nothing further needed here. Please reopen if I misread.
          Hide
          Knut Anders Hatlen added a comment -

          ij's prepare and execute commands are tested in tools/ij2.sql, so I think we are covered.

          Show
          Knut Anders Hatlen added a comment - ij's prepare and execute commands are tested in tools/ij2.sql, so I think we are covered.
          Hide
          Kathey Marsden added a comment -

          Hi Siddharth,

          Thank you for the patch. I committed it at revison 1090359

          One thing I wonder from the original test is about this section from the test which tests not only JDBC prepared statements but also the ij prepare and execute support;

          ij> – we do have prepare and execute support
          prepare stmt as 'select i,n,t,e,g,r from s';
          ij> execute stmt;
          I |N |T |E |G |R
          -----------------------------------------------------------------------
          1 |2 |3 |4 |5 |6
          10 |11 |12 |13 |14 |15
          ij> – execute can be done multiple times
          execute stmt;

          I wonder if the prepare and execute ij commands are tested elsewhere specifically in the tools testing. If not it should be added there.

          Show
          Kathey Marsden added a comment - Hi Siddharth, Thank you for the patch. I committed it at revison 1090359 One thing I wonder from the original test is about this section from the test which tests not only JDBC prepared statements but also the ij prepare and execute support; ij> – we do have prepare and execute support prepare stmt as 'select i,n,t,e,g,r from s'; ij> execute stmt; I |N |T |E |G |R ----------------------------------------------------------------------- 1 |2 |3 |4 |5 |6 10 |11 |12 |13 |14 |15 ij> – execute can be done multiple times execute stmt; I wonder if the prepare and execute ij commands are tested elsewhere specifically in the tools testing. If not it should be added there.
          Hide
          Siddharth Srivastava added a comment -

          changed the diff to contain the changed code from previous commit.
          The patch now conforms to 80 character a line limit

          Show
          Siddharth Srivastava added a comment - changed the diff to contain the changed code from previous commit. The patch now conforms to 80 character a line limit
          Hide
          Kathey Marsden added a comment -

          Hi Siddharth, since the test has already been committed, you should svn update and then submit a patch with just the changes. Please try to keep the lines to 80 characters.

          Show
          Kathey Marsden added a comment - Hi Siddharth, since the test has already been committed, you should svn update and then submit a patch with just the changes. Please try to keep the lines to 80 characters.
          Hide
          Siddharth Srivastava added a comment -

          Thanks Knut, Kathey for pointing out the mistake.

          thanks Houx for your suggestions. I too initially thought of creating separate functions but it will only compromise with the simplicity of the code (with just permutations being tested in most of the cases)

          Show
          Siddharth Srivastava added a comment - Thanks Knut, Kathey for pointing out the mistake. thanks Houx for your suggestions. I too initially thought of creating separate functions but it will only compromise with the simplicity of the code (with just permutations being tested in most of the cases)
          Hide
          Siddharth Srivastava added a comment -

          this patch adds the code to check the result set of the SQL queries tested.

          Show
          Siddharth Srivastava added a comment - this patch adds the code to check the result set of the SQL queries tested.
          Hide
          Houx Zhang added a comment -

          Just some opinions:

          1. Is it better to split the testBasicMadhare() into several functions, and test only one point in each function? It will make the test more readable and maintainable.

          2. Why not use dropTable(String) directly to replace current st.executeUpdate( "drop table t") to keep the code clean?

          Show
          Houx Zhang added a comment - Just some opinions: 1. Is it better to split the testBasicMadhare() into several functions, and test only one point in each function? It will make the test more readable and maintainable. 2. Why not use dropTable(String) directly to replace current st.executeUpdate( "drop table t") to keep the code clean?
          Hide
          Kathey Marsden added a comment -

          You are absolutely right. I guess I got wrapped up in the minutia in the review and missed this major point. Siddharth can you submit a new patch using org.apache.derbyTesting.junit.JDBC.assertFullResultSet() to verify the results? You can see sample usage in lang.SimpleTest and other tests.

          If the ResultSet is big I sometimes use org.apache.derbyTesting.junit.showResultSet() temporarily to print out the array to feed to assertFullResultSet().

          Show
          Kathey Marsden added a comment - You are absolutely right. I guess I got wrapped up in the minutia in the review and missed this major point. Siddharth can you submit a new patch using org.apache.derbyTesting.junit.JDBC.assertFullResultSet() to verify the results? You can see sample usage in lang.SimpleTest and other tests. If the ResultSet is big I sometimes use org.apache.derbyTesting.junit.showResultSet() temporarily to print out the array to feed to assertFullResultSet().
          Hide
          Knut Anders Hatlen added a comment -

          The new test executes many SQL statements, but it doesn't check the results from any of them. Is that intentional? It looks like the original test verified that the select queries returned the expected set of rows.

          Show
          Knut Anders Hatlen added a comment - The new test executes many SQL statements, but it doesn't check the results from any of them. Is that intentional? It looks like the original test verified that the select queries returned the expected set of rows.
          Hide
          Siddharth Srivastava added a comment -

          thanks Kathey. I will keep these points in mind in future patches.

          Show
          Siddharth Srivastava added a comment - thanks Kathey. I will keep these points in mind in future patches.
          Hide
          Kathey Marsden added a comment -

          I think the new patch looks fine except that the fixture throws an Exception instead of SQLException. Also the master/madhare.out (master file) needs to be removed with svn delete. I will commit with these two small changes. Normally I would say it would be good to have javadoc for each fixture but since this test just has one and there are comments in the class comments, I think that is ok. For future conversions, however make sure each fixture has javadoc.

          Show
          Kathey Marsden added a comment - I think the new patch looks fine except that the fixture throws an Exception instead of SQLException. Also the master/madhare.out (master file) needs to be removed with svn delete. I will commit with these two small changes. Normally I would say it would be good to have javadoc for each fixture but since this test just has one and there are comments in the class comments, I think that is ok. For future conversions, however make sure each fixture has javadoc.
          Hide
          Siddharth Srivastava added a comment -

          This diff addresses the formatting issues and other issues addressed by Lily Wei and Kathey Marsden.

          I have run suites.All and derbyAll. No error were caused due to this patch.
          suites.All produces these 3 failures:
          1) AuthenticationTest
          2) ReplicationRun_Local_1Indexing
          3) ReplicationRun_Local_3_p3

          Show
          Siddharth Srivastava added a comment - This diff addresses the formatting issues and other issues addressed by Lily Wei and Kathey Marsden. I have run suites.All and derbyAll. No error were caused due to this patch. suites.All produces these 3 failures: 1) AuthenticationTest 2) ReplicationRun_Local_1Indexing 3) ReplicationRun_Local_3_p3
          Hide
          Siddharth Srivastava added a comment -

          Thanks Dave for clarifying. Just tried with the latest commit by Knut (of Derby-5143) and the warning isn't there anymore

          Show
          Siddharth Srivastava added a comment - Thanks Dave for clarifying. Just tried with the latest commit by Knut (of Derby-5143) and the warning isn't there anymore
          Hide
          Dave Brosius added a comment -

          No, typically this is because code is casting the value coming out of some collection without checking if it really is the type expected, like

          String s = (String)mycollection.get;

          Since get() in 1.4 is declared to return java.lang.Object, the cast to String is a blind cast and 'potentially' unchecked.

          Show
          Dave Brosius added a comment - No, typically this is because code is casting the value coming out of some collection without checking if it really is the type expected, like String s = (String)mycollection.get ; Since get() in 1.4 is declared to return java.lang.Object, the cast to String is a blind cast and 'potentially' unchecked.
          Hide
          Siddharth Srivastava added a comment -

          While building derby(ant -quiet all), I get this error:

          [javac] Note: F:\eclipsework\trunk\java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\ConnectionTest.java uses unchecked or unsafe operations.
          [javac] Note: Recompile with -Xlint:unchecked for details.

          May be some of the code is not compatible with java 4 ?

          Show
          Siddharth Srivastava added a comment - While building derby(ant -quiet all), I get this error: [javac] Note: F:\eclipsework\trunk\java\testing\org\apache\derbyTesting\functionTests\tests\jdbc4\ConnectionTest.java uses unchecked or unsafe operations. [javac] Note: Recompile with -Xlint:unchecked for details. May be some of the code is not compatible with java 4 ?

            People

            • Assignee:
              Siddharth Srivastava
              Reporter:
              Siddharth Srivastava
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development