Merge lp:~doxxx/bzr/quiet-serve into lp:bzr

[Gordon Tyler]

[Gordon Tyler (doxxx)]

Adds a testcase for bug #252834. The actual bug seems to have been fixed some time ago, but I figured a testcase couldn't hurt.

[Robert Collins (lifeless)]

Thanks for submitting this. We really prefer to test things close to the
thing under question.

In this particular case, why is a subprocess needed?

 review needsfixing

review: Needs Fixing

[Gordon Tyler (doxxx)]

I'm not sure, I just followed the example of the other tests in that class. Perhaps it's because if you started it in-process there would be no way to interrupt it and it would hang the tests?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gordon Tyler wrote:
> I'm not sure, I just followed the example of the other tests in that class. Perhaps it's because if you started it in-process there would be no way to interrupt it and it would hang the tests?

Also note that if you are using a subprocess that you are going to kill,
then you need to skip the test on windows so it doesn't hang. (At least
until a patch comes along that can use TerminateProcess :)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksGsZIACgkQJdeBCYSNAAMGlgCdF0kYZpWNZ3DN1H/q6Xp4ZiSS
f9sAoLFJNgWPdpKRaZuCHQoex7al6UMH
=kvJ+
-----END PGP SIGNATURE-----

[Gordon Tyler (doxxx)]

> Also note that if you are using a subprocess that you are going to kill,
> then you need to skip the test on windows so it doesn't hang. (At least
> until a patch comes along that can use TerminateProcess :)

Note that it says "skip_if_plan_to_signal=True", which means skip on win32 until my other patch lands. ;)

[Robert Collins (lifeless)]

On Fri, 2009-11-20 at 13:02 +0000, Gordon Tyler wrote:
> I'm not sure, I just followed the example of the other tests in that
> class. Perhaps it's because if you started it in-process there would
> be no way to interrupt it and it would hang the tests?

So, you cargo culted, which is fine :). However this test doesn't need a
subprocess. The other ones did for a few reasons, but I don't see why
this one would.

subprocess tests are very expensive, its an exceptional thing to do to
add one.

-Rob

[Gordon Tyler (doxxx)]

> So, you cargo culted, which is fine :). However this test doesn't need a
> subprocess. The other ones did for a few reasons, but I don't see why
> this one would.

Can you explain to me why this test doesn't require one? I cannot see a difference between this and the other tests. If the test invokes 'bzr serve' in-process it will block and the tests will hang.

[Robert Collins (lifeless)]

A couple of ways to prevent that
 - use a server startup hook to trigger shutdown.
 - use a thread

-Rob

[Gordon Tyler (doxxx)]

> A couple of ways to prevent that
> - use a server startup hook to trigger shutdown.
> - use a thread

Well, silly me, I didn't see the utility functions in TestCmdServeChrooting which do all that. I'll adapt the new testcase to use that.

On a related note, why do the other tests in TestBzrServe launch a subprocess and not do the same thing as TestCmdServeChrooting?

[Gordon Tyler (doxxx)]

I've been able to adapt the testcase to run the command in-process. I had to get inventive with the KeyboardInterrupt handling so that the testcase could still get the stdout and stderr.

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2009-11-16 21:36:52 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2009-11-20 06:35:24 +0000
@@ -24,6 +24,7 @@
 import sys
 import thread
 import threading
+import time
 
 from bzrlib import (
     builtins,
@@ -119,6 +120,16 @@
         url = 'bzr://localhost:%d/' % port
         self.permit_url(url)
         return process, url
+    
+    def test_bzr_serve_quiet(self):
+        self.make_branch('.')
+        args = ['serve', '--port', 'localhost:0', '--quiet']
+        process = self.start_bzr_subprocess(args, skip_if_plan_to_signal=True)
+        time.sleep(1) # wait for subprocess to (possibly) write to stdout/err
+        out, err = self.finish_bzr_subprocess(process, retcode=None,
+                                              send_signal=signal.SIGTERM)
+        self.assertEqual('', out)
+        self.assertEqual('', err)
 
     def test_bzr_serve_inet_readonly(self):
         """bzr server should provide a read only filesystem by default."""