Details

    • Type: Sub-task Sub-task
    • 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

      There are currently 3 sets of GROUP BY tests:

      • GroupByExpressionTest.java
      • GroupByTest.java
      • groupBy.sql

      The first two tests are JUnit tests; the groupBy.sql tests are old-style harness tests,
      although they are now run in the JUnit framework using the LangScripts technique.

      This sub-task proposes to convert the groupBy.sql tests to JUnit tests, and to include
      them directly into GroupByTest.java.

      The DERBY-2151 conversion tool can be used to assist in the conversion process.

      1. afterMergeWith3257.diff
        138 kB
        Bryan Pendleton
      2. convertToJunit.diff
        138 kB
        Bryan Pendleton
      3. convertToJunit.diff
        137 kB
        Bryan Pendleton
      4. convertToJUnit.diff
        133 kB
        Bryan Pendleton
      5. convertToJUnit.stat
        0.5 kB
        Bryan Pendleton

        Issue Links

          Activity

          Hide
          Bryan Pendleton added a comment -

          Committed to the trunk as revision 614596.

          Show
          Bryan Pendleton added a comment - Committed to the trunk as revision 614596.
          Hide
          Bryan Pendleton added a comment -

          'afterMergeWith3257.diff' is an updated patch, which
          resolves the merge with the DERBY-3257 patch.

          I also ran junit-all and derbyall and I've got 100% pass rate.

          If there are no further comments, I'll commit this patch later this week.

          Show
          Bryan Pendleton added a comment - 'afterMergeWith3257.diff' is an updated patch, which resolves the merge with the DERBY-3257 patch. I also ran junit-all and derbyall and I've got 100% pass rate. If there are no further comments, I'll commit this patch later this week.
          Hide
          Bryan Pendleton added a comment -

          Thanks Dan!

          Updating the test to use CleanDatabaseTestSetup has the
          added benefit that the test now runs in 9 seconds, where it
          was taking close to a minute before. Very nice.

          In addition to using CleanDatabaseTestSetup, I also made
          the Statement and ResultSet variables local to the test case.

          Show
          Bryan Pendleton added a comment - Thanks Dan! Updating the test to use CleanDatabaseTestSetup has the added benefit that the test now runs in 9 seconds, where it was taking close to a minute before. Very nice. In addition to using CleanDatabaseTestSetup, I also made the Statement and ResultSet variables local to the test case.
          Hide
          Daniel John Debrunner added a comment -

          > 'm still a beginner at this process of converting the old-style IJ script tests to JUnit ...

          Just a reminder that setup and tearDown is executed once per fixture (each testXXX method) not once per class.
          From a quick look at the test fixtures it seems they do not modify the data, thus would it make more sense to perform
          the setup using the facilities of CleanDatabaseTestSetup? Ie. CleanDatabaseTestSetup.decorateSQL. Then no test specific
          code is needed for cleanup.

          Usuallly test classes do not need instance variables for JDBC objects (e.g Statements and ResultSets). They provide almost no benefit and have the downside that either code is needed to clean them up (assign the fields to null) or they continue to use memory after the test has finished.

          Such fields provide no benefit because the object in the field (e.g. a Statement) is not shared across fixtures, each fixture is represented by a unique instance of the class, thus with N fixutures N Statements will be created rather than the one that might be assumed by using a field. Since the Statement is local to the fixture it becomes cleaner to keep it locally scoped in the testXXX() fixture method, passing it into utility methods as required.
          The only benefit can be sharing a Statement object across a testXXX fixuture method and setup/teardown methods. Since Statement objects are not expensive to create then I think this is of dubious value, I think it's clearer for someone reading the test later to understand the code with a locally scoped object rather than an instance object.

          Show
          Daniel John Debrunner added a comment - > 'm still a beginner at this process of converting the old-style IJ script tests to JUnit ... Just a reminder that setup and tearDown is executed once per fixture (each testXXX method) not once per class. From a quick look at the test fixtures it seems they do not modify the data, thus would it make more sense to perform the setup using the facilities of CleanDatabaseTestSetup? Ie. CleanDatabaseTestSetup.decorateSQL. Then no test specific code is needed for cleanup. Usuallly test classes do not need instance variables for JDBC objects (e.g Statements and ResultSets). They provide almost no benefit and have the downside that either code is needed to clean them up (assign the fields to null) or they continue to use memory after the test has finished. Such fields provide no benefit because the object in the field (e.g. a Statement) is not shared across fixtures, each fixture is represented by a unique instance of the class, thus with N fixutures N Statements will be created rather than the one that might be assumed by using a field. Since the Statement is local to the fixture it becomes cleaner to keep it locally scoped in the testXXX() fixture method, passing it into utility methods as required. The only benefit can be sharing a Statement object across a testXXX fixuture method and setup/teardown methods. Since Statement objects are not expensive to create then I think this is of dubious value, I think it's clearer for someone reading the test later to understand the code with a locally scoped object rather than an instance object.
          Hide
          Bryan Pendleton added a comment -

          Set patchAvailable flag to request more feedback.

          I haven't yet run the complete test suites with this patch, but
          GroupByTest by itself runs correctly in my environment.

          If people are generally comfortable with this patch, I'll move
          ahead with more complete testing.

          Show
          Bryan Pendleton added a comment - Set patchAvailable flag to request more feedback. I haven't yet run the complete test suites with this patch, but GroupByTest by itself runs correctly in my environment. If people are generally comfortable with this patch, I'll move ahead with more complete testing.
          Hide
          Bryan Pendleton added a comment -

          Attached is a new version of 'convertToJunit.diff'.

          This version substantially revises GroupByTest.java
          to address Manish's suggestions regarding test case
          organization. I have broken the monolithic test case
          generated by the conversion tool into a dozen sub-tests,
          according to boundaries that seemed reasonable to me.

          I also:

          • consolidated all the table creation/population logic
            into setUp() and the drop logic into tearDown()
          • revised the existing test cases in GroupByTest to match
          • removed the line which was setting autocommit to
            false, and simply had all the tests run in autocommit mode.

          The .stat file is unchanged; this patch deletes 4 files and modifies 2.

          I'm still a beginner at this process of converting the old-style
          IJ script tests to JUnit, so there are probably more things
          I've missed. So, please have a look at this new patch and
          provide more feedback!

          Show
          Bryan Pendleton added a comment - Attached is a new version of 'convertToJunit.diff'. This version substantially revises GroupByTest.java to address Manish's suggestions regarding test case organization. I have broken the monolithic test case generated by the conversion tool into a dozen sub-tests, according to boundaries that seemed reasonable to me. I also: consolidated all the table creation/population logic into setUp() and the drop logic into tearDown() revised the existing test cases in GroupByTest to match removed the line which was setting autocommit to false, and simply had all the tests run in autocommit mode. The .stat file is unchanged; this patch deletes 4 files and modifies 2. I'm still a beginner at this process of converting the old-style IJ script tests to JUnit, so there are probably more things I've missed. So, please have a look at this new patch and provide more feedback!
          Hide
          Bryan Pendleton added a comment -

          Sorry, I kind of lost track of this issue. I intend to resume work on this and
          try to get it done for 10.4.

          Show
          Bryan Pendleton added a comment - Sorry, I kind of lost track of this issue. I intend to resume work on this and try to get it done for 10.4.
          Hide
          Bryan Pendleton added a comment -

          Thanks, these are great suggestions! I will have a look at improving the code accordingly.

          Show
          Bryan Pendleton added a comment - Thanks, these are great suggestions! I will have a look at improving the code accordingly.
          Hide
          Manish Khettry added a comment -

          A few thoughts on the test case.

          1. The testcase creates and drops the fixtures (tables t1,t2) that it needs If the test fails, the tables will not be dropped causing potential problems if others add more tests to this suite. It may be better to move the table creation to a setUp method or better still to a oneTimeSetup or atleast dropping the tables in a finally block.

          2. Do you think its worth the effort to break up the new test case testGroupBy into smaller test cases? There are several advantages to small test cases-- for one, if you encounter a failure, it does not hide other problems down the test case. Second it can quickly clue you to the nature of the failure. The comments in the test provide a handy way to split up the test case; i.e.

          // ?s in group by
          // group by on long varchar type
          // having clause cannot contain column references which
          // are not grouping columns

          If a test case called testParamInGroupBy fails, the person investigating the failure has a lot more to go by then a failure in testGroupBy.

          Show
          Manish Khettry added a comment - A few thoughts on the test case. 1. The testcase creates and drops the fixtures (tables t1,t2) that it needs If the test fails, the tables will not be dropped causing potential problems if others add more tests to this suite. It may be better to move the table creation to a setUp method or better still to a oneTimeSetup or atleast dropping the tables in a finally block. 2. Do you think its worth the effort to break up the new test case testGroupBy into smaller test cases? There are several advantages to small test cases-- for one, if you encounter a failure, it does not hide other problems down the test case. Second it can quickly clue you to the nature of the failure. The comments in the test provide a handy way to split up the test case; i.e. // ?s in group by // group by on long varchar type // having clause cannot contain column references which // are not grouping columns If a test case called testParamInGroupBy fails, the person investigating the failure has a lot more to go by then a failure in testGroupBy.
          Hide
          Bryan Pendleton added a comment -

          Attached is convertToJUnit.[stat,diff], a patch proposal.

          I took groupBy.sql and ran it through the DERBY-2151 converter, and it produced a nearly perfect file. (Wow!) I took the resulting file and merged the code into the existing GroupByTest.java, to make a (much larger) GroupByTest.java which tests GROUP BY processing in JUnit. I also removed the old groupBy.sql and its master files, and removed it from langScripts.

          The new GroupByTest runs cleanly as a standalone JUnit test, and my overall runs of derbyall and suites.All were also clean.

          Please have a look if you have a chance.

          Show
          Bryan Pendleton added a comment - Attached is convertToJUnit. [stat,diff] , a patch proposal. I took groupBy.sql and ran it through the DERBY-2151 converter, and it produced a nearly perfect file. (Wow!) I took the resulting file and merged the code into the existing GroupByTest.java, to make a (much larger) GroupByTest.java which tests GROUP BY processing in JUnit. I also removed the old groupBy.sql and its master files, and removed it from langScripts. The new GroupByTest runs cleanly as a standalone JUnit test, and my overall runs of derbyall and suites.All were also clean. Please have a look if you have a chance.
          Hide
          Bryan Pendleton added a comment -

          Thanks Kathey, I'll have a look. In the converted code that I've looked at so far,
          the DERBY-2151 tool seems to generate code like:

          assertStatementError("42Y35", st,
          "select * from t1 having 1 between 1 and 2");

          which seems to neatly avoid the problem you are concerned about, since
          assertStatementError does the right thing.

          Show
          Bryan Pendleton added a comment - Thanks Kathey, I'll have a look. In the converted code that I've looked at so far, the DERBY-2151 tool seems to generate code like: assertStatementError("42Y35", st, "select * from t1 having 1 between 1 and 2"); which seems to neatly avoid the problem you are concerned about, since assertStatementError does the right thing.
          Hide
          Kathey Marsden added a comment -

          Thanks Bryan for converting these tests. In using the DERBY-2152 conversion tool, can you double check that it does not omit the fail assert method after a method call in a try-catch block when the statement is expected to fail. e.g. instead of

          try

          { s.execute(command); }

          catch (SQLException e)

          { assertSQLState("42502", e); }


          It should be:

          try { s.execute(command); fail("Command expected to fail" + command); }
          catch (SQLException e){ assertSQLState("42502", e); }
          Show
          Kathey Marsden added a comment - Thanks Bryan for converting these tests. In using the DERBY-2152 conversion tool, can you double check that it does not omit the fail assert method after a method call in a try-catch block when the statement is expected to fail. e.g. instead of try { s.execute(command); } catch (SQLException e) { assertSQLState("42502", e); } It should be: try { s.execute(command); fail("Command expected to fail" + command); } catch (SQLException e){ assertSQLState("42502", e); }

            People

            • Assignee:
              Bryan Pendleton
              Reporter:
              Bryan Pendleton
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development