Derby
  1. Derby
  2. DERBY-4260

Make derbynet/NetworkServerControlClientCommandTest run regardless of the locale

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.6.1.0
    • Fix Version/s: 10.8.2.2, 10.9.1.0
    • Component/s: Test
    • Labels:
    • Urgency:
      Low
    • Issue & fix info:
      Patch Available

      Description

      The NetworkServerControlClientCommandTest has in it a check that will only run the test if the locale is set to 'en'.

      It is open to suggestion the how to achieve this and whether it should be done. Since the test is a pretty generic one, I think it should be ran regardless of the locale.

      Knut suggested we'd force the locale to 'en' on the setUp() and back to its previous value on the tearDown(). On a first analysis this seems like the ideal method to go about it. Just removing the check for the locale altogether might not be wise because of the calls to assertFailedPing() that involve some hardcoded strings.

      1. 4260-1.patch
        8 kB
        Houx Zhang

        Issue Links

          Activity

          Hide
          Myrna van Lunteren added a comment -

          oops, got the revision for wrong in my comment; backported to 10.8 branch with revision 1157923. The automatic revision reflect tool's got it right.

          Show
          Myrna van Lunteren added a comment - oops, got the revision for wrong in my comment; backported to 10.8 branch with revision 1157923. The automatic revision reflect tool's got it right.
          Hide
          Myrna van Lunteren added a comment -

          backported to 10.8 branch with revision 1156856.

          Show
          Myrna van Lunteren added a comment - backported to 10.8 branch with revision 1156856.
          Hide
          Myrna van Lunteren added a comment -

          reopen to document backport to 10.8

          Show
          Myrna van Lunteren added a comment - reopen to document backport to 10.8
          Hide
          Houx Zhang added a comment -

          OK, I've got it , thanks, Bryan. It's reasonable and helpful!

          Show
          Houx Zhang added a comment - OK, I've got it , thanks, Bryan. It's reasonable and helpful!
          Hide
          Bryan Pendleton added a comment -

          Typically, the Derby community prefers to work on multiple smaller patches, rather than a single larger patch.
          Each patch should address a single problem. We agree with you that this makes the SVN history easier
          to comprehend.

          So, in the cases that you identify, we'd prefer to address the test case refactoring separately
          from the non-English locale issues, as two patches rather than one.

          Similarly, we try to avoid doing things like removing unneeded "import" statements or changing whitespace
          or formatting/indentation of the code as part of a patch. The Derby code unfortunately has a significant
          number of whitespace issues, as it is a old and mature codebase that has been worked on by many
          programmers over the years, so we just accept those problems and try to avoid those changes in our patches.

          Meanwhile, Tiago, if you get a chance, can you confirm that this test now works properly for you in
          a non-English locale? Then I think you could close this job?

          Show
          Bryan Pendleton added a comment - Typically, the Derby community prefers to work on multiple smaller patches, rather than a single larger patch. Each patch should address a single problem. We agree with you that this makes the SVN history easier to comprehend. So, in the cases that you identify, we'd prefer to address the test case refactoring separately from the non-English locale issues, as two patches rather than one. Similarly, we try to avoid doing things like removing unneeded "import" statements or changing whitespace or formatting/indentation of the code as part of a patch. The Derby code unfortunately has a significant number of whitespace issues, as it is a old and mature codebase that has been worked on by many programmers over the years, so we just accept those problems and try to avoid those changes in our patches. Meanwhile, Tiago, if you get a chance, can you confirm that this test now works properly for you in a non-English locale? Then I think you could close this job?
          Hide
          Houx Zhang added a comment -

          Hehe, thanks for your encourage, Bryan!

          As to this issue, I still have a question.

          I have done big refatoring on this test class, then changed it to support non-English locale. Is it better to provide two patches, one for refactoring and one for non-Endlish locale supporting, please? By this way, the change will be more clear to read in SVN repository.

          As I will do many similiar testing work, I think the answer is useful.

          Thanks!

          Show
          Houx Zhang added a comment - Hehe, thanks for your encourage, Bryan! As to this issue, I still have a question. I have done big refatoring on this test class, then changed it to support non-English locale. Is it better to provide two patches, one for refactoring and one for non-Endlish locale supporting, please? By this way, the change will be more clear to read in SVN repository. As I will do many similiar testing work, I think the answer is useful. Thanks!
          Hide
          Bryan Pendleton added a comment -

          Yes, I think this patch is ready to commit.

          Committed to the trunk as revision 1095247.

          Thanks for the contribution to Derby!

          Show
          Bryan Pendleton added a comment - Yes, I think this patch is ready to commit. Committed to the trunk as revision 1095247. Thanks for the contribution to Derby!
          Hide
          Houx Zhang added a comment -

          Thanks for your checking, Bryan. And could you commit this patch, please?

          Show
          Houx Zhang added a comment - Thanks for your checking, Bryan. And could you commit this patch, please?
          Hide
          Bryan Pendleton added a comment -

          The patch builds and the tests pass in my environment:

          bpendleton@bpendleton-laptop:~/tests$ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest
          .....
          Time: 2.004

          OK (5 tests)

          Show
          Bryan Pendleton added a comment - The patch builds and the tests pass in my environment: bpendleton@bpendleton-laptop:~/tests$ java junit.textui.TestRunner org.apache.derbyTesting.functionTests.tests.derbynet.NetworkServerControlClientCommandTest ..... Time: 2.004 OK (5 tests)
          Hide
          Houx Zhang added a comment -

          Hi, Bryan. As we have discussed in DERBY-5155, it should be dealt with individually and it's also part of DERBY-5155.

          Please check the patch for the separate modification on locale, please. Thanks!

          Show
          Houx Zhang added a comment - Hi, Bryan. As we have discussed in DERBY-5155 , it should be dealt with individually and it's also part of DERBY-5155 . Please check the patch for the separate modification on locale, please. Thanks!
          Hide
          Houx Zhang added a comment -

          Hi, Bryan. IMHO, derbynet/NetworkServerControlClientCommandTest can not been applied a general resolution on. As in the testing, some new processes are created to run the java command, and a new locale must be set in the new processes.

          Show
          Houx Zhang added a comment - Hi, Bryan. IMHO, derbynet/NetworkServerControlClientCommandTest can not been applied a general resolution on. As in the testing, some new processes are created to run the java command, and a new locale must be set in the new processes.
          Hide
          Bryan Pendleton added a comment -

          Is it necessary to split the testPing test up as part of this? It seems like it would be
          a simpler patch if we just addressed the locale issue for now.

          Show
          Bryan Pendleton added a comment - Is it necessary to split the testPing test up as part of this? It seems like it would be a simpler patch if we just addressed the locale issue for now.
          Hide
          Houx Zhang added a comment -

          Derby-4260 relates to Derby-5155, but isn't part of them, as in Derby-4260, English locale is set to a separate java command rather than to a test case.

          Show
          Houx Zhang added a comment - Derby-4260 relates to Derby-5155, but isn't part of them, as in Derby-4260, English locale is set to a separate java command rather than to a test case.
          Hide
          Houx Zhang added a comment -

          In my testing on NetworkServerControlClientCommandTest.testPing(), it's useless to use LocaleTestSetup. The assertExecJavaCmdAsExpected() runs in a separate process.

          In my patch, "-Dderby.ui.locale=en_US" has been included in command args, and the testcase passed in Chinese Platform, while failed when reverted into default state.

          Furthermore, as the old testcase tests five situations in just one function testPing(), I have splitted it into five funcstions to make it more readable and maintainable.

          Wish for your comments, thanks!

          Show
          Houx Zhang added a comment - In my testing on NetworkServerControlClientCommandTest.testPing(), it's useless to use LocaleTestSetup. The assertExecJavaCmdAsExpected() runs in a separate process. In my patch, "-Dderby.ui.locale=en_US" has been included in command args, and the testcase passed in Chinese Platform, while failed when reverted into default state. Furthermore, as the old testcase tests five situations in just one function testPing(), I have splitted it into five funcstions to make it more readable and maintainable. Wish for your comments, thanks!
          Hide
          Knut Anders Hatlen added a comment -

          I don't think setting the locale in setUp() is going to work, at least not if we set it with Locale.setDefault(Locale.US) as I suggested on derby-dev, since the ping command is executed as a separate process. Perhaps we could make assertFailedPing() pass -Dderby.ui.locale=en_US to the forked process so that it always prints English error messages.

          Show
          Knut Anders Hatlen added a comment - I don't think setting the locale in setUp() is going to work, at least not if we set it with Locale.setDefault(Locale.US) as I suggested on derby-dev, since the ping command is executed as a separate process. Perhaps we could make assertFailedPing() pass -Dderby.ui.locale=en_US to the forked process so that it always prints English error messages.

            People

            • Assignee:
              Houx Zhang
              Reporter:
              Tiago R. Espinha
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development