Merge lp:~bac/launchpad/bug-987898 into lp:launchpad

[Brad Crittenden]

The use of mutable class data in a mock transport caused test isolation issues. Fix by copying class data to instance data in init.

= Summary =

Test isolation error causes failure when tests are run out of order
and sample data is changed.

Normally the following tests are run in this order:
lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt
lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt

However, due to shuffling in the parallel test environment they may be
run in the opposite order which exposes an error.

bugzilla-api-xmlrpc-transport.txt modifies the sample data for bug 1
which is then queried by bugzilla-xmlrpc-transport.txt. The expected
results are now modified when the ordering changes.

== Proposed fix ==

To account for the tests being run in either order, the offending data
is wildcarded with ellipsis in the doctest.

== Pre-implementation notes ==

None

== Implementation details ==

As above.

== Tests ==
bin/test -vv --load-list tests.txt

Where tests.txt contains:
lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt
lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt

== Demo and Q/A ==

N/A

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt

./lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt
     451: source exceeds 78 characters.

Will fix the lint.

[Gary Poster (gary)]

Brad and I talked. I have admittedly discouraged taking time on digging into some bugfixes, favoring doing the known right thing rather than digging into the underlying cause that makes the known right thing necessary (see bug 987903). However, in this case, switching from sample data to factories is a known better fix, and Brad says it should take a relatively small amount of time (less than a day) and that he is game to do so. I prefer that.

[Gary Poster (gary)]

Very nice!

(You communicated with me on IRC that the issue here was more complicated that a sample data -> factory change; what you've done here looks good in terms of adding better isolation.)

review: Approve

=== modified file 'lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt	2009-12-23 12:14:59 +0000
+++ lib/lp/bugs/tests/bugzilla-api-xmlrpc-transport.txt	2012-05-02 17:37:24 +0000
@@ -102,7 +102,7 @@
     >>> [bug_dict] = return_value['bugs']
     >>> for key in sorted(bug_dict):
     ...     print "%s: %s" % (key, bug_dict[key])
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53
@@ -156,7 +156,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -175,7 +175,7 @@
     >>> for bug_dict in bug_dicts:
     ...     for key in sorted(bug_dict):
     ...         print "%s: %s" % (key, bug_dict[key])
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53
@@ -224,7 +224,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -239,7 +239,7 @@
     >>> for bug_dict in bug_dicts:
     ...     for key in sorted(bug_dict):
     ...         print "%s: %s" % (key, bug_dict[key])
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53
@@ -264,7 +264,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -481,7 +481,7 @@
     >>> bug_dict = return_value['bugs'][0]
     >>> for key in sorted(bug_dict):
     ...     print "%s: %s" % (key, bug_dict.get(key))
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53

=== modified file 'lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt'
--- lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt	2010-11-08 14:16:17 +0000
+++ lib/lp/bugs/tests/bugzilla-xmlrpc-transport.txt	2012-05-02 17:37:24 +0000
@@ -141,7 +141,7 @@
     >>> [bug_dict] = return_value['bugs']
     >>> for key in sorted(bug_dict):
     ...     print "%s: %s" % (key, bug_dict[key])
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53
@@ -167,7 +167,7 @@
     ...     for key in sorted(bug_dict):
     ...         print "%s: %s" % (key, bug_dict[key])
     ...     print
-    alias: 
+    alias:
     assigned_to: [e-mail]
     component: GPPSystems
     creation_time: 2008-06-10 16:19:53
@@ -216,7 +216,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -252,7 +252,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -298,7 +298,7 @@
     last_change_time: 2008-06-11 09:24:29
     priority: P1
     product: HeartOfGold
-    resolution: 
+    resolution:
     see_also: []
     severity: high
     status: NEW
@@ -448,7 +448,8 @@
 
     >>> comment = "Didn't we have a lovely time the day we went to Bangor?"
     >>> bugzilla_transport.auth_cookie = 'open sesame'
-    >>> return_dict = server.Launchpad.add_comment({'id': 1, 'comment': comment})
+    >>> return_dict = server.Launchpad.add_comment(
+    ...     {'id': 1, 'comment': comment})
     >>> print return_dict['comment_id']
     5
 

=== modified file 'lib/lp/bugs/tests/externalbugtracker.py'
--- lib/lp/bugs/tests/externalbugtracker.py	2012-01-20 16:11:11 +0000
+++ lib/lp/bugs/tests/externalbugtracker.py	2012-05-02 17:37:24 +0000
@@ -7,6 +7,7 @@
 
 __metaclass__ = type
 
+from copy import deepcopy
 from datetime import (
     datetime,
     timedelta,
@@ -390,7 +391,7 @@
     utc_offset = 0
     print_method_calls = False
 
-    bugs = {
+    _bugs = {
         1: {'alias': '',
             'assigned_to': '[e-mail]',
             'component': 'GPPSystems',
@@ -426,14 +427,14 @@
         }
 
     # Map aliases onto bugs.
-    bug_aliases = {
+    _bug_aliases = {
         'bug-two': 2,
         }
 
     # Comments are mapped to bug IDs.
     comment_id_index = 4
     new_comment_time = datetime(2008, 6, 20, 11, 42, 42)
-    bug_comments = {
+    _bug_comments = {
         1: {
             1: {'author': 'trillian',
                 'id': 1,
@@ -467,26 +468,35 @@
 
     # Map namespaces onto method names.
     methods = {
-        'Launchpad': [
+        'Launchpad': (
             'add_comment',
             'comments',
             'get_bugs',
             'login',
             'time',
             'set_link',
-            ],
+            ),
         'Test': ['login_required'],
         }
 
     # Methods that require authentication.
-    auth_required_methods = [
+    auth_required_methods = (
         'add_comment',
         'login_required',
         'set_link',
-        ]
+        )
 
     expired_cookie = None
 
+    def __init__(self, *args, **kwargs):
+        """Ensure mutable class data is copied to the instance."""
+        # UrlLib2Transport is not a new style class so 'super' cannot be
+        # used.
+        UrlLib2Transport.__init__(self, *args, **kwargs)
+        self.bugs = deepcopy(TestBugzillaXMLRPCTransport._bugs)
+        self.bug_aliases = deepcopy(self._bug_aliases)
+        self.bug_comments = deepcopy(self._bug_comments)
+
     def expireCookie(self, cookie):
         """Mark the cookie as expired."""
         self.expired_cookie = cookie
@@ -800,7 +810,7 @@
         ]
 
     # A list of comments on bugs.
-    bug_comments = {
+    _bug_comments = {
         1: {
             1: {'author': 'trillian',
                 'bug_id': 1,
@@ -836,6 +846,10 @@
             },
         }
 
+    def __init__(self, *args, **kwargs):
+        """Ensure mutable class data is copied to the instance."""
+        TestBugzillaXMLRPCTransport.__init__(self, *args, **kwargs)
+
     def version(self):
         """Return the version of Bugzilla being used."""
         # This is to work around the old "xmlrpclib tries to expand