Derby
  1. Derby
  2. DERBY-3663

Convert store/streamingColumn to JUnit

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.5.1.1
    • Component/s: Test
    • Labels:
      None

      Description

      Convert org.apache.derbyTesting.functionTests.tests.store.streamingColumn to a JUnit testcase.

      1. derby-3663-4.diff
        178 kB
        Suran Jayathilaka
      2. derby-3663-3.diff
        163 kB
        Suran Jayathilaka
      3. derby-3663-2.diff
        79 kB
        Suran Jayathilaka
      4. derby-3663-1.diff
        82 kB
        Suran Jayathilaka

        Issue Links

          Activity

          Hide
          Suran Jayathilaka added a comment -

          Note: This patch (derby-3663-1.diff) is NOT commit-ready.

          Submitted for initial review.

          TO DO:
          Fix 'file access permission denied' errors
          investigate testDerby500 failure

          Show
          Suran Jayathilaka added a comment - Note: This patch (derby-3663-1.diff) is NOT commit-ready. Submitted for initial review. TO DO: Fix 'file access permission denied' errors investigate testDerby500 failure
          Hide
          Kathey Marsden added a comment -

          Hi Suran, I have started going through the test but will be out tomorrow so may not finish my review until Monday. I am marking patch available in the hopes that someone else has some time to look at it as well. It is a big test so more eyes would be better.

          Show
          Kathey Marsden added a comment - Hi Suran, I have started going through the test but will be out tomorrow so may not finish my review until Monday. I am marking patch available in the hopes that someone else has some time to look at it as well. It is a big test so more eyes would be better.
          Hide
          Kathey Marsden added a comment -

          Here are some comments. I will look more on Monday.
          Probably already have these on your list.

          • add test to suite.
          • remove from derbyall and cleanup old test files.

          Comments on StreamingColumnTest

          • It's a failing of the original test, but it would be great to have some javadoc comments for the fixtures.
          • The unused method private static void expectedException1(SQLException sqle) can be removed.

          – basesuite()

          • In basesuite we have
            TestSuite suite = new TestSuite(name);
            suite.addTestSuite(StreamingColumnTest.class);
            Test test = new SupportFilesSetup(suite, new String[] { "functionTests/tests/store/short.data", "functionTests/tests/store/shortbanner", "functionTests/tests/store/derby.banner", "functionTests/tests/store/empty.data"}

            );
            return new CleanDatabaseTestSetup(DatabasePropertyTestSetup
            .setLockTimeouts(suite, 2, 4)) {

          Shouldn't it be test and not suite that gets passed to CleanDatabaseTestSetup?

          – testStream1()

          • We have:
            try {// if trying to insert data > 32700, there will be an // exception ps.executeUpdate(); println("No truncation and hence no error"); }

            catch (SQLException e) {
            if (fileLength[i] > Limits.DB2_LONGVARCHAR_MAXWIDTH) {
            assertSQLState("22001", e); // was getting data longer
            // than maxValueAllowed
            println("expected exception for data > "
            + Limits.DB2_LONGVARCHAR_MAXWIDTH
            + " in length");
            \
            There should be a fail(....) after the ps.excecuteUpdate for the cases where we expect an exception.

          • I know it was part of the original test, but it seems strange that the test imports and uses mport org.apache.derby.iapi.reference.Limits. Typically the test code does not use the engine code. Perhaps we could use constants in the test insteadx.

          --testStream2*()

          • I feel a little funny about the duplicate code in testStream2_1500, testStream2_5000 and testStream2_10000. Could you extract some of the code and have a
            testStream2(long length, String tablename) that you call from these methods?

          – testStream3*()

          • Same comment about duplicate code.
          • Statement sourceStmt = createStatement(); is not needed

          – testStream4()

          • local variable met is not used.
          • you can remove the catch and throw of the exception since the method already throws Exception.
          • one thing that is funny is that the results that are processed don't seem to be checked at all. This is a carryover from the original test, but at least perhaps we could check the columnSize.
          Show
          Kathey Marsden added a comment - Here are some comments. I will look more on Monday. Probably already have these on your list. add test to suite. remove from derbyall and cleanup old test files. Comments on StreamingColumnTest It's a failing of the original test, but it would be great to have some javadoc comments for the fixtures. The unused method private static void expectedException1(SQLException sqle) can be removed. – basesuite() In basesuite we have TestSuite suite = new TestSuite(name); suite.addTestSuite(StreamingColumnTest.class); Test test = new SupportFilesSetup(suite, new String[] { "functionTests/tests/store/short.data", "functionTests/tests/store/shortbanner", "functionTests/tests/store/derby.banner", "functionTests/tests/store/empty.data"} ); return new CleanDatabaseTestSetup(DatabasePropertyTestSetup .setLockTimeouts(suite, 2, 4)) { Shouldn't it be test and not suite that gets passed to CleanDatabaseTestSetup? – testStream1() We have: try {// if trying to insert data > 32700, there will be an // exception ps.executeUpdate(); println("No truncation and hence no error"); } catch (SQLException e) { if (fileLength [i] > Limits.DB2_LONGVARCHAR_MAXWIDTH) { assertSQLState("22001", e); // was getting data longer // than maxValueAllowed println("expected exception for data > " + Limits.DB2_LONGVARCHAR_MAXWIDTH + " in length"); \ There should be a fail(....) after the ps.excecuteUpdate for the cases where we expect an exception. I know it was part of the original test, but it seems strange that the test imports and uses mport org.apache.derby.iapi.reference.Limits. Typically the test code does not use the engine code. Perhaps we could use constants in the test insteadx. --testStream2*() I feel a little funny about the duplicate code in testStream2_1500, testStream2_5000 and testStream2_10000. Could you extract some of the code and have a testStream2(long length, String tablename) that you call from these methods? – testStream3*() Same comment about duplicate code. Statement sourceStmt = createStatement(); is not needed – testStream4() local variable met is not used. you can remove the catch and throw of the exception since the method already throws Exception. one thing that is funny is that the results that are processed don't seem to be checked at all. This is a carryover from the original test, but at least perhaps we could check the columnSize.
          Hide
          Kristian Waagan added a comment -

          Now, this is a big test!

          I had a look, and found two things that should be fixed:
          a) Change the class name in the license header.
          b) Replace all tabs with spaces (4 spaces per tab for indentation).

          Regarding point b, there are tabs both at the beginning of the lines and elsewhere. There are also a few occurrences of trailing whitespace on some lines.

          Maybe I get some time to look more in detail at the tests themselves later, after you have addressed the comments from Kathey.
          Keep up the good work

          Show
          Kristian Waagan added a comment - Now, this is a big test! I had a look, and found two things that should be fixed: a) Change the class name in the license header. b) Replace all tabs with spaces (4 spaces per tab for indentation). Regarding point b, there are tabs both at the beginning of the lines and elsewhere. There are also a few occurrences of trailing whitespace on some lines. Maybe I get some time to look more in detail at the tests themselves later, after you have addressed the comments from Kathey. Keep up the good work
          Hide
          Suran Jayathilaka added a comment -

          Thanks a lot for the comments.

          Attaching derby-3663-2.diff for further review.

              • This patch is NOT commit ready.***

          TO DOs:
          1. Investigate remaining test failures and fix errors
          2. Remove old test and support files and references
          3. Additional javadoc comments
          4. Rectify style and formatting issues
          5. Anything else?

          Show
          Suran Jayathilaka added a comment - Thanks a lot for the comments. Attaching derby-3663-2.diff for further review. This patch is NOT commit ready.*** TO DOs: 1. Investigate remaining test failures and fix errors 2. Remove old test and support files and references 3. Additional javadoc comments 4. Rectify style and formatting issues 5. Anything else?
          Hide
          Kathey Marsden added a comment -

          I looked at the new patch. Thank you for converting this big test. In general I think you need to review the places where you catch exceptions and make sure there is a fail call if the exception is expected and assertSQLState for the expected exceptions. I have a few specific comments below.

          – Where you have code that catches an exception and rethrows it or prints the stack trace, you can eliminate the try catch block and just let the exception be thrown.
          .... e.g.

          } catch (Exception e)

          { throw e; }

          } catch (SQLException sqle)

          { println("UNEXPECTED EXCEPTION - update should have actually gone through"); throw sqle; }

          – You can eliminate the unused local variables for ResultSetMetaData and ResultSet.

          – We could probably use java.util.Arrays.equals() instead of byteArrayEquals

          – insertDataUsingConcat
          we have:
          // for varchars, trailing blank truncation will not throw an exception.
          // Only non-blank characters will cause truncation error
          // for long varchars, any character truncation will throw an exception.
          try

          { stmt.execute(sql); println("No truncation and hence no error."); }

          catch (SQLException e)

          { assertSQLState("22001", e); // truncation error println("expected exception for data > " + maxValueAllowed + " in length"); }

          There is Is no fail call after stmt.execute(sql) for the cases that are expected to fail.
          Similar problem in insertDataUsingCharacterStream, insertDataUsingAsciiStream, testStream1.

          --testDerby500
          Need a fail before catching the expected SQLException.
          and should assertSQLState for the expected exception
          println("DERBY500 #1 Rows updated =" + rowCount);

          } catch (SQLException sqle)

          { println("EXPECTED EXCEPTION - streams cannot be re-used"); // expectedException(sqle); rollback(); }
          Show
          Kathey Marsden added a comment - I looked at the new patch. Thank you for converting this big test. In general I think you need to review the places where you catch exceptions and make sure there is a fail call if the exception is expected and assertSQLState for the expected exceptions. I have a few specific comments below. – Where you have code that catches an exception and rethrows it or prints the stack trace, you can eliminate the try catch block and just let the exception be thrown. .... e.g. } catch (Exception e) { throw e; } } catch (SQLException sqle) { println("UNEXPECTED EXCEPTION - update should have actually gone through"); throw sqle; } – You can eliminate the unused local variables for ResultSetMetaData and ResultSet. – We could probably use java.util.Arrays.equals() instead of byteArrayEquals – insertDataUsingConcat we have: // for varchars, trailing blank truncation will not throw an exception. // Only non-blank characters will cause truncation error // for long varchars, any character truncation will throw an exception. try { stmt.execute(sql); println("No truncation and hence no error."); } catch (SQLException e) { assertSQLState("22001", e); // truncation error println("expected exception for data > " + maxValueAllowed + " in length"); } There is Is no fail call after stmt.execute(sql) for the cases that are expected to fail. Similar problem in insertDataUsingCharacterStream, insertDataUsingAsciiStream, testStream1. --testDerby500 Need a fail before catching the expected SQLException. and should assertSQLState for the expected exception println("DERBY500 #1 Rows updated =" + rowCount); } catch (SQLException sqle) { println("EXPECTED EXCEPTION - streams cannot be re-used"); // expectedException(sqle); rollback(); }
          Hide
          Suran Jayathilaka added a comment -

          All fixtures pass, although testStream11 depends on the patch for derby-3705 being applied.

          I think I have addressed the issues pointed out in the previous submissions. Also removed the old files for streamingColumn.

          Please review. derby-3663-3.diff

          Show
          Suran Jayathilaka added a comment - All fixtures pass, although testStream11 depends on the patch for derby-3705 being applied. I think I have addressed the issues pointed out in the previous submissions. Also removed the old files for streamingColumn. Please review. derby-3663-3.diff
          Hide
          Suran Jayathilaka added a comment -

          Fixture testStream11 depends on the patch for derby-3705 to pass.

          Show
          Suran Jayathilaka added a comment - Fixture testStream11 depends on the patch for derby-3705 to pass.
          Hide
          Kathey Marsden added a comment -

          Thanks Suran for converting this huge test. I think it is getting very close. Just a couple of nits. There is some code like this:
          try

          { psu2.setBinaryStream(1, new ByteArrayInputStream(buf), len); psu2.setAsciiStream(2, new ByteArrayInputStream(buf), len); psu2.setInt(3, 0); rowCount += psu2.executeUpdate(); println("DERBY500 #2 Rows updated =" + rowCount); }

          catch (SQLException sqle)

          { println("UNEXPECTED EXCEPTION - update should have actually gone through"); throw sqle; }

          Where you catch the exception, just to throw it again. I think it would be better to just remove the try/catch blocks and let the exception be thrown.

          The code should be reformatted to have space indentation instead of tabs.

          There were a couple of properties in the streamingColumn_derby.properties which should probably be set for the test. you can use SystemPropertyTestSetup to set these.

          derby.storage.sortBufferMax=5
          derby.debug.true=testSort

          I noticed these jvmflags in the app.properties
          jvmflags=-ms32M^-mx128M

          Looking back into the archives this was added a long time ago to increase the memory available not to restrict memory for the test, so I think it is ok to just run with the default memory configuration with modern jvms.

          Show
          Kathey Marsden added a comment - Thanks Suran for converting this huge test. I think it is getting very close. Just a couple of nits. There is some code like this: try { psu2.setBinaryStream(1, new ByteArrayInputStream(buf), len); psu2.setAsciiStream(2, new ByteArrayInputStream(buf), len); psu2.setInt(3, 0); rowCount += psu2.executeUpdate(); println("DERBY500 #2 Rows updated =" + rowCount); } catch (SQLException sqle) { println("UNEXPECTED EXCEPTION - update should have actually gone through"); throw sqle; } Where you catch the exception, just to throw it again. I think it would be better to just remove the try/catch blocks and let the exception be thrown. The code should be reformatted to have space indentation instead of tabs. There were a couple of properties in the streamingColumn_derby.properties which should probably be set for the test. you can use SystemPropertyTestSetup to set these. derby.storage.sortBufferMax=5 derby.debug.true=testSort I noticed these jvmflags in the app.properties jvmflags=-ms32M^-mx128M Looking back into the archives this was added a long time ago to increase the memory available not to restrict memory for the test, so I think it is ok to just run with the default memory configuration with modern jvms.
          Hide
          Suran Jayathilaka added a comment -

          derby-3663-4.diff has the following improvements.

          1. Removed unnecessary try/catch blocks.
          2. Replaced tabs with spaces.
          3. Set properties with SystemPropertyTestSetup
          derby.storage.sortBufferMax=5
          derby.debug.true=testSort

          Show
          Suran Jayathilaka added a comment - derby-3663-4.diff has the following improvements. 1. Removed unnecessary try/catch blocks. 2. Replaced tabs with spaces. 3. Set properties with SystemPropertyTestSetup derby.storage.sortBufferMax=5 derby.debug.true=testSort
          Hide
          Kathey Marsden added a comment - - edited

          Thanks Suran,

          I'll run tests and commit this and DERBY-3705 if all goes well. One important thing I noticed is that the test was not added to the store _Suite. I will add it.

          Kathey

          Show
          Kathey Marsden added a comment - - edited Thanks Suran, I'll run tests and commit this and DERBY-3705 if all goes well. One important thing I noticed is that the test was not added to the store _Suite. I will add it. Kathey
          Hide
          Suran Jayathilaka added a comment -

          Test converted and committed. Old test removed.

          Show
          Suran Jayathilaka added a comment - Test converted and committed. Old test removed.

            People

            • Assignee:
              Suran Jayathilaka
              Reporter:
              Suran Jayathilaka
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development