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

[Martin Pool]

change some test tearDowns to addCleanup or overrideAttr

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.

[Vincent Ladeuil (vila)]

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

[Martin Pool (mbp)]

sent to pqm by email

[Robert Collins (lifeless)]

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

[Martin Pool (mbp)]

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

[John A Meinel (jameinel)]

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

[Robert Collins (lifeless)]

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

[Vincent Ladeuil (vila)]

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

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

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

[Vincent Ladeuil (vila)]

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

[Robert Collins (lifeless)]

Ok, +1

[John A Meinel (jameinel)]

sent to pqm by email

[John A Meinel (jameinel)]

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]

5325. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-28

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

5326. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-30

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

5327. By Canonical.com Patch Queue Manager <email address hidden> on 2010-06-30

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

5328. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-01

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

5329. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-01

(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> on 2010-07-02

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

5331. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-03

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

5332. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-04

(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> on 2010-07-04

(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> on 2010-07-05

(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> on 2010-07-05

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

5336. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-06

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

5337. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-07

(mbp) developer docs about testing (Martin Pool)

5338. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-07

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

5339. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-08

(parthm) Better regex compile errors (Parth Malwankar)

5340. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-09

(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> on 2010-07-12

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

5342. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-13

(jam) Bump trunk to 2.3-dev1

[Martin Pool (mbp)]

sent to pqm by email

[lp:~mbp/bzr/deprecation]

5343. By Canonical.com Patch Queue Manager <email address hidden> on 2010-07-13

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

=== 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 @@
 
 class TestStatusEncodings(TestCaseWithTransport):
 
-    def setUp(self):
-        TestCaseWithTransport.setUp(self)
-        self.user_encoding = osutils._cached_user_encoding
-        self.stdout = sys.stdout
-
-    def tearDown(self):
-        osutils._cached_user_encoding = self.user_encoding
-        sys.stdout = self.stdout
-        TestCaseWithTransport.tearDown(self)
-
     def make_uncommitted_tree(self):
         """Build a branch with uncommitted unicode named changes in the cwd."""
         working_tree = self.make_branch_and_tree(u'.')
@@ -727,8 +717,7 @@
         return working_tree
 
     def test_stdout_ascii(self):
-        sys.stdout = StringIO()
-        osutils._cached_user_encoding = 'ascii'
+        self.overrideAttr(osutils, '_cached_user_encoding', 'ascii')
         working_tree = self.make_uncommitted_tree()
         stdout, stderr = self.run_bzr("status")
 
@@ -738,8 +727,7 @@
 """)
 
     def test_stdout_latin1(self):
-        sys.stdout = StringIO()
-        osutils._cached_user_encoding = 'latin-1'
+        self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
         working_tree = self.make_uncommitted_tree()
         stdout, stderr = self.run_bzr('status')
 

=== 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 @@
     def setUp(self):
         tests.TestCase.setUp(self)
         self._old_env = {}
-
-    def tearDown(self):
-        self._restore_env()
-        tests.TestCase.tearDown(self)
+        self.addCleanup(self._restore_env)
 
     def _install_env(self, env):
         for name, value in env.iteritems():
@@ -1974,13 +1971,8 @@
         # We override at class level because constructors may propagate the
         # bound method and render instance overriding ineffective (an
         # alternative would be to define a specific ui factory instead...)
-        self.orig_report_activity = self._transport._report_activity
-        self._transport._report_activity = report_activity
-
-    def tearDown(self):
-        self._transport._report_activity = self.orig_report_activity
-        self.server.stop_server()
-        tests.TestCase.tearDown(self)
+        self.overrideAttr(self._transport, '_report_activity', report_activity)
+        self.addCleanup(self.server.stop_server)
 
     def get_transport(self):
         return self._transport(self.server.get_url())
@@ -2103,51 +2095,22 @@
 class TestActivity(tests.TestCase, TestActivityMixin):
 
     def setUp(self):
-        tests.TestCase.setUp(self)
-        self.server = self._activity_server(self._protocol_version)
-        self.server.start_server()
-        self.activities = {}
-        def report_activity(t, bytes, direction):
-            count = self.activities.get(direction, 0)
-            count += bytes
-            self.activities[direction] = count
-
-        # We override at class level because constructors may propagate the
-        # bound method and render instance overriding ineffective (an
-        # alternative would be to define a specific ui factory instead...)
-        self.orig_report_activity = self._transport._report_activity
-        self._transport._report_activity = report_activity
-
-    def tearDown(self):
-        self._transport._report_activity = self.orig_report_activity
-        self.server.stop_server()
-        tests.TestCase.tearDown(self)
+        TestActivityMixin.setUp(self)
 
 
 class TestNoReportActivity(tests.TestCase, TestActivityMixin):
 
+    # Unlike TestActivity, we are really testing ReportingFileSocket and
+    # ReportingSocket, so we don't need all the parametrization. Since
+    # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
+    # test them through their use by the transport than directly (that's a
+    # bit less clean but far more simpler and effective).
+    _activity_server = ActivityHTTPServer
+    _protocol_version = 'HTTP/1.1'
+
     def setUp(self):
-        tests.TestCase.setUp(self)
-        # Unlike TestActivity, we are really testing ReportingFileSocket and
-        # ReportingSocket, so we don't need all the parametrization. Since
-        # ReportingFileSocket and ReportingSocket are wrappers, it's easier to
-        # test them through their use by the transport than directly (that's a
-        # bit less clean but far more simpler and effective).
-        self.server = ActivityHTTPServer('HTTP/1.1')
-        self._transport=_urllib.HttpTransport_urllib
-
-        self.server.start_server()
-
-        # We override at class level because constructors may propagate the
-        # bound method and render instance overriding ineffective (an
-        # alternative would be to define a specific ui factory instead...)
-        self.orig_report_activity = self._transport._report_activity
-        self._transport._report_activity = None
-
-    def tearDown(self):
-        self._transport._report_activity = self.orig_report_activity
-        self.server.stop_server()
-        tests.TestCase.tearDown(self)
+        self._transport =_urllib.HttpTransport_urllib
+        TestActivityMixin.setUp(self)
 
     def assertActivitiesMatch(self):
         # Nothing to check here

=== 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 @@
         self.transport = self.get_transport()
         # make a branch that can be opened over the smart transport
         self.local_wt = BzrDir.create_standalone_workingtree('.')
-
-    def tearDown(self):
-        self.transport.disconnect()
-        tests.TestCaseWithTransport.tearDown(self)
+        self.addCleanup(self.transport.disconnect)
 
     def test_create_remote_bzrdir(self):
         b = remote.RemoteBzrDir(self.transport, remote.RemoteBzrDirFormat())

=== 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 @@
 
 class TestJailHook(TestCaseWithMemoryTransport):
 
-    def tearDown(self):
-        request.jail_info.transports = None
-        TestCaseWithMemoryTransport.tearDown(self)
+    def setUp(self):
+        super(TestJailHook, self).setUp()
+        def clear_jail_info():
+            request.jail_info.transports = None
+        self.addCleanup(clear_jail_info)
 
     def test_jail_hook(self):
         request.jail_info.transports = None

=== 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 @@
 Please see bzrlib.treebuilder for more details.
 
 
+Temporarily changing state
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If your test needs to temporarily mutate some global state, and you need
+it restored at the end, you can say for example::
+
+    self.overrideAttr(osutils, '_cached_user_encoding', 'latin-1')
+
+Cleaning up
+~~~~~~~~~~~
+
+Our base ``TestCase`` class provides an ``addCleanup`` method, which
+should be used instead of ``tearDown``.  All the cleanups are run when the
+test finishes, regardless of whether it passes or fails.  If one cleanup
+fails, later cleanups are still run.
+
+(The same facility is available outside of tests through
+``bzrlib.cleanup``.)
+
 .. |--| unicode:: U+2014
 
 ..