Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None
    1. derby-2491-pre.diff
      14 kB
      Andrew McIntyre
    2. derby-2491-v1.stat
      1 kB
      Andrew McIntyre
    3. derby-2491-v1.diff
      847 kB
      Andrew McIntyre

      Activity

      Hide
      Knut Anders Hatlen added a comment -

      Thanks for your comments, Andrew! I agree that the redundancy of the queries gives us the needed confidence. As to the ordering of the results, I have just committed a new assert method (assertUnorderedResultSet) as part of DERBY-2493. It takes the same parameters as assertFullResultSet and ignores the order of the results. I also updated one of the test cases in DistinctTest that would have failed because of DERBY-2493.

      Show
      Knut Anders Hatlen added a comment - Thanks for your comments, Andrew! I agree that the redundancy of the queries gives us the needed confidence. As to the ordering of the results, I have just committed a new assert method (assertUnorderedResultSet) as part of DERBY-2493 . It takes the same parameters as assertFullResultSet and ignores the order of the results. I also updated one of the test cases in DistinctTest that would have failed because of DERBY-2493 .
      Hide
      Andrew McIntyre added a comment -

      Committed new test to repository with revision 523546, closing.

      Show
      Andrew McIntyre added a comment - Committed new test to repository with revision 523546, closing.
      Hide
      Andrew McIntyre added a comment -

      Whoops! Meant to attach my response to the review comments to this issue, but accidentally ended up attaching them to DERBY-2493. Please see the comments in that issue if interested.

      Show
      Andrew McIntyre added a comment - Whoops! Meant to attach my response to the review comments to this issue, but accidentally ended up attaching them to DERBY-2493 . Please see the comments in that issue if interested.
      Hide
      Knut Anders Hatlen added a comment -

      If I have read the patch correctly, DistinctTest.checkDistinctRows only tests that the queries return the correct number of rows. Are we confident that the other aspects of SELECT DISTINCT (for instance that the returned values in fact are distinct and the ones we expect) are tested well enough by other tests?

      What I do like about the approach is that the test doesn't fail if the order of the rows changes, since I am planning to post a patch for DERBY-2493 which will change the order.

      Some tiny nits:

      • checkDistinctRows() could be private, I think
      • in checkDistinctRows(), "if (rowcounts.length != 37) fail(...)" could be changed to assertEquals
      • in javadoc for RuntimeStatisticsParser.indexOf(): "founf" -> "found"
      Show
      Knut Anders Hatlen added a comment - If I have read the patch correctly, DistinctTest.checkDistinctRows only tests that the queries return the correct number of rows. Are we confident that the other aspects of SELECT DISTINCT (for instance that the returned values in fact are distinct and the ones we expect) are tested well enough by other tests? What I do like about the approach is that the test doesn't fail if the order of the rows changes, since I am planning to post a patch for DERBY-2493 which will change the order. Some tiny nits: checkDistinctRows() could be private, I think in checkDistinctRows(), "if (rowcounts.length != 37) fail(...)" could be changed to assertEquals in javadoc for RuntimeStatisticsParser.indexOf(): "founf" -> "found"
      Hide
      Andrew McIntyre added a comment -

      Accidentally redirected stat output over my diff file, attaching the correct diff here.

      Show
      Andrew McIntyre added a comment - Accidentally redirected stat output over my diff file, attaching the correct diff here.
      Hide
      Andrew McIntyre added a comment -

      Stat file

      Show
      Andrew McIntyre added a comment - Stat file
      Hide
      Andrew McIntyre added a comment -

      Attaching converted test for this issue, note that one testcase for group by that somehow ended up in distinct.sql was moved over to a new GroupByTest. Please review, if there are no comments, I'll commit this tomorrow morning after adding a few more javadoc comments.

      Show
      Andrew McIntyre added a comment - Attaching converted test for this issue, note that one testcase for group by that somehow ended up in distinct.sql was moved over to a new GroupByTest. Please review, if there are no comments, I'll commit this tomorrow morning after adding a few more javadoc comments.
      Hide
      Andrew McIntyre added a comment -

      Will include distinctElimination and distinctFiltering in this conversion.

      Show
      Andrew McIntyre added a comment - Will include distinctElimination and distinctFiltering in this conversion.
      Hide
      Andrew McIntyre added a comment -

      Attaching a preliminary patch for this issue. This converts all of the testcases that utilize the distinct.subsql script, which accounts for about 75% of the test. I'll follow up with the remaining testcases tomorrow. If anyone has any suggestions on how to improve the test so that it is more readable / debuggable, please let me know.

      Show
      Andrew McIntyre added a comment - Attaching a preliminary patch for this issue. This converts all of the testcases that utilize the distinct.subsql script, which accounts for about 75% of the test. I'll follow up with the remaining testcases tomorrow. If anyone has any suggestions on how to improve the test so that it is more readable / debuggable, please let me know.

        People

        • Assignee:
          Andrew McIntyre
          Reporter:
          Andrew McIntyre
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development