Merge lp:~jameinel/bzr/2.4-overridAttr-non-existant into lp:bzr/2.4

[John A Meinel]

Teach TestCase.overrideAttr how to handle attributes that don't exist yet.

This is a precursor branch for another fix I want to land.

Essentially, TestCase.overrideAttr didn't handle the case where you want to set an attribute and that attribute doesn't exist. (I'm using it in a follow up branch to force an OS level function to a known state.)

I'm just submitting it now because it should be clearer factored out into 2 patches.

[Martin Packman (gz)]

Looks fine, but I'm not sure having special logic for the case where new is uninited and value is uninited makes sense. That doesn't seem like something a test author would do deliberately, except perhaps if they were later in the test setting the attribute to something and wanted the cleanup at the end, in which case the value would leak through in this case.

review: Approve

[Vincent Ladeuil (vila)]

IIRC we didn't implement the unexisting attr case because we didn't need it.

Later on, testtools provided 'patch' which takes this case into account.

I think it would be better to use the testtools version for your specific case.

Also, unless you intend to cut a release for 2.4, I think we should avoid landing patches on stable releases for which we don't intend to make a release, the SRU process is resource intensive and we still haven't a 2.6.0 release yet...

[John A Meinel (jameinel)]

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

On 11/6/2012 12:06 PM, Martin Packman wrote:
> Review: Approve
>
> Looks fine, but I'm not sure having special logic for the case
> where new is uninited and value is uninited makes sense. That
> doesn't seem like something a test author would do deliberately,
> except perhaps if they were later in the test setting the attribute
> to something and wanted the cleanup at the end, in which case the
> value would leak through in this case.
>

It happens explicitly on Windows when I'm testing that the test case
handles when ENOTSUP doesn't exist. Either I support that case, or I
have to add logic around:
 if getattr(foo, "bar", None) is not None:
    self.overrideAttr(foo, "bar")

In other words, I still need to only remove the attribute if it
exists, and I'd rather have that logic in one place.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCYzZ4ACgkQJdeBCYSNAAOZ4gCgx2Z8Lk4Ud0wRXD9TlRb2cx8w
tVQAn28G5xt6eGbwuuzy+CeOrM55Fwtx
=em4F
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

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

On 11/6/2012 12:39 PM, Vincent Ladeuil wrote:
> IIRC we didn't implement the unexisting attr case because we didn't
> need it.
>
> Later on, testtools provided 'patch' which takes this case into
> account.
>
> I think it would be better to use the testtools version for your
> specific case.
>
> Also, unless you intend to cut a release for 2.4, I think we should
> avoid landing patches on stable releases for which we don't intend
> to make a release, the SRU process is resource intensive and we
> still haven't a 2.6.0 release yet...
>

So I intended to do the SSH fdatasync patch in the release that added
fdatasync (2.4). I'd rather have that patch in a 2.4 series that
people can download if they want even if we don't SRU it, rather than
only land it in something newer.

I don't plan on SRUing this, but I still feel it is worth landing it
in a branch. If we ever do a 2.4 SRU, then we have this patch
available, rather than having to think about it after the fact, and
possibly backport a fix.

I tried grepping through the codebase, and could find no references to
testtools.monkey.patch. Even in bzr.dev tip.

Also, reading the code in monkey.py it *doesn't* handle the delattr
case. At least in 0.9.14.final.0 that I have here.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCYz8sACgkQJdeBCYSNAANxmACg2DZAYhtSTKtWOMUScq5t5PMe
658AoNSuBXU/cFzZ4hB8O2CRYHjRKacm
=GGKp
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 11/6/2012 12:39 PM, Vincent Ladeuil wrote:
> > IIRC we didn't implement the unexisting attr case because we didn't
> > need it.
> >
> > Later on, testtools provided 'patch' which takes this case into
> > account.
> >
> > I think it would be better to use the testtools version for your
> > specific case.
> >
> > Also, unless you intend to cut a release for 2.4, I think we should
> > avoid landing patches on stable releases for which we don't intend
> > to make a release, the SRU process is resource intensive and we
> > still haven't a 2.6.0 release yet...
> >
>
> So I intended to do the SSH fdatasync patch in the release that added
> fdatasync (2.4). I'd rather have that patch in a 2.4 series that
> people can download if they want even if we don't SRU it, rather than
> only land it in something newer.

Daggy fixing as you did is enough (and a Good Thing anyway) for people to easily get the patch if they need.

>
> I don't plan on SRUing this,

Then don't make a potentially critical and required SRU harder by adding code that is not mandatory.

> I tried grepping through the codebase, and could find no references to
> testtools.monkey.patch. Even in bzr.dev tip.

Yes, because the existing overrideAttr cover all the needed cases.

>
> Also, reading the code in monkey.py it *doesn't* handle the delattr
> case. At least in 0.9.14.final.0 that I have here.

It is there in 0.9.12 and... still there on tip, check again (hint: restore()).

[John A Meinel (jameinel)]

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

>> Also, reading the code in monkey.py it *doesn't* handle the
>> delattr case. At least in 0.9.14.final.0 that I have here.
>
> It is there in 0.9.12 and... still there on tip, check again (hint:
> restore()).
>

It will delete an attribute that did not previously exist. There is
*no way* to specify that you want to delete an attribute that already
exists. (Hint *patch*)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iEYEARECAAYFAlCZV1oACgkQJdeBCYSNAANGQACfQP4RdjnkBz6nYnochK/lvd9J
x1UAnA7E/yJinS5cxkveUYCOqBqSwn/g
=nb5c
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

sent to pqm by email

[Vincent Ladeuil (vila)]

> It will delete an attribute that did not previously exist. There is *no way* to specify that you want to delete an attribute that already exists. (Hint *patch*)

So you're taking the same syntax as 'patch' to mean something completely different ?

And you don't mention it in the docstring, news entry, cover letter ?

The fact that I mis-read your patch and that you fail to explain the difference with patch clearly demonstrate that other people will fall in the same trap.

Not to mention the other trap described by mgz.

A far safer and clearer idiom would be:

overrideAttr(obj, name) # Guarantees isolation
delattr(obj, name) # Testing what happens when name is deleted

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-11-30 22:47:17 +0000
+++ bzrlib/tests/__init__.py	2012-11-05 11:40:33 +0000
@@ -1751,9 +1751,15 @@
 
         :returns: The actual attr value.
         """
-        value = getattr(obj, attr_name)
         # The actual value is captured by the call below
-        self.addCleanup(setattr, obj, attr_name, value)
+        value = getattr(obj, attr_name, _unitialized_attr)
+        if value is _unitialized_attr:
+            # When the test completes, the attribute should not exist, but if
+            # we aren't setting a value, we don't need to do anything.
+            if new is not _unitialized_attr:
+                self.addCleanup(delattr, obj, attr_name)
+        else:
+            self.addCleanup(setattr, obj, attr_name, value)
         if new is not _unitialized_attr:
             setattr(obj, attr_name, new)
         return value
 
=== modified file 'bzrlib/tests/test_selftest.py'
--- bzrlib/tests/test_selftest.py	2011-10-26 16:42:12 +0000
+++ bzrlib/tests/test_selftest.py	2012-11-05 11:40:33 +0000
@@ -1651,6 +1651,12 @@
         self.assertRaises(AssertionError,
             self.assertListRaises, _TestException, success_generator)
 
+    def _run_successful_test(self, test):
+        result = testtools.TestResult()
+        test.run(result)
+        self.assertTrue(result.wasSuccessful())
+        return result
+
     def test_overrideAttr_without_value(self):
         self.test_attr = 'original' # Define a test attribute
         obj = self # Make 'obj' visible to the embedded test
@@ -1666,8 +1672,7 @@
                 obj.test_attr = 'modified'
                 self.assertEqual('modified', obj.test_attr)
 
-        test = Test('test_value')
-        test.run(unittest.TestResult())
+        self._run_successful_test(Test('test_value'))
         self.assertEqual('original', obj.test_attr)
 
     def test_overrideAttr_with_value(self):
@@ -1683,10 +1688,41 @@
                 self.assertEqual('original', self.orig)
                 self.assertEqual('modified', obj.test_attr)
 
-        test = Test('test_value')
-        test.run(unittest.TestResult())
+        self._run_successful_test(Test('test_value'))
         self.assertEqual('original', obj.test_attr)
 
+    def test_overrideAttr_with_no_existing_value_and_value(self):
+        # Do not define the test_attribute
+        obj = self # Make 'obj' visible to the embedded test
+        class Test(tests.TestCase):
+
+            def setUp(self):
+                tests.TestCase.setUp(self)
+                self.orig = self.overrideAttr(obj, 'test_attr', new='modified')
+
+            def test_value(self):
+                self.assertEqual(tests._unitialized_attr, self.orig)
+                self.assertEqual('modified', obj.test_attr)
+
+        self._run_successful_test(Test('test_value'))
+        self.assertRaises(AttributeError, getattr, obj, 'test_attr')
+
+    def test_overrideAttr_with_no_existing_value_and_no_value(self):
+        # Do not define the test_attribute
+        obj = self # Make 'obj' visible to the embedded test
+        class Test(tests.TestCase):
+
+            def setUp(self):
+                tests.TestCase.setUp(self)
+                self.orig = self.overrideAttr(obj, 'test_attr')
+
+            def test_value(self):
+                self.assertEqual(tests._unitialized_attr, self.orig)
+                self.assertRaises(AttributeError, getattr, obj, 'test_attr')
+
+        self._run_successful_test(Test('test_value'))
+        self.assertRaises(AttributeError, getattr, obj, 'test_attr')
+
     def test_recordCalls(self):
         from bzrlib.tests import test_selftest
         calls = self.recordCalls(