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
=== modified file 'bzrlib/tests/blackbox/test_status.py'
--- bzrlib/tests/blackbox/test_status.py 2010-04-07 21:34:13 +0000
+++ bzrlib/tests/blackbox/test_status.py 2010-07-13 16:43:44 +0000
@@ -704,16 +704,6 @@
704704
705class TestStatusEncodings(TestCaseWithTransport):705class TestStatusEncodings(TestCaseWithTransport):
706706
707 def setUp(self):
708 TestCaseWithTransport.setUp(self)
709 self.user_encoding = osutils._cached_user_encoding
710 self.stdout = sys.stdout
711
712 def tearDown(self):
713 osutils._cached_user_encoding = self.user_encoding
714 sys.stdout = self.stdout
715 TestCaseWithTransport.tearDown(self)
716
717 def make_uncommitted_tree(self):707 def make_uncommitted_tree(self):
718 """Build a branch with uncommitted unicode named changes in the cwd."""708 """Build a branch with uncommitted unicode named changes in the cwd."""
719 working_tree = self.make_branch_and_tree(u'.')709 working_tree = self.make_branch_and_tree(u'.')
@@ -727,8 +717,7 @@
727 return working_tree717 return working_tree
728718
729 def test_stdout_ascii(self):719 def test_stdout_ascii(self):
730 sys.stdout = StringIO()720 self.overrideAttr(osutils, '_cached_user_encoding', 'ascii')
731 osutils._cached_user_encoding = 'ascii'
732 working_tree = self.make_uncommitted_tree()721 working_tree = self.make_uncommitted_tree()
733 stdout, stderr = self.run_bzr("status")722 stdout, stderr = self.run_bzr("status")
734723
@@ -738,8 +727,7 @@
738""")727""")
739728
740 def test_stdout_latin1(self):729 def test_stdout_latin1(self):
741 sys.stdout = StringIO()730 self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
742 osutils._cached_user_encoding = 'latin-1'
743 working_tree = self.make_uncommitted_tree()731 working_tree = self.make_uncommitted_tree()
744 stdout, stderr = self.run_bzr('status')732 stdout, stderr = self.run_bzr('status')
745733
746734
=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py 2010-06-11 10:59:23 +0000
+++ bzrlib/tests/test_http.py 2010-07-13 16:43:44 +0000
@@ -1125,10 +1125,7 @@
1125 def setUp(self):1125 def setUp(self):
1126 tests.TestCase.setUp(self)1126 tests.TestCase.setUp(self)
1127 self._old_env = {}1127 self._old_env = {}
11281128 self.addCleanup(self._restore_env)
1129 def tearDown(self):
1130 self._restore_env()
1131 tests.TestCase.tearDown(self)
11321129
1133 def _install_env(self, env):1130 def _install_env(self, env):
1134 for name, value in env.iteritems():1131 for name, value in env.iteritems():
@@ -1974,13 +1971,8 @@
1974 # We override at class level because constructors may propagate the1971 # We override at class level because constructors may propagate the
1975 # bound method and render instance overriding ineffective (an1972 # bound method and render instance overriding ineffective (an
1976 # alternative would be to define a specific ui factory instead...)1973 # alternative would be to define a specific ui factory instead...)
1977 self.orig_report_activity = self._transport._report_activity1974 self.overrideAttr(self._transport, '_report_activity', report_activity)
1978 self._transport._report_activity = report_activity1975 self.addCleanup(self.server.stop_server)
1979
1980 def tearDown(self):
1981 self._transport._report_activity = self.orig_report_activity
1982 self.server.stop_server()
1983 tests.TestCase.tearDown(self)
19841976
1985 def get_transport(self):1977 def get_transport(self):
1986 return self._transport(self.server.get_url())1978 return self._transport(self.server.get_url())
@@ -2103,51 +2095,22 @@
2103class TestActivity(tests.TestCase, TestActivityMixin):2095class TestActivity(tests.TestCase, TestActivityMixin):
21042096
2105 def setUp(self):2097 def setUp(self):
2106 tests.TestCase.setUp(self)2098 TestActivityMixin.setUp(self)
2107 self.server = self._activity_server(self._protocol_version)
2108 self.server.start_server()
2109 self.activities = {}
2110 def report_activity(t, bytes, direction):
2111 count = self.activities.get(direction, 0)
2112 count += bytes
2113 self.activities[direction] = count
2114
2115 # We override at class level because constructors may propagate the
2116 # bound method and render instance overriding ineffective (an
2117 # alternative would be to define a specific ui factory instead...)
2118 self.orig_report_activity = self._transport._report_activity
2119 self._transport._report_activity = report_activity
2120
2121 def tearDown(self):
2122 self._transport._report_activity = self.orig_report_activity
2123 self.server.stop_server()
2124 tests.TestCase.tearDown(self)
21252099
21262100
2127class TestNoReportActivity(tests.TestCase, TestActivityMixin):2101class TestNoReportActivity(tests.TestCase, TestActivityMixin):
21282102
2103 # Unlike TestActivity, we are really testing ReportingFileSocket and
2104 # ReportingSocket, so we don't need all the parametrization. Since
2105 # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
2106 # test them through their use by the transport than directly (that's a
2107 # bit less clean but far more simpler and effective).
2108 _activity_server = ActivityHTTPServer
2109 _protocol_version = 'HTTP/1.1'
2110
2129 def setUp(self):2111 def setUp(self):
2130 tests.TestCase.setUp(self)2112 self._transport =_urllib.HttpTransport_urllib
2131 # Unlike TestActivity, we are really testing ReportingFileSocket and2113 TestActivityMixin.setUp(self)
2132 # ReportingSocket, so we don't need all the parametrization. Since
2133 # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
2134 # test them through their use by the transport than directly (that's a
2135 # bit less clean but far more simpler and effective).
2136 self.server = ActivityHTTPServer('HTTP/1.1')
2137 self._transport=_urllib.HttpTransport_urllib
2138
2139 self.server.start_server()
2140
2141 # We override at class level because constructors may propagate the
2142 # bound method and render instance overriding ineffective (an
2143 # alternative would be to define a specific ui factory instead...)
2144 self.orig_report_activity = self._transport._report_activity
2145 self._transport._report_activity = None
2146
2147 def tearDown(self):
2148 self._transport._report_activity = self.orig_report_activity
2149 self.server.stop_server()
2150 tests.TestCase.tearDown(self)
21512114
2152 def assertActivitiesMatch(self):2115 def assertActivitiesMatch(self):
2153 # Nothing to check here2116 # Nothing to check here
21542117
=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py 2010-06-20 11:18:38 +0000
+++ bzrlib/tests/test_remote.py 2010-07-13 16:43:44 +0000
@@ -89,10 +89,7 @@
89 self.transport = self.get_transport()89 self.transport = self.get_transport()
90 # make a branch that can be opened over the smart transport90 # make a branch that can be opened over the smart transport
91 self.local_wt = BzrDir.create_standalone_workingtree('.')91 self.local_wt = BzrDir.create_standalone_workingtree('.')
9292 self.addCleanup(self.transport.disconnect)
93 def tearDown(self):
94 self.transport.disconnect()
95 tests.TestCaseWithTransport.tearDown(self)
9693
97 def test_create_remote_bzrdir(self):94 def test_create_remote_bzrdir(self):
98 b = remote.RemoteBzrDir(self.transport, remote.RemoteBzrDirFormat())95 b = remote.RemoteBzrDir(self.transport, remote.RemoteBzrDirFormat())
9996
=== modified file 'bzrlib/tests/test_smart_request.py'
--- bzrlib/tests/test_smart_request.py 2010-06-20 11:18:38 +0000
+++ bzrlib/tests/test_smart_request.py 2010-07-13 16:43:44 +0000
@@ -187,9 +187,11 @@
187187
188class TestJailHook(TestCaseWithMemoryTransport):188class TestJailHook(TestCaseWithMemoryTransport):
189189
190 def tearDown(self):190 def setUp(self):
191 request.jail_info.transports = None191 super(TestJailHook, self).setUp()
192 TestCaseWithMemoryTransport.tearDown(self)192 def clear_jail_info():
193 request.jail_info.transports = None
194 self.addCleanup(clear_jail_info)
193195
194 def test_jail_hook(self):196 def test_jail_hook(self):
195 request.jail_info.transports = None197 request.jail_info.transports = None
196198
=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt 2010-06-11 06:19:48 +0000
+++ doc/developers/testing.txt 2010-07-13 16:43:44 +0000
@@ -870,6 +870,25 @@
870Please see bzrlib.treebuilder for more details.870Please see bzrlib.treebuilder for more details.
871871
872872
873Temporarily changing state
874~~~~~~~~~~~~~~~~~~~~~~~~~~
875
876If your test needs to temporarily mutate some global state, and you need
877it restored at the end, you can say for example::
878
879 self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
880
881Cleaning up
882~~~~~~~~~~~
883
884Our base ``TestCase`` class provides an ``addCleanup`` method, which
885should be used instead of ``tearDown``. All the cleanups are run when the
886test finishes, regardless of whether it passes or fails. If one cleanup
887fails, later cleanups are still run.
888
889(The same facility is available outside of tests through
890``bzrlib.cleanup``.)
891
873.. |--| unicode:: U+2014892.. |--| unicode:: U+2014
874893
875..894..