Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: Test
    • Labels:
      None

      Description

      Convert casting.java to Junit

      1. DERBY_2382.diff
        32 kB
        Kathey Marsden

        Activity

        Hide
        Kathey Marsden added a comment -

        DERBY_2382.diff has the new test CastingTest.java which will replace casting.java.
        Currently the test checks only whether the casts succeed and that results can be drained or fails. I would like opinions on whether checking the values returned for each cast is required or whether it is ok just to verify the success of the cast.

        Show
        Kathey Marsden added a comment - DERBY_2382.diff has the new test CastingTest.java which will replace casting.java. Currently the test checks only whether the casts succeed and that results can be drained or fails. I would like opinions on whether checking the values returned for each cast is required or whether it is ok just to verify the success of the cast.
        Hide
        Knut Anders Hatlen added a comment -

        Since the old test verifies that the values are correct, I think the new one should do that too, unless it's covered by other tests. It's kind of important that the returned value of a cast is correct...

        Some minor nits:

        • tearDown() should call super.tearDown()
        • in some cases fail() is used to report an unexpected exception. I think it's better just to (re-)throw the exception so that we preserve the stack trace.
        • some uses of fail, like
          + if (!isSupportedAssignment(sourceType,targetType))
          + fail(description + "should not succeed");
          would perhaps be clearer if they were written as assertTrue(...)
        • the indentation is a bit inconsistent (some places 4 characters, but most places 8)
        • most of the "public static" methods could be "private static", I think
        • the test methods are declared as "throws SQLException, Throwable". I don't think the Throwable is needed (but I haven't checked, so I might be wrong)
        Show
        Knut Anders Hatlen added a comment - Since the old test verifies that the values are correct, I think the new one should do that too, unless it's covered by other tests. It's kind of important that the returned value of a cast is correct... Some minor nits: tearDown() should call super.tearDown() in some cases fail() is used to report an unexpected exception. I think it's better just to (re-)throw the exception so that we preserve the stack trace. some uses of fail, like + if (!isSupportedAssignment(sourceType,targetType)) + fail(description + "should not succeed"); would perhaps be clearer if they were written as assertTrue(...) the indentation is a bit inconsistent (some places 4 characters, but most places 8) most of the "public static" methods could be "private static", I think the test methods are declared as "throws SQLException, Throwable". I don't think the Throwable is needed (but I haven't checked, so I might be wrong)
        Hide
        Kathey Marsden added a comment -

        Thanks Knut for the review. cast.sql covers most of the output, but likely doesn't have as many permutations. I wrote the test originally to test what was a legal cast and what was not but I'll look at the test some more to see how I might best check the output.

        Show
        Kathey Marsden added a comment - Thanks Knut for the review. cast.sql covers most of the output, but likely doesn't have as many permutations. I wrote the test originally to test what was a legal cast and what was not but I'll look at the test some more to see how I might best check the output.
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for addressing my comments. One more thing about tearDown(): The call to super.tearDown() needs to be moved to the end of the method. Since getConnection() is called after super.tearDown(), the connection is reinitialized and won't be garbage collected.

        Show
        Knut Anders Hatlen added a comment - Thanks for addressing my comments. One more thing about tearDown(): The call to super.tearDown() needs to be moved to the end of the method. Since getConnection() is called after super.tearDown(), the connection is reinitialized and won't be garbage collected.
        Hide
        Kathey Marsden added a comment -

        Committed revision 514846

        Show
        Kathey Marsden added a comment - Committed revision 514846
        Hide
        Knut Anders Hatlen added a comment -

        I moved the call to super.tearDown() to the end of tearDown() and added some calls to ResultSet.close(). Committed revision 515490.

        Show
        Knut Anders Hatlen added a comment - I moved the call to super.tearDown() to the end of tearDown() and added some calls to ResultSet.close(). Committed revision 515490.
        Hide
        Daniel John Debrunner added a comment -

        Can casting.java be removed now the test is converted? I just thought it was a valid test and ran it, but it fails because there's no master file.

        Show
        Daniel John Debrunner added a comment - Can casting.java be removed now the test is converted? I just thought it was a valid test and ran it, but it fails because there's no master file.

          People

          • Assignee:
            Kathey Marsden
            Reporter:
            Kathey Marsden
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development