Derby
  1. Derby
  2. DERBY-5156

convert store/longColumn.sql into junit test case

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: Test
    • Labels:
    1. DERBY-5156-2.stat
      0.5 kB
      Houx Zhang
    2. DERBY-5156-2.patch
      115 kB
      Houx Zhang
    3. DERBY-5156-1.stat
      0.5 kB
      Houx Zhang
    4. DERBY-5156-1.patch
      103 kB
      Houx Zhang
    5. DERBY-5156.patch
      31 kB
      Houx Zhang

      Issue Links

        Activity

        Hide
        Houx Zhang added a comment -

        Thanks for your advice and checking, Bryan.

        Show
        Houx Zhang added a comment - Thanks for your advice and checking, Bryan.
        Hide
        Bryan Pendleton added a comment -

        Thanks for the updated patch, it looks great. Committed to the trunk as revision 1137258.

        Show
        Bryan Pendleton added a comment - Thanks for the updated patch, it looks great. Committed to the trunk as revision 1137258.
        Hide
        Bryan Pendleton added a comment -

        Thanks Houx! I will have a look

        Show
        Bryan Pendleton added a comment - Thanks Houx! I will have a look
        Hide
        Houx Zhang added a comment -

        I have adopted your two advices, please check it!

        Show
        Houx Zhang added a comment - I have adopted your two advices, please check it!
        Hide
        Dag H. Wanvik added a comment -

        Looking briefly at this test, it looks to me as though the results here are significant part of the test. I'd recommend including it.

        Show
        Dag H. Wanvik added a comment - Looking briefly at this test, it looks to me as though the results here are significant part of the test. I'd recommend including it.
        Hide
        Bryan Pendleton added a comment -

        The patch looks very good to me. I have two thoughts:

        1) In a few places, the test deliberately causes a failure, using code like:

        try

        { st.executeUpdate("create index zz on testing (a)"); fail("try creating btree index on long column, should fail"); }

        catch (SQLException e)

        { assertTrue(true); }

        It seems like it would be nice if the body of the "catch" statement verified
        that we received the expected exception. That is, instead of "assertTrue(true)",
        the test said something like assertSQLState("XSCB6", e);

        2) I think that the existing test runs queries, and verifies not only that the
        number of rows is correct, but also that the contents of the column values
        that are returned by the SELECT statement match the expected results.
        The new test appears to only check the number of rows returned. Is this
        sufficient, for the purposes of this test? (for the other developers reading this,
        please offer an opinion about whether this test will test the long column
        behaviors adequately by checking the number of rows in the result set)

        Show
        Bryan Pendleton added a comment - The patch looks very good to me. I have two thoughts: 1) In a few places, the test deliberately causes a failure, using code like: try { st.executeUpdate("create index zz on testing (a)"); fail("try creating btree index on long column, should fail"); } catch (SQLException e) { assertTrue(true); } It seems like it would be nice if the body of the "catch" statement verified that we received the expected exception. That is, instead of "assertTrue(true)", the test said something like assertSQLState("XSCB6", e); 2) I think that the existing test runs queries, and verifies not only that the number of rows is correct, but also that the contents of the column values that are returned by the SELECT statement match the expected results. The new test appears to only check the number of rows returned. Is this sufficient, for the purposes of this test? (for the other developers reading this, please offer an opinion about whether this test will test the long column behaviors adequately by checking the number of rows in the result set)
        Hide
        Bryan Pendleton added a comment -

        Thanks Houx, the stat file is very helpful. I am working on reviewing the full patch this week.

        Show
        Bryan Pendleton added a comment - Thanks Houx, the stat file is very helpful. I am working on reviewing the full patch this week.
        Hide
        Houx Zhang added a comment -

        Thanks for your checking and encourage, Bryan.

        I have added a stat file and removed the longColumn.out in DERBY-5156-1.patch, please check it, thanks!

        Show
        Houx Zhang added a comment - Thanks for your checking and encourage, Bryan. I have added a stat file and removed the longColumn.out in DERBY-5156 -1.patch, please check it, thanks!
        Hide
        Bryan Pendleton added a comment -

        The patch looks good to me; the new test is nice and clear. I like the way
        you segmented the testing into separate logical tests to match the original
        SQL test's structure, thanks.

        When you contribute a patch which contains added and/or deleted files, it
        is convenient if you can also include the output of 'svn stat' so it is obvious
        which files are to be added and/or deleted, as that can be hard to tell sometimes
        just from reading the patch.

        Should the patch also delete derbyTesting/functionTests/master/longColumn.out? It
        looks like that might be missing from the patch.

        Show
        Bryan Pendleton added a comment - The patch looks good to me; the new test is nice and clear. I like the way you segmented the testing into separate logical tests to match the original SQL test's structure, thanks. When you contribute a patch which contains added and/or deleted files, it is convenient if you can also include the output of 'svn stat' so it is obvious which files are to be added and/or deleted, as that can be hard to tell sometimes just from reading the patch. Should the patch also delete derbyTesting/functionTests/master/longColumn.out? It looks like that might be missing from the patch.
        Hide
        Houx Zhang added a comment -

        please check the patch, thanks!

        Show
        Houx Zhang added a comment - please check the patch, thanks!

          People

          • Assignee:
            Houx Zhang
            Reporter:
            Houx Zhang
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 168h
              168h
              Remaining:
              Remaining Estimate - 168h
              168h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development