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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Needs Fixing | ||
Robert Collins (community) | Approve | ||
Review via email: mp+15112@code.launchpad.net |
This proposal supersedes a proposal from 2009-11-20.
Commit message
Description of the change
Gordon Tyler (doxxx) wrote : Posted in a previous version of this proposal | # |
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
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://
iEYEARECAAYFAks
f9sAoLFJNgWPdpK
=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_
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 TestCmdServeChr
On a related note, why do the other tests in TestBzrServe launch a subprocess and not do the same thing as TestCmdServeChr
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.
Robert Collins (lifeless) wrote : | # |
The change to setUp is wrong - has to be fixed. This can be done as its merged.
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 _TestBzrServeBa
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_
+ 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_
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.
Vincent Ladeuil (vila) wrote : | # |
Excellent, thanks, I'll merge (just checked that all tests pass for python 2.[456]).
Preview Diff
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. |
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.