Derby
  1. Derby
  2. DERBY-4317

convert columnDefaults.sql to JUNIT

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.6.1.0
    • Component/s: Test
    • Labels:
      None
    • Issue & fix info:
      Newcomer

      Description

      Conversion of the columnDefaults.sql to JUNIT

        Activity

        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan,
        I make some little changes to the test and after that test ran successfully
        ran in my platform.
        Here I am attaching the patch file.

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan, I make some little changes to the test and after that test ran successfully ran in my platform. Here I am attaching the patch file.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan/Kathey,I did complete the test conversion and testing.
        Here is my final patch for your concern. I think this can commit as a
        revision.

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan/Kathey,I did complete the test conversion and testing. Here is my final patch for your concern. I think this can commit as a revision.
        Hide
        Mamta A. Satoor added a comment -

        Thanks, Eranda, for working on the test conversion.

        One comment I had is rather than have the each test do the table creation as part of the test fixture, which makes the test code intermingle with setup code and hence takes some focus away from the actual test. Instead, may be you can consider using CleanDatabaseTestSetup decorator. One example of this usage of this decorator can be found in GroupByExpression test but there are many other tests who use this decorator. Using this decorator will allow you to overload decorateSQL method in which you can put all the create table/view/function etc sqls.

        If I have any further feedback on the rest of the converted junit test, I will post another comment but thanks again for converting the test into junit.

        Show
        Mamta A. Satoor added a comment - Thanks, Eranda, for working on the test conversion. One comment I had is rather than have the each test do the table creation as part of the test fixture, which makes the test code intermingle with setup code and hence takes some focus away from the actual test. Instead, may be you can consider using CleanDatabaseTestSetup decorator. One example of this usage of this decorator can be found in GroupByExpression test but there are many other tests who use this decorator. Using this decorator will allow you to overload decorateSQL method in which you can put all the create table/view/function etc sqls. If I have any further feedback on the rest of the converted junit test, I will post another comment but thanks again for converting the test into junit.
        Hide
        Mamta A. Satoor added a comment -

        Eranda, I just finished reviewing the test conversion and it looks good. After reviewing the test, I think there are quite a few create tables that you probably just need to keep in your test fixtures since we are testing default at create table time for some of those tests. There may be handful of other table creation which can be moved into decorateSQL since they test defaults at insert/update time. I will leave it to your judgement call if you want to leave all the table creations in their test fixtures for clarity purpose since we are mainly testing for defaults at create and right after create table times.

        Show
        Mamta A. Satoor added a comment - Eranda, I just finished reviewing the test conversion and it looks good. After reviewing the test, I think there are quite a few create tables that you probably just need to keep in your test fixtures since we are testing default at create table time for some of those tests. There may be handful of other table creation which can be moved into decorateSQL since they test defaults at insert/update time. I will leave it to your judgement call if you want to leave all the table creations in their test fixtures for clarity purpose since we are mainly testing for defaults at create and right after create table times.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Mamta,
        Thanks for looking at my tests and your comments. I do understand what you
        said. I'll make the test as you said.

        Show
        Eranda Sooriyabandara added a comment - Hi Mamta, Thanks for looking at my tests and your comments. I do understand what you said. I'll make the test as you said.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan, Mamta,
        I try to use a decorator to overload decorateSQL method in which you can put
        all the create table/view/function. But the problem is if I use that method
        some of the create table must leave as it is now,

        1.assertStatementError("42894", st," create table neg(c1 int default
        asdf(0))");
        These are asserting errors so we can keep them as they are now.

        2. some times the same table names used by dropping the table.
        st.executeUpdate(" drop table neg");
        st.executeUpdate("create table neg(c1 int default 10)");
        This can recover by using other name for the table.

        But the biggest problem is the when we move the create table statements in
        a separate method the readability is reduced(I think), because it is
        checking the default values of the columns.
        We can use alter table add column for this as a solution.

        So the problem what can I do?

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan, Mamta, I try to use a decorator to overload decorateSQL method in which you can put all the create table/view/function. But the problem is if I use that method some of the create table must leave as it is now, 1.assertStatementError("42894", st," create table neg(c1 int default asdf(0))"); These are asserting errors so we can keep them as they are now. 2. some times the same table names used by dropping the table. st.executeUpdate(" drop table neg"); st.executeUpdate("create table neg(c1 int default 10)"); This can recover by using other name for the table. But the biggest problem is the when we move the create table statements in a separate method the readability is reduced(I think), because it is checking the default values of the columns. We can use alter table add column for this as a solution. So the problem what can I do?
        Hide
        Bryan Pendleton added a comment -

        Hi Eranda,

        I think you are right, it may be better to keep most of the create table statements in the test cases themselves,
        since as you say the test is specifically testing the create table statement behavior.

        We often use the decorateSQL pattern for tests which set up some sample data, and then test the
        behavior of the system with the sample data.

        But here we are testing the create table statement itself, so this is a special case.

        If you think that the test is more readable in its current state, then that is fine, I think Mamta agreed
        that this is an unusual situation and the decorateSQL technique may or may not be applicable:
        https://issues.apache.org/jira/browse/DERBY-4317?focusedCommentId=12736829&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12736829

        Show
        Bryan Pendleton added a comment - Hi Eranda, I think you are right, it may be better to keep most of the create table statements in the test cases themselves, since as you say the test is specifically testing the create table statement behavior. We often use the decorateSQL pattern for tests which set up some sample data, and then test the behavior of the system with the sample data. But here we are testing the create table statement itself, so this is a special case. If you think that the test is more readable in its current state, then that is fine, I think Mamta agreed that this is an unusual situation and the decorateSQL technique may or may not be applicable: https://issues.apache.org/jira/browse/DERBY-4317?focusedCommentId=12736829&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12736829
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan,According to me patch is ready to commit.
        If Mamta agree with us you can take a look and can commit the patch.
        Thanks
        Eranda

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan,According to me patch is ready to commit. If Mamta agree with us you can take a look and can commit the patch. Thanks Eranda
        Hide
        Mamta A. Satoor added a comment -

        Yes, I agree that majority of the table creation is for testing default functionality and hence they need to be in the fixture rather than decorateSQL.

        Show
        Mamta A. Satoor added a comment - Yes, I agree that majority of the table creation is for testing default functionality and hence they need to be in the fixture rather than decorateSQL.
        Hide
        Bryan Pendleton added a comment -

        Thanks Mamta, Eranda. I intend to move ahead with reviewing and committing this patch.

        Show
        Bryan Pendleton added a comment - Thanks Mamta, Eranda. I intend to move ahead with reviewing and committing this patch.
        Hide
        Bryan Pendleton added a comment -

        I made two small changes:
        1) Removed columnDefaults.sql from derbylang.runall
        2) Renamed a couple test methods in ColumnDefaultsTest.java so that their
        names start "testXXX" so that JUnit will find them by default.

        I'm re-running the regression suites to verify that the new tests pass. Everything else looks good to me.

        Show
        Bryan Pendleton added a comment - I made two small changes: 1) Removed columnDefaults.sql from derbylang.runall 2) Renamed a couple test methods in ColumnDefaultsTest.java so that their names start "testXXX" so that JUnit will find them by default. I'm re-running the regression suites to verify that the new tests pass. Everything else looks good to me.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan,
        Thanks for the changes that you did in appropriate places.
        I think we can close this issue soon.
        Thanks
        Eranda

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan, Thanks for the changes that you did in appropriate places. I think we can close this issue soon. Thanks Eranda
        Hide
        Bryan Pendleton added a comment -

        Committed the patch to the trunk as svn revision 801492.

        Thank you very much for this contribution, Eranda!

        Please confirm that the updated code looks correct on your machine, and then close this issue at your convenience.

        Show
        Bryan Pendleton added a comment - Committed the patch to the trunk as svn revision 801492. Thank you very much for this contribution, Eranda! Please confirm that the updated code looks correct on your machine, and then close this issue at your convenience.
        Hide
        Eranda Sooriyabandara added a comment -

        Hi Bryan,
        Thanks for committing the patch. I update the trunk and run
        ColumnDefaultsTest successfully.
        I am closing this issue.

        Show
        Eranda Sooriyabandara added a comment - Hi Bryan, Thanks for committing the patch. I update the trunk and run ColumnDefaultsTest successfully. I am closing this issue.

          People

          • Assignee:
            Eranda Sooriyabandara
            Reporter:
            Eranda Sooriyabandara
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development