Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.9.1.0
    • Component/s: Test
    • Labels:
    1. 5311-1.patch
      7 kB
      Houx Zhang
    2. 5311-1.stat
      0.5 kB
      Houx Zhang
    3. 5311-followup.txt
      2 kB
      Bryan Pendleton
    4. sysprop.diff
      0.7 kB
      Knut Anders Hatlen
    5. noSecMgr.txt
      2 kB
      Bryan Pendleton
    6. resetWidth.txt
      2 kB
      Bryan Pendleton
    7. 5311-2.patch
      6 kB
      Houx Zhang
    8. 5311-2.stat
      0.4 kB
      Houx Zhang

      Activity

      Hide
      Houx Zhang added a comment -

      Thanks.

      Show
      Houx Zhang added a comment - Thanks.
      Hide
      Bryan Pendleton added a comment -

      Marking as resolved, since no additional problems appear to have arisen.

      Show
      Bryan Pendleton added a comment - Marking as resolved, since no additional problems appear to have arisen.
      Hide
      Bryan Pendleton added a comment -

      I think we could resolve this issue now, does that sound good?

      Show
      Bryan Pendleton added a comment - I think we could resolve this issue now, does that sound good?
      Hide
      Bryan Pendleton added a comment -

      The new patch looks great to me, and the tools suite runs fine on my configuration with the new patch.

      Committed to the trunk as revision 1143416.

      I haven't marked this issue as resolved yet, because I'd like to see that we are getting clean runs
      of the complete suites in the master test configurations, first.

      Show
      Bryan Pendleton added a comment - The new patch looks great to me, and the tools suite runs fine on my configuration with the new patch. Committed to the trunk as revision 1143416. I haven't marked this issue as resolved yet, because I'd like to see that we are getting clean runs of the complete suites in the master test configurations, first.
      Hide
      Houx Zhang added a comment -

      Hi, thanks for your wonderful work, Bryan and Knut.

      I also prefer to add new tests by modifying ToolScripts.java, and hope it does work for ij*.sqls.

      In 5311-2.patch, ij4.sql is added into ToolScripts.java, and maximumdisplaywidth reseting is adopted.

      Please check the new patch, thanks!

      Show
      Houx Zhang added a comment - Hi, thanks for your wonderful work, Bryan and Knut. I also prefer to add new tests by modifying ToolScripts.java, and hope it does work for ij*.sqls. In 5311-2.patch, ij4.sql is added into ToolScripts.java, and maximumdisplaywidth reseting is adopted. Please check the new patch, thanks!
      Hide
      Knut Anders Hatlen added a comment -

      I added ij4Test to i18n._Suite before the other tests in the suite, and then I could reproduce the failures by running only that suite. resetWidth.txt fixed the failures, so +1 to that patch. (The maximumdisplaywidth command affecting all ij sessions in the JVM might perhaps be considered a bug, but fixing that is outside the scope of this JIRA.)

      The noSecMgr.txt patch also looks like an improvement. Since there's no special setup in ij4Test after those changes, I don't think there's anything stopping us from adding it to ToolScripts and removing the ij4Test class. Hopefully the same could be done with the other ij*.sql tests.

      Show
      Knut Anders Hatlen added a comment - I added ij4Test to i18n._Suite before the other tests in the suite, and then I could reproduce the failures by running only that suite. resetWidth.txt fixed the failures, so +1 to that patch. (The maximumdisplaywidth command affecting all ij sessions in the JVM might perhaps be considered a bug, but fixing that is outside the scope of this JIRA.) The noSecMgr.txt patch also looks like an improvement. Since there's no special setup in ij4Test after those changes, I don't think there's anything stopping us from adding it to ToolScripts and removing the ij4Test class. Hopefully the same could be done with the other ij*.sql tests.
      Hide
      Bryan Pendleton added a comment -

      Thanks for tracking down the security manager issue with ij's exit command! I verified that, with the latest trunk, I no longer need the security manager test setup, and have attached the resulting patch to ij4Test.java as noSecMgr.txt.

      Regarding the maximumdisplaywidth issue, it seems certainly cleaner to reset it at the end of the test. I attached a simple 'resetWidth.txt' patch which resets the max width to 128 at the end of the test. I'm not sure I have the appropriate test setup to verify whether that fixes the problem seen in the full test suite or not.

      Show
      Bryan Pendleton added a comment - Thanks for tracking down the security manager issue with ij's exit command! I verified that, with the latest trunk, I no longer need the security manager test setup, and have attached the resulting patch to ij4Test.java as noSecMgr.txt. Regarding the maximumdisplaywidth issue, it seems certainly cleaner to reset it at the end of the test. I attached a simple 'resetWidth.txt' patch which resets the max width to 128 at the end of the test. I'm not sure I have the appropriate test setup to verify whether that fixes the problem seen in the full test suite or not.
      Hide
      Knut Anders Hatlen added a comment -

      The tinderbox had three failures in the i18n tests after the test was converted:

      http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/1143066-org.apache.derbyTesting.functionTests.suites.All_diff.txt

      Maybe the maximumdisplaywidth setting must be reset before we continue with the other tests?

      Show
      Knut Anders Hatlen added a comment - The tinderbox had three failures in the i18n tests after the test was converted: http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/1143066-org.apache.derbyTesting.functionTests.suites.All_diff.txt Maybe the maximumdisplaywidth setting must be reset before we continue with the other tests?
      Hide
      Knut Anders Hatlen added a comment -

      The failure with security manager enabled looks like a bug. ij's exit command attempts to read the system property ij.expect, but it doesn't do so in a privileged block. The attached patch makes it use a helper method that does the required security manager magic. Both tools._Suite and derbytools ran cleanly with the patch, so I took the chance and committed it (revision 1143093).

      Show
      Knut Anders Hatlen added a comment - The failure with security manager enabled looks like a bug. ij's exit command attempts to read the system property ij.expect, but it doesn't do so in a privileged block. The attached patch makes it use a helper method that does the required security manager magic. Both tools._Suite and derbytools ran cleanly with the patch, so I took the chance and committed it (revision 1143093).
      Hide
      Bryan Pendleton added a comment -

      If I remove the noSecurityManager() call, the test fails with

      ij4(org.apache.derbyTesting.functionTests.tests.tools.ij4Test)junit.framework.AssertionFailedError: More output from test than expected

      However, if I remove the system properties and the JSR 169
      code, the test still passes as expected. I've attached the
      diff as 5311-followup.txt.

      I like the idea of using ToolsScripts.java directly; Houx,
      what do you think about that approach?

      Show
      Bryan Pendleton added a comment - If I remove the noSecurityManager() call, the test fails with ij4(org.apache.derbyTesting.functionTests.tests.tools.ij4Test)junit.framework.AssertionFailedError: More output from test than expected However, if I remove the system properties and the JSR 169 code, the test still passes as expected. I've attached the diff as 5311-followup.txt. I like the idea of using ToolsScripts.java directly; Houx, what do you think about that approach?
      Hide
      Bryan Pendleton added a comment -

      I committed the initial patch prior to seeing the comments by Knut Anders. Trunk revision 1143062.

      I haven't marked the issue resolved yet, since I think we'd like to consider the feedback some more first.

      Show
      Bryan Pendleton added a comment - I committed the initial patch prior to seeing the comments by Knut Anders. Trunk revision 1143062. I haven't marked the issue resolved yet, since I think we'd like to consider the feedback some more first.
      Hide
      Knut Anders Hatlen added a comment -

      The patch looks fine to me too. By the way, why was a SecurityManagerSetup needed in this test?

      I don't know, but it doesn't look like the original test was meant to test ij.showNoConnectionsAtStart and ij.showNoCountForSelect specifically. Perhaps we could just skip setting those properties? Then we could add ij4 directly to one of the arrays in ToolScripts and remove the ij4Test class. Might be a useful simplification while we're in there.

      I don't see anything in the test that won't run on Java ME (except the original connect command, which was removed by the patch), so I think it would be safe to remove the check for JDBC.vmSupportsJSR169() and enable the test on Java ME now.

      Show
      Knut Anders Hatlen added a comment - The patch looks fine to me too. By the way, why was a SecurityManagerSetup needed in this test? I don't know, but it doesn't look like the original test was meant to test ij.showNoConnectionsAtStart and ij.showNoCountForSelect specifically. Perhaps we could just skip setting those properties? Then we could add ij4 directly to one of the arrays in ToolScripts and remove the ij4Test class. Might be a useful simplification while we're in there. I don't see anything in the test that won't run on Java ME (except the original connect command, which was removed by the patch), so I think it would be safe to remove the check for JDBC.vmSupportsJSR169() and enable the test on Java ME now.
      Hide
      Bryan Pendleton added a comment -

      The patch looks simple and clean to me. In my environment, both tests.tools._Suite and derbytools
      run successfully with the patch applied, with ij4 moved successfully to the former from
      the latter. I am intending to commit this patch.

      Show
      Bryan Pendleton added a comment - The patch looks simple and clean to me. In my environment, both tests.tools._Suite and derbytools run successfully with the patch applied, with ij4 moved successfully to the former from the latter. I am intending to commit this patch.
      Hide
      Houx Zhang added a comment -

      I have removed

      'connect 'jdbc:derby:wombat;create=true';'

      in ij4.sql. As for a subclass of ScriptTestCase, a default database(wombat) will be connected, so maybe this line is redundant.

      Wish for your opinion.

      Show
      Houx Zhang added a comment - I have removed 'connect 'jdbc:derby:wombat;create=true';' in ij4.sql. As for a subclass of ScriptTestCase, a default database(wombat) will be connected, so maybe this line is redundant. Wish for your opinion.

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development