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.

[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.

I've been able to adapt the testcase since the previous merge proposal 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.

[Robert Collins (lifeless)]

The change to setUp is wrong - has to be fixed. This can be done as its merged.

review: Approve

[Gordon Tyler (doxxx)]

Sorry, still not used to Python's super call. Fixed and pushed.

[Robert Collins (lifeless)]

I'll shepard this through. At this point we still need a second review.

[Vincent Ladeuil (vila)]

Really tweaks:

+ class _TestBzrServeBase(TestCaseWithTransport):

We never use leading '_' for test classes, even if that may sound reasonable here.
I.e. I understand the intent, but let's not surprise future readers by starting
a new coding rule without good reason.

+ try:
+ self.run_bzr(['serve'] + list(serve_args))
+ except KeyboardInterrupt as e:
+ return e.args

1) I doubt python2.4 will accept 'as'.

2) While it's understandable in this patch context that e.args is really stdout, stderr, it will quickly become obscure. A comment will address that.

3) If the exception is raised, None will be returned instead of the (stdout, stderr) tuple. Again, today, the only that cares will pass, but if someone try to reuse the run_bzr_serve_then_func method, he will have a hard time. Please return (None, None) at least.

review: Needs Fixing

[Gordon Tyler (doxxx)]

Thanks for the suggestions. You're quite right about the stdout, stderr tuple. I've pushed an update which should fix both the clarity and the consistency of the return arguments, as well as the class name and Python 2.4 compatibility.

[Vincent Ladeuil (vila)]

Excellent, thanks, I'll merge (just checked that all tests pass for python 2.[456]).

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2009-11-24 08:28:41 +0000
+++ bzrlib/tests/__init__.py	2009-11-25 13:25:23 +0000
@@ -1836,10 +1836,22 @@
             os.chdir(working_dir)
 
         try:
-            result = self.apply_redirected(ui.ui_factory.stdin,
-                stdout, stderr,
-                bzrlib.commands.run_bzr_catch_user_errors,
-                args)
+            try:
+                result = self.apply_redirected(ui.ui_factory.stdin,
+                    stdout, stderr,
+                    bzrlib.commands.run_bzr_catch_user_errors,
+                    args)
+            except KeyboardInterrupt:
+                # Reraise KeyboardInterrupt with contents of redirected stdout and 
+                # stderr as arguments, for tests which are interested in stdout and
+                # stderr and are expecting the exception.
+                out = stdout.getvalue()
+                err = stderr.getvalue()
+                if out:
+                    self.log('output:\n%r', out)
+                if err:
+                    self.log('errors:\n%r', err)
+                raise KeyboardInterrupt(out, err)
         finally:
             logger.removeHandler(handler)
             ui.ui_factory = old_ui_factory

=== 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-25 13:25:23 +0000
@@ -45,8 +45,49 @@
 from bzrlib.trace import mutter
 from bzrlib.transport import get_transport, remote
 
-
-class TestBzrServe(TestCaseWithTransport):
+class TestBzrServeBase(TestCaseWithTransport):
+
+    def run_bzr_serve_then_func(self, serve_args, retcode=0, func=None,
+                                *func_args, **func_kwargs):
+        """Run 'bzr serve', and run the given func in a thread once the server
+        has started.
+        
+        When 'func' terminates, the server will be terminated too.
+        
+        Returns stdout and stderr.
+        """
+        # install hook
+        def on_server_start(backing_urls, tcp_server):
+            t = threading.Thread(
+                target=on_server_start_thread, args=(tcp_server,))
+            t.start()
+        def on_server_start_thread(tcp_server):
+            try:
+                # Run func if set
+                self.tcp_server = tcp_server
+                if not func is None:
+                    try:
+                        func(*func_args, **func_kwargs)
+                    except Exception, e:
+                        # Log errors to make some test failures a little less
+                        # mysterious.
+                        mutter('func broke: %r', e)
+            finally:
+                # Then stop the server
+                mutter('interrupting...')
+                thread.interrupt_main()
+        SmartTCPServer.hooks.install_named_hook(
+            'server_started_ex', on_server_start,
+            'run_bzr_serve_then_func hook')
+        # start a TCP server
+        try:
+            out, err = self.run_bzr(['serve'] + list(serve_args))
+        except KeyboardInterrupt, e:
+            out, err = e.args
+        return out, err
+
+
+class TestBzrServe(TestBzrServeBase):
 
     def setUp(self):
         super(TestBzrServe, self).setUp()
@@ -119,6 +160,13 @@
         url = 'bzr://localhost:%d/' % port
         self.permit_url(url)
         return process, url
+    
+    def test_bzr_serve_quiet(self):
+        self.make_branch('.')
+        args = ['--port', 'localhost:0', '--quiet']
+        out, err = self.run_bzr_serve_then_func(args, retcode=3)
+        self.assertEqual('', out)
+        self.assertEqual('', err)
 
     def test_bzr_serve_inet_readonly(self):
         """bzr server should provide a read only filesystem by default."""
@@ -168,7 +216,7 @@
         self.assertServerFinishesCleanly(process)
 
 
-class TestCmdServeChrooting(TestCaseWithTransport):
+class TestCmdServeChrooting(TestBzrServeBase):
 
     def test_serve_tcp(self):
         """'bzr serve' wraps the given --directory in a ChrootServer.
@@ -183,47 +231,12 @@
             ['--port', '127.0.0.1:0',
              '--directory', t.local_abspath('server-root'),
              '--allow-writes'],
-            self.when_server_started)
+            func=self.when_server_started)
         # The when_server_started method issued a find_repositoryV3 that should
         # fail with 'norepository' because there are no repositories inside the
         # --directory.
         self.assertEqual(('norepository',), self.client_resp)
 
-    def run_bzr_serve_then_func(self, serve_args, func, *func_args,
-            **func_kwargs):
-        """Run 'bzr serve', and run the given func in a thread once the server
-        has started.
-        
-        When 'func' terminates, the server will be terminated too.
-        """
-        # install hook
-        def on_server_start(backing_urls, tcp_server):
-            t = threading.Thread(
-                target=on_server_start_thread, args=(tcp_server,))
-            t.start()
-        def on_server_start_thread(tcp_server):
-            try:
-                # Run func
-                self.tcp_server = tcp_server
-                try:
-                    func(*func_args, **func_kwargs)
-                except Exception, e:
-                    # Log errors to make some test failures a little less
-                    # mysterious.
-                    mutter('func broke: %r', e)
-            finally:
-                # Then stop the server
-                mutter('interrupting...')
-                thread.interrupt_main()
-        SmartTCPServer.hooks.install_named_hook(
-            'server_started_ex', on_server_start,
-            'run_bzr_serve_then_func hook')
-        # start a TCP server
-        try:
-            self.run_bzr(['serve'] + list(serve_args))
-        except KeyboardInterrupt:
-            pass
-
     def when_server_started(self):
         # Connect to the TCP server and issue some requests and see what comes
         # back.