Bazaar

Merge lp:~mbp/bzr/deprecation into lp:bzr

  1. deprecation
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Merged
Approved by: Martin Pool
Approved revision: no longer in the revision history of the source branch.
Merged at revision: 5343
Proposed branch: lp:~mbp/bzr/deprecation
Merge into: lp:bzr
Diff against target: 200 lines (+41/-72)
5 files modified
bzrlib/tests/blackbox/test_status.py (+2/-14)
bzrlib/tests/test_http.py (+14/-51)
bzrlib/tests/test_remote.py (+1/-4)
bzrlib/tests/test_smart_request.py (+5/-3)
doc/developers/testing.txt (+19/-0)
To merge this branch: bzr merge lp:~mbp/bzr/deprecation
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Vincent Ladeuil Approve
Robert Collins (community) Needs Fixing
Review via email: mp+27582@code.launchpad.net

Commit message

change some test tearDowns to addCleanup or overrideAttr

Description of the change

This deletes some tearDown methods from test cases and adds generally shorter calls to addCleanup, and a request people do this in future.

These cope better if some cleanups fail to run etc.

Now updated to use overrideAttr where appropriate, to suggest that in the docs, and to remove repetition from test_http.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

24 + saved_user_encoding = osutils._cached_user_encoding
25 + saved_stdout = sys.stdout
26 + def restore_stuff():
27 + osutils._cached_user_encoding = saved_user_encoding
28 + sys.stdout = saved_stdout
29 + self.addCleanup(restore_stuff)

You missed the even simpler:

self.overrideAttr(sys, 'stdout')
self.overrideAttr(osutils._cached_user_encoding)

which will set an illegal value that could even catch invalid tests there.

And looking at the tests, you don't even need to override sys.stdout anyway.

Hmm, your branch seems to be based on a bzr version that doesn't provide overrideAttr,
yet you target bzr.dev, is that really wanted ?

=== modified file 'bzrlib/tests/test_http.py'

Same remarks, plus it will conflict with my lealing-tests proposal, but I'll handle the conflicts there (I've addressed the same problem :)

This is still better than what we have so feel free to land whatever version you want, my vote is optional Tweak here :)

review: Needs Fixing
Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Revision history for this message
Robert Collins (lifeless) wrote :

This has conflicts as vincent warned about.

Whats up with this:

38 +<<<<<<< TREE
39 # Copyright (C) 2005-2010 Canonical Ltd
40 +=======
41 +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
42 +>>>>>>> MERGE-SOURCE
43 #

The 2005-2010 form is a lot pithier; I don't recall a policy decision to avoid it?

Could you please comment on vincents notes about using overrideAttr too. Thanks.

review: Needs Information
Revision history for this message
Martin Pool (mbp) wrote :

On 21 June 2010 07:29, Robert Collins <email address hidden> wrote:
> Review: Needs Information
> This has conflicts as vincent warned about.
>
> Whats up with this:
>
> 38      +<<<<<<< TREE
> 39       # Copyright (C) 2005-2010 Canonical Ltd
> 40      +=======
> 41      +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
> 42      +>>>>>>> MERGE-SOURCE
> 43       #
>
> The 2005-2010 form is a lot pithier; I don't recall a policy decision to avoid it?

I think John did a batch update to the first form and I arbitrarily
resolved the conflict to the first. The shorter form is fine with me.

> Could you please comment on vincents notes about using overrideAttr too. Thanks.

I didn't forget to use overrideAttr, I just wrote this before it was landed.

--
Martin

Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin Pool wrote:
> On 21 June 2010 07:29, Robert Collins <email address hidden> wrote:
>> Review: Needs Information
>> This has conflicts as vincent warned about.
>>
>> Whats up with this:
>>
>> 38 +<<<<<<< TREE
>> 39 # Copyright (C) 2005-2010 Canonical Ltd
>> 40 +=======
>> 41 +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010 Canonical Ltd
>> 42 +>>>>>>> MERGE-SOURCE
>> 43 #
>>
>> The 2005-2010 form is a lot pithier; I don't recall a policy decision to avoid it?
>
> I think John did a batch update to the first form and I arbitrarily
> resolved the conflict to the first. The shorter form is fine with me.
>
>> Could you please comment on vincents notes about using overrideAttr too. Thanks.
>
> I didn't forget to use overrideAttr, I just wrote this before it was landed.
>

I have a pre-commit hook "update-copyright" plugin that checks the
history for the file and updates the copyright line to match.

In the past there was a discussion of whether the pithy form was
"legally ok" (long time ago). So I used the expanded form. Then I saw
Martin manually writing the pithy form, and I was happy to update the
plugin to use it. (I prefer it, as it seems does everyone but
potentially lawyers.)

Vincent and I are both using the plugin, so things will converge on that
form. (It only updates files that are already modified.)

The only other difference is that it actually does the exact years. So a
file that was modified in 2005, 2006, 2007, 2009, 2010, 2011, will get 2
ranges: 2005-2007, 2009-2011, whereas a human is probably likely to just
collapse that to 2005-2011.

I *certainly* am not a lawyer, or even have a great grasp of why each
file has a copyright line and why that line differs per file. (Why not
just have 1 2005-2010 copyright line at the top of every file...)

John
=:->

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

iEYEARECAAYFAkwg2IEACgkQJdeBCYSNAANEjwCgm4V0Q+izQQ3CyLMrPbJTDaFR
hwEAn1nbcreJ8skIca0XY4flZ61vwPUx
=tpwn
-----END PGP SIGNATURE-----

Revision history for this message
Robert Collins (lifeless) wrote :

I need to look at TestActivityMixin - it looks, right now, like you end up not upcalling in setUp, and that should cause a failure in the test framework.

Other than that it looks pretty nice now. So 'tweak'.

review: Needs Fixing
Revision history for this message
Vincent Ladeuil (vila) wrote :

Nice cleanup, thanks.

131 + self._transport =_urllib.HttpTransport_urllib

missing space after the '=' sign (I know it wasn't there before either ;)

The distinction between TestActivity and TestNoReportActivity and why TestActivityMixin is required
is unclear (says who ?) so I mentioned it in https://bugs.edge.launchpad.net/bzr/+bug/597791/comments/1

Thanks to your cleanup the flaw is more obvious now.

review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

Revision history for this message
Martin Pool (mbp) wrote :

> I need to look at TestActivityMixin - it looks, right now, like you end up not
> upcalling in setUp, and that should cause a failure in the test framework.
>
> Other than that it looks pretty nice now. So 'tweak'.

It is just a mixin, and the concrete test subclasses do upcall (I think.)

There is a thing here where if it was sufficiently clean to do official parameterization, people wouldn't generate such mixins. But I got rid of a bit of copy and paste, perhaps vila will do more later.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> There is a thing here where if it was sufficiently clean to do official
> parameterization, people wouldn't generate such mixins. But I got rid of a
> bit of copy and paste, perhaps vila will do more later.

As part of bug #597791, yes.

Revision history for this message
Robert Collins (lifeless) wrote :

Ok, +1

Revision history for this message
John A Meinel (jameinel) wrote :

sent to pqm by email

Revision history for this message
John A Meinel (jameinel) wrote :

This is failing with:
======================================================================
ERROR: bzrlib.tests.test_smart_request.TestJailHook.test_jail_hook
----------------------------------------------------------------------
_StringException: Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 342, in _run_setup
    raise ValueError("setUp was not called")
ValueError: setUp was not called
------------

======================================================================
ERROR: bzrlib.tests.test_smart_request.TestJailHook.test_open_bzrdir_in_non_main_thread
----------------------------------------------------------------------
_StringException: Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line 342, in _run_setup
    raise ValueError("setUp was not called")
ValueError: setUp was not called
------------

----------------------------------------------------------------------

I think Vincent was right that your class needs to be calling super().setUp()

I think the issue is that if the base class is using super() then this class needs to use super(). If the child is using ParentClass.setUp(self) then you wouldn't, although in that case the *mixin*'s super would never get called.

review: Needs Fixing
lp:~mbp/bzr/deprecation updated
5325. By Canonical.com Patch Queue Manager <email address hidden>

(Martin von Gagern) Deal with branch: revision specs in readonly
 transactions

5326. By Canonical.com Patch Queue Manager <email address hidden>

(vila) Cleanup tests importing get_transport. (Vincent Ladeuil)

5327. By Canonical.com Patch Queue Manager <email address hidden>

(jameinel) locking for cmd_ignore is now in add_cleanup. (Parth Malwankar)

5328. By Canonical.com Patch Queue Manager <email address hidden>

(spiv) Fix Branch._unstack to work with repeatedly locked RemoteBranch
 objects. (#551525) (Andrew Bennetts)

5329. By Canonical.com Patch Queue Manager <email address hidden>

(parthm) fixed import order of lazy_regex w.r.t _format_version_tuple
 definition in bzrlib (Parth Malwankar)

5330. By Canonical.com Patch Queue Manager <email address hidden>

(jameinel) fixed trace import in bzrlib_initialize (Parth Malwankar)

5331. By Canonical.com Patch Queue Manager <email address hidden>

(jameinel) a couple of doc cleanups about the ppa (Martin Pool)

5332. By Canonical.com Patch Queue Manager <email address hidden>

(lifeless) Make lsprof usage serialise rather than generated corrupted
 results when used concurrently in a threaded application. Can now cause
 deadlocks when used reentrantly. (Robert Collins)

5333. By Canonical.com Patch Queue Manager <email address hidden>

(gz) Make windows installer use a 2 second resolution for plugin script
 timestamps to avoid recompilation at runtime (Martin [gz])

5334. By Canonical.com Patch Queue Manager <email address hidden>

(lifeless) Fix regression from r5326 that caused infinite recursion in
 osutils._win32_mkdtemp (Martin [gz])

5335. By Canonical.com Patch Queue Manager <email address hidden>

(lifeless) Remove duplicate test get_parent_map_after_insert_stream. (Robert
 Collins)

5336. By Canonical.com Patch Queue Manager <email address hidden>

(lifeless) Document the basedir on WorkingTree. (Robert Collins)

5337. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) developer docs about testing (Martin Pool)

5338. By Canonical.com Patch Queue Manager <email address hidden>

(jameinel) Use a better list of PyQt includes and excludes. (Gary van der
 Merwe)

5339. By Canonical.com Patch Queue Manager <email address hidden>

(parthm) Better regex compile errors (Parth Malwankar)

5340. By Canonical.com Patch Queue Manager <email address hidden>

(jameinel) Rename errors.InvalidPattern argument message=>msg to work with
 multiple Python versions. (Parth Malwankar)

5341. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Merge 2.2b4 to dev, bump the version numbers.

5342. By Canonical.com Patch Queue Manager <email address hidden>

(jam) Bump trunk to 2.3-dev1

Revision history for this message
Martin Pool (mbp) wrote :

sent to pqm by email

lp:~mbp/bzr/deprecation updated
5343. By Canonical.com Patch Queue Manager <email address hidden>

(mbp) change some test tearDowns to addCleanup or overrideAttr (Martin Pool)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/blackbox/test_status.py'
2--- bzrlib/tests/blackbox/test_status.py 2010-04-07 21:34:13 +0000
3+++ bzrlib/tests/blackbox/test_status.py 2010-07-13 16:43:44 +0000
4@@ -704,16 +704,6 @@
5
6 class TestStatusEncodings(TestCaseWithTransport):
7
8- def setUp(self):
9- TestCaseWithTransport.setUp(self)
10- self.user_encoding = osutils._cached_user_encoding
11- self.stdout = sys.stdout
12-
13- def tearDown(self):
14- osutils._cached_user_encoding = self.user_encoding
15- sys.stdout = self.stdout
16- TestCaseWithTransport.tearDown(self)
17-
18 def make_uncommitted_tree(self):
19 """Build a branch with uncommitted unicode named changes in the cwd."""
20 working_tree = self.make_branch_and_tree(u'.')
21@@ -727,8 +717,7 @@
22 return working_tree
23
24 def test_stdout_ascii(self):
25- sys.stdout = StringIO()
26- osutils._cached_user_encoding = 'ascii'
27+ self.overrideAttr(osutils, '_cached_user_encoding', 'ascii')
28 working_tree = self.make_uncommitted_tree()
29 stdout, stderr = self.run_bzr("status")
30
31@@ -738,8 +727,7 @@
32 """)
33
34 def test_stdout_latin1(self):
35- sys.stdout = StringIO()
36- osutils._cached_user_encoding = 'latin-1'
37+ self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
38 working_tree = self.make_uncommitted_tree()
39 stdout, stderr = self.run_bzr('status')
40
41
42=== modified file 'bzrlib/tests/test_http.py'
43--- bzrlib/tests/test_http.py 2010-06-11 10:59:23 +0000
44+++ bzrlib/tests/test_http.py 2010-07-13 16:43:44 +0000
45@@ -1125,10 +1125,7 @@
46 def setUp(self):
47 tests.TestCase.setUp(self)
48 self._old_env = {}
49-
50- def tearDown(self):
51- self._restore_env()
52- tests.TestCase.tearDown(self)
53+ self.addCleanup(self._restore_env)
54
55 def _install_env(self, env):
56 for name, value in env.iteritems():
57@@ -1974,13 +1971,8 @@
58 # We override at class level because constructors may propagate the
59 # bound method and render instance overriding ineffective (an
60 # alternative would be to define a specific ui factory instead...)
61- self.orig_report_activity = self._transport._report_activity
62- self._transport._report_activity = report_activity
63-
64- def tearDown(self):
65- self._transport._report_activity = self.orig_report_activity
66- self.server.stop_server()
67- tests.TestCase.tearDown(self)
68+ self.overrideAttr(self._transport, '_report_activity', report_activity)
69+ self.addCleanup(self.server.stop_server)
70
71 def get_transport(self):
72 return self._transport(self.server.get_url())
73@@ -2103,51 +2095,22 @@
74 class TestActivity(tests.TestCase, TestActivityMixin):
75
76 def setUp(self):
77- tests.TestCase.setUp(self)
78- self.server = self._activity_server(self._protocol_version)
79- self.server.start_server()
80- self.activities = {}
81- def report_activity(t, bytes, direction):
82- count = self.activities.get(direction, 0)
83- count += bytes
84- self.activities[direction] = count
85-
86- # We override at class level because constructors may propagate the
87- # bound method and render instance overriding ineffective (an
88- # alternative would be to define a specific ui factory instead...)
89- self.orig_report_activity = self._transport._report_activity
90- self._transport._report_activity = report_activity
91-
92- def tearDown(self):
93- self._transport._report_activity = self.orig_report_activity
94- self.server.stop_server()
95- tests.TestCase.tearDown(self)
96+ TestActivityMixin.setUp(self)
97
98
99 class TestNoReportActivity(tests.TestCase, TestActivityMixin):
100
101+ # Unlike TestActivity, we are really testing ReportingFileSocket and
102+ # ReportingSocket, so we don't need all the parametrization. Since
103+ # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
104+ # test them through their use by the transport than directly (that's a
105+ # bit less clean but far more simpler and effective).
106+ _activity_server = ActivityHTTPServer
107+ _protocol_version = 'HTTP/1.1'
108+
109 def setUp(self):
110- tests.TestCase.setUp(self)
111- # Unlike TestActivity, we are really testing ReportingFileSocket and
112- # ReportingSocket, so we don't need all the parametrization. Since
113- # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
114- # test them through their use by the transport than directly (that's a
115- # bit less clean but far more simpler and effective).
116- self.server = ActivityHTTPServer('HTTP/1.1')
117- self._transport=_urllib.HttpTransport_urllib
118-
119- self.server.start_server()
120-
121- # We override at class level because constructors may propagate the
122- # bound method and render instance overriding ineffective (an
123- # alternative would be to define a specific ui factory instead...)
124- self.orig_report_activity = self._transport._report_activity
125- self._transport._report_activity = None
126-
127- def tearDown(self):
128- self._transport._report_activity = self.orig_report_activity
129- self.server.stop_server()
130- tests.TestCase.tearDown(self)
131+ self._transport =_urllib.HttpTransport_urllib
132+ TestActivityMixin.setUp(self)
133
134 def assertActivitiesMatch(self):
135 # Nothing to check here
136
137=== modified file 'bzrlib/tests/test_remote.py'
138--- bzrlib/tests/test_remote.py 2010-06-20 11:18:38 +0000
139+++ bzrlib/tests/test_remote.py 2010-07-13 16:43:44 +0000
140@@ -89,10 +89,7 @@
141 self.transport = self.get_transport()
142 # make a branch that can be opened over the smart transport
143 self.local_wt = BzrDir.create_standalone_workingtree('.')
144-
145- def tearDown(self):
146- self.transport.disconnect()
147- tests.TestCaseWithTransport.tearDown(self)
148+ self.addCleanup(self.transport.disconnect)
149
150 def test_create_remote_bzrdir(self):
151 b = remote.RemoteBzrDir(self.transport, remote.RemoteBzrDirFormat())
152
153=== modified file 'bzrlib/tests/test_smart_request.py'
154--- bzrlib/tests/test_smart_request.py 2010-06-20 11:18:38 +0000
155+++ bzrlib/tests/test_smart_request.py 2010-07-13 16:43:44 +0000
156@@ -187,9 +187,11 @@
157
158 class TestJailHook(TestCaseWithMemoryTransport):
159
160- def tearDown(self):
161- request.jail_info.transports = None
162- TestCaseWithMemoryTransport.tearDown(self)
163+ def setUp(self):
164+ super(TestJailHook, self).setUp()
165+ def clear_jail_info():
166+ request.jail_info.transports = None
167+ self.addCleanup(clear_jail_info)
168
169 def test_jail_hook(self):
170 request.jail_info.transports = None
171
172=== modified file 'doc/developers/testing.txt'
173--- doc/developers/testing.txt 2010-06-11 06:19:48 +0000
174+++ doc/developers/testing.txt 2010-07-13 16:43:44 +0000
175@@ -870,6 +870,25 @@
176 Please see bzrlib.treebuilder for more details.
177
178
179+Temporarily changing state
180+~~~~~~~~~~~~~~~~~~~~~~~~~~
181+
182+If your test needs to temporarily mutate some global state, and you need
183+it restored at the end, you can say for example::
184+
185+ self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
186+
187+Cleaning up
188+~~~~~~~~~~~
189+
190+Our base ``TestCase`` class provides an ``addCleanup`` method, which
191+should be used instead of ``tearDown``. All the cleanups are run when the
192+test finishes, regardless of whether it passes or fails. If one cleanup
193+fails, later cleanups are still run.
194+
195+(The same facility is available outside of tests through
196+``bzrlib.cleanup``.)
197+
198 .. |--| unicode:: U+2014
199
200 ..