Qpid
  1. Qpid
  2. QPID-2492

Changes to brokertest.py to better manage Windows brokers

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: 0.7
    • Component/s: Python Test Suite
    • Labels:
      None

      Description

      I've been developing tests for the persistence/store functionality using python/qpid/brokertest.py and I've made some local changes to get this to work well on Windows. I'm a Python beginner, and there may be better ways to do what I've done. Could someone please review the patch here and let me know any suggestions?

      1. brokertest.diff
        5 kB
        Steve Huston

        Issue Links

          Activity

          Hide
          Alan Conway added a comment -

          I've got 2 concerns with this patch:

          1. I deliberately did not use the subprocess module so this could would run on python 2,3. Is it essential to use subprocess in order for this to work on windows?

          2. Why did you comment out draining of stderr? We need to capture stderr output from failed processes to diagnose test failures.

          Show
          Alan Conway added a comment - I've got 2 concerns with this patch: 1. I deliberately did not use the subprocess module so this could would run on python 2,3. Is it essential to use subprocess in order for this to work on windows? 2. Why did you comment out draining of stderr? We need to capture stderr output from failed processes to diagnose test failures.
          Hide
          Steve Huston added a comment -

          Thanks for the review, Alan.
          1. I'll double check if subprocess is required.
          2. The subprocess init has an option to pipe stderr to stdout without creating a separate stream and reading it.

          Show
          Steve Huston added a comment - Thanks for the review, Alan. 1. I'll double check if subprocess is required. 2. The subprocess init has an option to pipe stderr to stdout without creating a separate stream and reading it.
          Hide
          Steve Huston added a comment -

          The popen2.Popen3 class is not available on Windows. It may be possible to use the popen2.open3 function instead of Popen3. I'll check that out.

          Show
          Steve Huston added a comment - The popen2.Popen3 class is not available on Windows. It may be possible to use the popen2.open3 function instead of Popen3. I'll check that out.
          Hide
          Steve Huston added a comment -

          The main hitch with using popen2 is access to the pid, which is required for the process-killing capability, which is necessary. Popen3 class provides it but Popen3/4 are not available on Windows.

          So, Windows needs to use subprocess instead of popen2. I'll try to develop a patch that uses subprocess only on Windows and popen2 otherwise. How does that sound?

          Show
          Steve Huston added a comment - The main hitch with using popen2 is access to the pid, which is required for the process-killing capability, which is necessary. Popen3 class provides it but Popen3/4 are not available on Windows. So, Windows needs to use subprocess instead of popen2. I'll try to develop a patch that uses subprocess only on Windows and popen2 otherwise. How does that sound?
          Hide
          Steve Huston added a comment -

          This has to use subprocess on Windows; I can move the Popen class to its own file and implement it with subprocess and leave the original popen one parallel to it. Is there a way I can tell the Python version at run time and pick one implementation over the other? Or would you rather this be left as a Windows vs. Linux thing? I'd rather not divide by OS to avoid diverging capabilities that aren't portable up to the test using it.

          Show
          Steve Huston added a comment - This has to use subprocess on Windows; I can move the Popen class to its own file and implement it with subprocess and leave the original popen one parallel to it. Is there a way I can tell the Python version at run time and pick one implementation over the other? Or would you rather this be left as a Windows vs. Linux thing? I'd rather not divide by OS to avoid diverging capabilities that aren't portable up to the test using it.
          Hide
          Alan Conway added a comment -

          I withdraw my objection to subprocess. Its already been used in some other test code, and popen2 is deprecated in current python versions. I think we can skip tests if necessary on old platforms that don't support it, or write a subprocess compatibility wrapper if we really need to. That makes more sense then wrapping subprocess in a popen wrapper.

          Show
          Alan Conway added a comment - I withdraw my objection to subprocess. Its already been used in some other test code, and popen2 is deprecated in current python versions. I think we can skip tests if necessary on old platforms that don't support it, or write a subprocess compatibility wrapper if we really need to. That makes more sense then wrapping subprocess in a popen wrapper.
          Hide
          Alan Conway added a comment -

          One minor comment: the stderr lines that are commented out can simply be removed.

          Show
          Alan Conway added a comment - One minor comment: the stderr lines that are commented out can simply be removed.
          Hide
          Steve Huston added a comment -

          Fixed on trunk r1028156

          Show
          Steve Huston added a comment - Fixed on trunk r1028156

            People

            • Assignee:
              Steve Huston
              Reporter:
              Steve Huston
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development