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

Proposed by Gordon Tyler on 2009-11-21
Status: Merged
Merged at revision: not available
Proposed branch: lp:~doxxx/bzr/quiet-serve
Merge into: lp:bzr
Diff against target: 158 lines (+68/-43)
2 files modified
bzrlib/tests/__init__.py (+16/-4)
bzrlib/tests/blackbox/test_serve.py (+52/-39)
To merge this branch: bzr merge lp:~doxxx/bzr/quiet-serve
Reviewer Review Type Date Requested Status
Vincent Ladeuil 2009-11-23 Needs Fixing on 2009-11-25
Robert Collins (community) 2009-11-21 Approve on 2009-11-23
Review via email: mp+15112@code.launchpad.net

This proposal supersedes a proposal from 2009-11-20.

To post a comment you must log in.
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

-----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) wrote : Posted in a previous version of this proposal

> 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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

> 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) wrote : Posted in a previous version of this proposal

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

-Rob

Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal

> 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) wrote : Posted in a previous version of this proposal

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) wrote :

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.

lp:~doxxx/bzr/quiet-serve updated on 2009-11-21
4819. By Gordon Tyler on 2009-11-21

Removed unnecessary import.

4820. By Gordon Tyler on 2009-11-21

Fixed try/except/finally to try/try/except/finally for Python 2.4 compatibility.

Robert Collins (lifeless) wrote :

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

review: Approve
lp:~doxxx/bzr/quiet-serve updated on 2009-11-23
4821. By Gordon Tyler on 2009-11-23

Fixed super call in TestBzrServe.

Gordon Tyler (doxxx) wrote :

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

Robert Collins (lifeless) wrote :

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

Vincent Ladeuil (vila) wrote :

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) wrote :

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.

lp:~doxxx/bzr/quiet-serve updated on 2009-11-25
4822. By Gordon Tyler on 2009-11-25

Made a few changes at the suggestion of vila.

- Fixed TestBzrServe class name.
- Fixed try/except syntax for python 2.4 compatibility.
- Fixed run_bzr_serve_then_func to always return stdout and stderr, even if bzr serve does not raise KeyboardInterrupt.

Vincent Ladeuil (vila) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/__init__.py'
2--- bzrlib/tests/__init__.py 2009-11-24 08:28:41 +0000
3+++ bzrlib/tests/__init__.py 2009-11-25 13:25:23 +0000
4@@ -1836,10 +1836,22 @@
5 os.chdir(working_dir)
6
7 try:
8- result = self.apply_redirected(ui.ui_factory.stdin,
9- stdout, stderr,
10- bzrlib.commands.run_bzr_catch_user_errors,
11- args)
12+ try:
13+ result = self.apply_redirected(ui.ui_factory.stdin,
14+ stdout, stderr,
15+ bzrlib.commands.run_bzr_catch_user_errors,
16+ args)
17+ except KeyboardInterrupt:
18+ # Reraise KeyboardInterrupt with contents of redirected stdout and
19+ # stderr as arguments, for tests which are interested in stdout and
20+ # stderr and are expecting the exception.
21+ out = stdout.getvalue()
22+ err = stderr.getvalue()
23+ if out:
24+ self.log('output:\n%r', out)
25+ if err:
26+ self.log('errors:\n%r', err)
27+ raise KeyboardInterrupt(out, err)
28 finally:
29 logger.removeHandler(handler)
30 ui.ui_factory = old_ui_factory
31
32=== modified file 'bzrlib/tests/blackbox/test_serve.py'
33--- bzrlib/tests/blackbox/test_serve.py 2009-11-16 21:36:52 +0000
34+++ bzrlib/tests/blackbox/test_serve.py 2009-11-25 13:25:23 +0000
35@@ -45,8 +45,49 @@
36 from bzrlib.trace import mutter
37 from bzrlib.transport import get_transport, remote
38
39-
40-class TestBzrServe(TestCaseWithTransport):
41+class TestBzrServeBase(TestCaseWithTransport):
42+
43+ def run_bzr_serve_then_func(self, serve_args, retcode=0, func=None,
44+ *func_args, **func_kwargs):
45+ """Run 'bzr serve', and run the given func in a thread once the server
46+ has started.
47+
48+ When 'func' terminates, the server will be terminated too.
49+
50+ Returns stdout and stderr.
51+ """
52+ # install hook
53+ def on_server_start(backing_urls, tcp_server):
54+ t = threading.Thread(
55+ target=on_server_start_thread, args=(tcp_server,))
56+ t.start()
57+ def on_server_start_thread(tcp_server):
58+ try:
59+ # Run func if set
60+ self.tcp_server = tcp_server
61+ if not func is None:
62+ try:
63+ func(*func_args, **func_kwargs)
64+ except Exception, e:
65+ # Log errors to make some test failures a little less
66+ # mysterious.
67+ mutter('func broke: %r', e)
68+ finally:
69+ # Then stop the server
70+ mutter('interrupting...')
71+ thread.interrupt_main()
72+ SmartTCPServer.hooks.install_named_hook(
73+ 'server_started_ex', on_server_start,
74+ 'run_bzr_serve_then_func hook')
75+ # start a TCP server
76+ try:
77+ out, err = self.run_bzr(['serve'] + list(serve_args))
78+ except KeyboardInterrupt, e:
79+ out, err = e.args
80+ return out, err
81+
82+
83+class TestBzrServe(TestBzrServeBase):
84
85 def setUp(self):
86 super(TestBzrServe, self).setUp()
87@@ -119,6 +160,13 @@
88 url = 'bzr://localhost:%d/' % port
89 self.permit_url(url)
90 return process, url
91+
92+ def test_bzr_serve_quiet(self):
93+ self.make_branch('.')
94+ args = ['--port', 'localhost:0', '--quiet']
95+ out, err = self.run_bzr_serve_then_func(args, retcode=3)
96+ self.assertEqual('', out)
97+ self.assertEqual('', err)
98
99 def test_bzr_serve_inet_readonly(self):
100 """bzr server should provide a read only filesystem by default."""
101@@ -168,7 +216,7 @@
102 self.assertServerFinishesCleanly(process)
103
104
105-class TestCmdServeChrooting(TestCaseWithTransport):
106+class TestCmdServeChrooting(TestBzrServeBase):
107
108 def test_serve_tcp(self):
109 """'bzr serve' wraps the given --directory in a ChrootServer.
110@@ -183,47 +231,12 @@
111 ['--port', '127.0.0.1:0',
112 '--directory', t.local_abspath('server-root'),
113 '--allow-writes'],
114- self.when_server_started)
115+ func=self.when_server_started)
116 # The when_server_started method issued a find_repositoryV3 that should
117 # fail with 'norepository' because there are no repositories inside the
118 # --directory.
119 self.assertEqual(('norepository',), self.client_resp)
120
121- def run_bzr_serve_then_func(self, serve_args, func, *func_args,
122- **func_kwargs):
123- """Run 'bzr serve', and run the given func in a thread once the server
124- has started.
125-
126- When 'func' terminates, the server will be terminated too.
127- """
128- # install hook
129- def on_server_start(backing_urls, tcp_server):
130- t = threading.Thread(
131- target=on_server_start_thread, args=(tcp_server,))
132- t.start()
133- def on_server_start_thread(tcp_server):
134- try:
135- # Run func
136- self.tcp_server = tcp_server
137- try:
138- func(*func_args, **func_kwargs)
139- except Exception, e:
140- # Log errors to make some test failures a little less
141- # mysterious.
142- mutter('func broke: %r', e)
143- finally:
144- # Then stop the server
145- mutter('interrupting...')
146- thread.interrupt_main()
147- SmartTCPServer.hooks.install_named_hook(
148- 'server_started_ex', on_server_start,
149- 'run_bzr_serve_then_func hook')
150- # start a TCP server
151- try:
152- self.run_bzr(['serve'] + list(serve_args))
153- except KeyboardInterrupt:
154- pass
155-
156 def when_server_started(self):
157 # Connect to the TCP server and issue some requests and see what comes
158 # back.