Derby
  1. Derby
  2. DERBY-2304

Convert derbynet/callable.java to junit

    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

      Description

      Convert the derbynet.callable.java test to the junit framework.

      1. derby-2304-preview.diff
        14 kB
        Jean T. Anderson
      2. derby-2304-preview-2.diff
        12 kB
        Jean T. Anderson
      3. derby-2304-preview-3.diff
        13 kB
        Jean T. Anderson
      4. derby-2304.diff
        31 kB
        Jean T. Anderson

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        25d 23h 3m 1 Jean T. Anderson 05/Mar/07 17:06
        Gavin made changes -
        Workflow jira [ 12396011 ] Default workflow, editable Closed status [ 12797653 ]
        Jean T. Anderson made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 10.3.0.0 [ 12310800 ]
        Resolution Fixed [ 1 ]
        Hide
        Jean T. Anderson added a comment -

        Base conversion to junit is complete and the old test has been removed.

        Show
        Jean T. Anderson added a comment - Base conversion to junit is complete and the old test has been removed.
        Hide
        Jean T. Anderson added a comment -

        Dan wrote:
        : Jean> Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169())
        :
        : That should really be if (JDBC.vmSupportsJDBC2()) since the fixture is testing methods that are in JDBC 2.

        thanks for the clarification, Dan! That nicely declutters the suite code.

        Show
        Jean T. Anderson added a comment - Dan wrote: : Jean> Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169()) : : That should really be if (JDBC.vmSupportsJDBC2()) since the fixture is testing methods that are in JDBC 2. thanks for the clarification, Dan! That nicely declutters the suite code.
        Hide
        Daniel John Debrunner added a comment -

        Jean> Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169())

        That should really be if (JDBC.vmSupportsJDBC2()) since the fixture is testing methods that are in JDBC 2.

        Show
        Daniel John Debrunner added a comment - Jean> Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169()) That should really be if (JDBC.vmSupportsJDBC2()) since the fixture is testing methods that are in JDBC 2.
        Jean T. Anderson made changes -
        Attachment derby-2304.diff [ 12352201 ]
        Hide
        Jean T. Anderson added a comment -

        (Never go on vacation – it sets you behind by the amount you spent off * 2.)

        I'm attaching derby-2304.diff, which incorporates the feedback I've received:

        1) Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169())
        2) Tests that need DriverManager only run if (JDBC.vmSupportsJDBC2())
        3) The assertDecimalSameValue method wraps the BigDecimal.compareTo() call (I'll pass for now on trying to get an equality check working).

        I think one of the tests from derbynet/callable.java is dubious and I would appreciate opinions – I included it in the patch as testSystemOutPrintlnProc() . The sql procedure it invokes just does this:

        System.out.println("I'm doing something here...");

        So, of course, the window in which you run the junit test gets:
        I'm doing something here...
        I'm doing something here...

        I somehow suspect System.out.println in a junit test isn't good, but I thought I'd ask. If there's no feedback, I'm inclined to commit these changes with that test disabled.
        And, of course, I'd appreciate any other observations anyone has.

        Show
        Jean T. Anderson added a comment - (Never go on vacation – it sets you behind by the amount you spent off * 2.) I'm attaching derby-2304.diff, which incorporates the feedback I've received: 1) Tests that get/set BigDecimal only run if (!JDBC.vmSupportsJSR169()) 2) Tests that need DriverManager only run if (JDBC.vmSupportsJDBC2()) 3) The assertDecimalSameValue method wraps the BigDecimal.compareTo() call (I'll pass for now on trying to get an equality check working). I think one of the tests from derbynet/callable.java is dubious and I would appreciate opinions – I included it in the patch as testSystemOutPrintlnProc() . The sql procedure it invokes just does this: System.out.println("I'm doing something here..."); So, of course, the window in which you run the junit test gets: I'm doing something here... I'm doing something here... I somehow suspect System.out.println in a junit test isn't good, but I thought I'd ask. If there's no feedback, I'm inclined to commit these changes with that test disabled. And, of course, I'd appreciate any other observations anyone has.
        Hide
        Jean T. Anderson added a comment -

        Yes, you're absolutely right about the naming confusion. Naming the method I have working so far, which uses compareTo, "assertDecimalSameValue" would make far more sense.

        So far I haven't had success with an equality operation, even with variations on scale and roundingMode as in:
        BigDecimal expected = (new BigDecimal(val1));
        expected = expected.setScale(4); // the procedure uses DECIMAL(14,4)
        or:
        expected = expected.setScale(4, BigDecimal.ROUND_HALF_UP); // 8 rounding modes
        then:
        assertEquals(msg, val1, val2);

        I'll keep working on an equality check.

        Show
        Jean T. Anderson added a comment - Yes, you're absolutely right about the naming confusion. Naming the method I have working so far, which uses compareTo, "assertDecimalSameValue" would make far more sense. So far I haven't had success with an equality operation, even with variations on scale and roundingMode as in: BigDecimal expected = (new BigDecimal(val1)); expected = expected.setScale(4); // the procedure uses DECIMAL(14,4) or: expected = expected.setScale(4, BigDecimal.ROUND_HALF_UP); // 8 rounding modes then: assertEquals(msg, val1, val2); I'll keep working on an equality check.
        Hide
        Daniel John Debrunner added a comment -

        Using BigDecimal.compareTo() in the assertDecimalEquals might cause confusion in the future. See my earlier comment about having two methods.

        Show
        Daniel John Debrunner added a comment - Using BigDecimal.compareTo() in the assertDecimalEquals might cause confusion in the future. See my earlier comment about having two methods.
        Hide
        Jean T. Anderson added a comment -

        Thanks for the J2ME comments, Myrna! I suspect that for the moment I'm best off excluding JSR169 until I have the entire test converted and can see what is and is not supported. Thanks for pointing it out.

        Show
        Jean T. Anderson added a comment - Thanks for the J2ME comments, Myrna! I suspect that for the moment I'm best off excluding JSR169 until I have the entire test converted and can see what is and is not supported. Thanks for pointing it out.
        Hide
        Jean T. Anderson added a comment -

        Following up on Dan's suggestion to implement a assertDecimalEquals method, this works:

        public void assertDecimalEquals (String msg, String val1, BigDecimal val2)

        { BigDecimal expected = (new BigDecimal(val1)); assertTrue(msg + " expected:<" + val1 + "> but was:<" + val2.toString() + ">", expected.compareTo(val2)==0); }

        It gets called like this:

        assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2));

        If an assert fails, it outputs a message that is consistent with the style of other assertEquals messages:

        OUT 9 expected: <-88888888.0000> but was <-99999999.0000>

        Does anyone spot any problems with this approach?

        Show
        Jean T. Anderson added a comment - Following up on Dan's suggestion to implement a assertDecimalEquals method, this works: public void assertDecimalEquals (String msg, String val1, BigDecimal val2) { BigDecimal expected = (new BigDecimal(val1)); assertTrue(msg + " expected:<" + val1 + "> but was:<" + val2.toString() + ">", expected.compareTo(val2)==0); } It gets called like this: assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2)); If an assert fails, it outputs a message that is consistent with the style of other assertEquals messages: OUT 9 expected: <-88888888.0000> but was <-99999999.0000> Does anyone spot any problems with this approach?
        Hide
        Myrna van Lunteren added a comment -

        I looked at patch 3 and have 2 comments related to running with j2ME...

        • the test proc updateLongVarbinaryProc (byte[] in_param) uses
          Connection conn = DriverManager.getConnection("jdbc:default:connection");
          Used in testUpdateLongBinaryProc.
          I am wondering if it is possible to change this to not use DriverManager.
          If that were possible, that subtest theoretically could run in j2ME environment, I think.
        • because of the call to cs.getBigDecimal() in testBigDecimalInAndOutParameters that subtest will fail with JSR169/j2ME.

        If the subtests that fail for the reasons above could get a if (!JDBC.vmSupportsJSR169()) block, then this test can be added where it is in jdbcapi._Suite.java; otherwise, it the entire test needs to be only added if the jvm is not one supporting only JSR169.

        For the rest, this looks good to me, and passes for me with jdk14.

        Myrna

        Show
        Myrna van Lunteren added a comment - I looked at patch 3 and have 2 comments related to running with j2ME... the test proc updateLongVarbinaryProc (byte[] in_param) uses Connection conn = DriverManager.getConnection("jdbc:default:connection"); Used in testUpdateLongBinaryProc. I am wondering if it is possible to change this to not use DriverManager. If that were possible, that subtest theoretically could run in j2ME environment, I think. because of the call to cs.getBigDecimal() in testBigDecimalInAndOutParameters that subtest will fail with JSR169/j2ME. If the subtests that fail for the reasons above could get a if (!JDBC.vmSupportsJSR169()) block, then this test can be added where it is in jdbcapi._Suite.java; otherwise, it the entire test needs to be only added if the jvm is not one supporting only JSR169. For the rest, this looks good to me, and passes for me with jdk14. Myrna
        Hide
        Jean T. Anderson added a comment -

        Dan asked:
        > On the assertDecimalEquals() how were you comparing the string to the BigDecimal?

        I goofed – I was moving too quickly and didn't pay close enough attention to your assertDecimalEquals() suggestion.

        I did this, which was silly:
        assertEquals("OUT 2", "33.3330", cs.getBigDecimal(2));

        This code passes on 1.4.2, but it can't be reliable:
        assertEquals("OUT 2", "33.3330", cs.getBigDecimal(2).toString());

        I'll slow down and work on a real decimal comparison solution. Thanks for all the help!

        Show
        Jean T. Anderson added a comment - Dan asked: > On the assertDecimalEquals() how were you comparing the string to the BigDecimal? I goofed – I was moving too quickly and didn't pay close enough attention to your assertDecimalEquals() suggestion. I did this, which was silly: assertEquals("OUT 2", "33.3330", cs.getBigDecimal(2)); This code passes on 1.4.2, but it can't be reliable: assertEquals("OUT 2", "33.3330", cs.getBigDecimal(2).toString()); I'll slow down and work on a real decimal comparison solution. Thanks for all the help!
        Jean T. Anderson made changes -
        Attachment derby-2304-preview-3.diff [ 12350601 ]
        Hide
        Jean T. Anderson added a comment -

        Don't look at derby-2304-preview-2.diff – it has confusing cruft leftover from my initial teardown implementation [1].

        derby-2304-preview-3.diff cleans that up.

        [1] For newbies coming after me, my first shot at CallableTest.java explicitly coded both setUp() and tearDown(). But I switched the strategy to use CleanDatabaseTestSetup() instead, which automatically does the tearDown, eliminating any need to provide code to drop database objects.

        Show
        Jean T. Anderson added a comment - Don't look at derby-2304-preview-2.diff – it has confusing cruft leftover from my initial teardown implementation [1] . derby-2304-preview-3.diff cleans that up. [1] For newbies coming after me, my first shot at CallableTest.java explicitly coded both setUp() and tearDown(). But I switched the strategy to use CleanDatabaseTestSetup() instead, which automatically does the tearDown, eliminating any need to provide code to drop database objects.
        Hide
        Daniel John Debrunner added a comment -

        On the assertDecimalEquals() how were you comparing the string to the BigDecimal?
        This assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2)); is possible but it might need careful definition as to what equality means.
        basically the same numerical value or the same numerical value with the same scale. E.g.

        32.00
        32

        are the same for BigDecimal.compareTo() but are not for BigDecimal.equals().

        Maybe have two methods ...

        assertDecimalSameValue (uses compareTo)
        assertDecimalEquals (uses equals)

        Show
        Daniel John Debrunner added a comment - On the assertDecimalEquals() how were you comparing the string to the BigDecimal? This assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2)); is possible but it might need careful definition as to what equality means. basically the same numerical value or the same numerical value with the same scale. E.g. 32.00 32 are the same for BigDecimal.compareTo() but are not for BigDecimal.equals(). Maybe have two methods ... assertDecimalSameValue (uses compareTo) assertDecimalEquals (uses equals)
        Jean T. Anderson made changes -
        Attachment derby-2304-preview-2.diff [ 12350599 ]
        Hide
        Jean T. Anderson added a comment -

        derby-2304-preview-2.diff incorporates feedback from Dan.

        Show
        Jean T. Anderson added a comment - derby-2304-preview-2.diff incorporates feedback from Dan.
        Hide
        Jean T. Anderson added a comment -

        Thanks for the speedy comments, Dan!

        R1) Regarding the teardown, I was confused by what was happening under the hood and when.
        The 'DROP ROUTINE' error I encountered sounds like something I should reproduce separately and log as an issue.

        R2) I really like your approach that avoids explicitly adding all fixtures.

        R3) Excellent – thanks!

        R4) assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2));

        That still resulted in differences detected:
        OUT 2 expected:<33.3330> but was:<33.3330>

        However, changing the last arg to string worked:

        assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2).toString());

        thanks for the feedback!

        Show
        Jean T. Anderson added a comment - Thanks for the speedy comments, Dan! R1) Regarding the teardown, I was confused by what was happening under the hood and when. The 'DROP ROUTINE' error I encountered sounds like something I should reproduce separately and log as an issue. R2) I really like your approach that avoids explicitly adding all fixtures. R3) Excellent – thanks! R4) assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2)); That still resulted in differences detected: OUT 2 expected:<33.3330> but was:<33.3330> However, changing the last arg to string worked: assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2).toString()); thanks for the feedback!
        Hide
        Daniel John Debrunner added a comment -

        I think this test shows a useful common pattern:

        some SQL schema setup run once for a set of fixtures (CleanDatabaseTestSetup.decorateSQL)
        emptying the tables for each fixture (the setUp method)

        Myrna's batchUpdate() test in DERBY-2293 I think shows the same requirement.
        ProcedureTest has the same pattern.

        Having a utility method to perform the emptying (all the DELETE FROM TABLE commands) without having to explicitly list the tables in the test would be useful. Currently we have tests implementing their own mechanisms and I think the ProcedureTest approach is somewhat awkward in having to specify the table name twice in the list.

        Something like
        JDBC.emptyAllTables(Connection conn)
        it could pull logic from the code that drops all tables.

        Show
        Daniel John Debrunner added a comment - I think this test shows a useful common pattern: some SQL schema setup run once for a set of fixtures (CleanDatabaseTestSetup.decorateSQL) emptying the tables for each fixture (the setUp method) Myrna's batchUpdate() test in DERBY-2293 I think shows the same requirement. ProcedureTest has the same pattern. Having a utility method to perform the emptying (all the DELETE FROM TABLE commands) without having to explicitly list the tables in the test would be useful. Currently we have tests implementing their own mechanisms and I think the ProcedureTest approach is somewhat awkward in having to specify the table name twice in the list. Something like JDBC.emptyAllTables(Connection conn) it could pull logic from the code that drops all tables.
        Hide
        Daniel John Debrunner added a comment -

        A1) all the fixtures will be run with autocommit false, since you set that in the setup method. That may be leaving held result sets open though I couldn't see any missing closes, so maybe it's a bug.
        I wasn't clear what you meant on no teardown, your CleanDatabaseTestSetup method has a tearDown method that will drop all the objects in the database, so if the test is passing the routines must be being dropped without error.

        A2) The suite method is correct, the concept of being in a framework doesn't really exist anymore, at least not during the calls to suite(). The class needs to define what primary configurations the test will run in, this is driven by the suite method itself, not be any external framework setting. Thus the test is saying I want to run these fixtures in embedded and these in client/server.

        Note also that fixtures do not have to begin with test. One can use 'test' for methods that run in both configurations and thus use defaultSuite and then add other fixtures explicitly, e.g. embededdTestOneInOneOutFunc. If only one or two fixtures are "abnormal" then this is a better approach than adding all fixtures explicitly.

        A3) An alternative would be to use java.util.Arrays.equals(), something like

        assertTrue(Arrays.equals(b1, b2))

        A4) is because you are passing an imprecise double to BigDecimal() and BigDecimal is correctly reflecting the double value. You can use a String to create the BigDecimal, e..g new BigDecimal("33.3330");
        One could simplify the method by creating an assertDecimalEquals() method, that took the string and created the BigDecimal within the method. e.g. called as:

        assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2));

        Show
        Daniel John Debrunner added a comment - A1) all the fixtures will be run with autocommit false, since you set that in the setup method. That may be leaving held result sets open though I couldn't see any missing closes, so maybe it's a bug. I wasn't clear what you meant on no teardown, your CleanDatabaseTestSetup method has a tearDown method that will drop all the objects in the database, so if the test is passing the routines must be being dropped without error. A2) The suite method is correct, the concept of being in a framework doesn't really exist anymore, at least not during the calls to suite(). The class needs to define what primary configurations the test will run in, this is driven by the suite method itself, not be any external framework setting. Thus the test is saying I want to run these fixtures in embedded and these in client/server. Note also that fixtures do not have to begin with test. One can use 'test' for methods that run in both configurations and thus use defaultSuite and then add other fixtures explicitly, e.g. embededdTestOneInOneOutFunc. If only one or two fixtures are "abnormal" then this is a better approach than adding all fixtures explicitly. A3) An alternative would be to use java.util.Arrays.equals(), something like assertTrue(Arrays.equals(b1, b2)) A4) is because you are passing an imprecise double to BigDecimal() and BigDecimal is correctly reflecting the double value. You can use a String to create the BigDecimal, e..g new BigDecimal("33.3330"); One could simplify the method by creating an assertDecimalEquals() method, that took the string and created the BigDecimal within the method. e.g. called as: assertDecimalEquals("OUT 2", "33.3330", cs.getBigDecimal(2));
        Jean T. Anderson made changes -
        Field Original Value New Value
        Attachment derby-2304-preview.diff [ 12350580 ]
        Hide
        Jean T. Anderson added a comment -

        I'm working on a CallableTest.java to convert derbynet/callable.java to junit. derby-2304-preview.diff is way incomplete, but I'm looking for feedback on the overall structure and implementation so far. I summarized issues down below.

        So far, CallableTest.java implements just 3 tests (and only runs 2 tests – see issue #4 down below).

        The code tries to model itself after DatabaseMetaDataTest.java for reducing try {} clutter, and after ProcedureTest.java for its basic setup, which creates all the SQL objects up front and doesn't tear them down – and also enables one of the tests to run in the client framework (see issue #1). I may have gone overboard in some cases and misunderstood stuff so please be picky!

        I'm reorganizing the original test in the following ways:

        • Move it from derbynet to jdbcacpi because I think it should be able to run under both embedded and client frameworks.
        • Carve it up into smaller tests.
        • Create more descriptive function/procedure/method/table names like ProcedureTest.java does; for example, instead of "CREATE FUNCTION method2" calling "method2" I have "CREATE FUNCTION ONE_IN_ONE_OUT_FUNC" which calls "OneInOneOutFunc" and the method that tests it is "testOneInOneOutFunc".

        I ran into these issues:

        1) derbynet/callable.java
        Line 92 of the original test comments out the cleanup:
        // stmt.execute("DROP FUNCTION method2");
        I discovered that this DROP statement works fine in embedded, but raises this exception in the client:

        'DROP ROUTINE' cannot be performed on object 'ONE_IN_INE_OUT_FUNC' because there is an open resultSet dependent on that object."

        Does this error ring a bell with anyone?

        ProcedureTest.java sets up everything at the start and there is no teardown, so that's the approach I adopted with CallableTest.java. That approach enables this test to run without error (the error did occur with my first attempt that implemented create/drops in setup() and tearDown() methods). But I'm uncomfortable about masking the DROP error, wonder if the current strategy will be solid for all test scenarios.

        2) Because of the client problem in #1 I initially set up baseSuite() to not run that test in client; however, I discovered that usingEmbedded() is always true, so I did a "Hack!" doing a string compare on embedded / client. Is this the wrong place to exclude the run of a specific test for a given framework?

        3) I created a local assertEquals(byte[] b1, byte[] b2) for the byte comparison in testUpdateLongBinaryProc() . Is there already a way to get support for byte comparisons? I didn't spot any. If not, is this something I should move to BaseJDBCTestCase.java (and also completely implement)?

        4) testBigDecimalInAndOutParameters() doesn't get called yet – I need to spin up on how to do assertEquals with BigDecimal.

        For example, this assert:
        assertEquals("OUT 2", new BigDecimal(33.3330), cs.getBigDecimal(2));

        generates this:
        OUT 2 expected:<33.33299999999999999840838> but was:<33.3330>

        I'll probably start a thread on derby-dev to ask about the original objectives of that test. I grew a little puzzled looking at it.

        Show
        Jean T. Anderson added a comment - I'm working on a CallableTest.java to convert derbynet/callable.java to junit. derby-2304-preview.diff is way incomplete, but I'm looking for feedback on the overall structure and implementation so far. I summarized issues down below. So far, CallableTest.java implements just 3 tests (and only runs 2 tests – see issue #4 down below). The code tries to model itself after DatabaseMetaDataTest.java for reducing try {} clutter, and after ProcedureTest.java for its basic setup, which creates all the SQL objects up front and doesn't tear them down – and also enables one of the tests to run in the client framework (see issue #1). I may have gone overboard in some cases and misunderstood stuff so please be picky! I'm reorganizing the original test in the following ways: Move it from derbynet to jdbcacpi because I think it should be able to run under both embedded and client frameworks. Carve it up into smaller tests. Create more descriptive function/procedure/method/table names like ProcedureTest.java does; for example, instead of "CREATE FUNCTION method2" calling "method2" I have "CREATE FUNCTION ONE_IN_ONE_OUT_FUNC" which calls "OneInOneOutFunc" and the method that tests it is "testOneInOneOutFunc". I ran into these issues: 1) derbynet/callable.java Line 92 of the original test comments out the cleanup: // stmt.execute("DROP FUNCTION method2"); I discovered that this DROP statement works fine in embedded, but raises this exception in the client: 'DROP ROUTINE' cannot be performed on object 'ONE_IN_INE_OUT_FUNC' because there is an open resultSet dependent on that object." Does this error ring a bell with anyone? ProcedureTest.java sets up everything at the start and there is no teardown, so that's the approach I adopted with CallableTest.java. That approach enables this test to run without error (the error did occur with my first attempt that implemented create/drops in setup() and tearDown() methods). But I'm uncomfortable about masking the DROP error, wonder if the current strategy will be solid for all test scenarios. 2) Because of the client problem in #1 I initially set up baseSuite() to not run that test in client; however, I discovered that usingEmbedded() is always true, so I did a "Hack!" doing a string compare on embedded / client. Is this the wrong place to exclude the run of a specific test for a given framework? 3) I created a local assertEquals(byte[] b1, byte[] b2) for the byte comparison in testUpdateLongBinaryProc() . Is there already a way to get support for byte comparisons? I didn't spot any. If not, is this something I should move to BaseJDBCTestCase.java (and also completely implement)? 4) testBigDecimalInAndOutParameters() doesn't get called yet – I need to spin up on how to do assertEquals with BigDecimal. For example, this assert: assertEquals("OUT 2", new BigDecimal(33.3330), cs.getBigDecimal(2)); generates this: OUT 2 expected:<33.33299999999999999840838> but was:<33.3330> I'll probably start a thread on derby-dev to ask about the original objectives of that test. I grew a little puzzled looking at it.
        Jean T. Anderson created issue -

          People

          • Assignee:
            Jean T. Anderson
            Reporter:
            Jean T. Anderson
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development