Merge lp:~jameinel/bzr/2.3-per-transport-tests into lp:bzr

[John A Meinel]

Use Transport.get_bytes() instead of Transport.get().read() in the test suite.

http://babune.ladeuil.net:24842/job/selftest-windows/lastCompletedBuild/testReport/bzrlib.tests.per_transport/TransportTests/test_put_bytes_SFTPTransport_SFTPAbsoluteServer_/

I think this failure is just a race condition, where the server/etc is still holding the file open, and then we are trying to write over top of it.

I *hope* the issue is that .get().read() is not very atomic and quick at closing its file handle. So I switched to .get_bytes() which should be a bit better at knowing that it can close the handle immediately.

[Martin Packman (gz)]

Code changes all look right to me, am not completely clear on this hunk though:

             self.assertEqual('b', t.get_bytes('foo'))
- self.assertEqual('b', t.get('foo').read())
+ f = t.get('foo')
+ try:
+ self.assertEqual('b', f.read())
+ finally:
+ f.close()

What's the intended effect there, that's different from two get_bytes calls or one get and two reads?

I'm not sure if this'll fix the random failures, but vila also suspects this as a potential issue in bug 681047. I'd assumed refcounts would be doing the right thing here anyway, but jam teaches me paramiko does slightly different things in __del__ from a normal close.

review: Approve

[John A Meinel (jameinel)]

that is testing that the t.get() api understands when a file has been mutated via the write_stream api.

I did want to make sure that both .get() and .get_bytes() were updated to be aware of writing from a different method.

*many* of the transports implement get_bytes() in terms of get() or get() in terms of get_bytes() but there is no guarantee that they do so.

So I felt it was still reasonable to do a separate get + read + close.

I do feel like most of the random failures you would see should be fixed by:
8 - """Check that transport.get(relpath).read() == content."""
9 - self.assertEqualDiff(content, transport.get(relpath).read())
10 + """Check that transport.get_bytes(relpath) == content."""
11 + self.assertEqualDiff(content, transport.get_bytes(relpath))

[John A Meinel (jameinel)]

sent to pqm by email

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2011-01-10 22:20:12 +0000
+++ bzrlib/tests/per_transport.py	2011-01-13 01:04:35 +0000
@@ -101,8 +101,8 @@
         self.overrideEnv('BZR_NO_SMART_VFS', None)
 
     def check_transport_contents(self, content, transport, relpath):
-        """Check that transport.get(relpath).read() == content."""
-        self.assertEqualDiff(content, transport.get(relpath).read())
+        """Check that transport.get_bytes(relpath) == content."""
+        self.assertEqualDiff(content, transport.get_bytes(relpath))
 
     def test_ensure_base_missing(self):
         """.ensure_base() should create the directory if it doesn't exist"""
@@ -256,7 +256,7 @@
         handle = t.open_write_stream('foo')
         try:
             handle.write('b')
-            self.assertEqual('b', t.get('foo').read())
+            self.assertEqual('b', t.get_bytes('foo'))
         finally:
             handle.close()
 
@@ -268,7 +268,11 @@
         try:
             handle.write('b')
             self.assertEqual('b', t.get_bytes('foo'))
-            self.assertEqual('b', t.get('foo').read())
+            f = t.get('foo')
+            try:
+                self.assertEqual('b', f.read())
+            finally:
+                f.close()
         finally:
             handle.close()
 
@@ -640,7 +644,7 @@
             self.build_tree(files, transport=transport_from)
             self.assertEqual(4, transport_from.copy_to(files, transport_to))
             for f in files:
-                self.check_transport_contents(transport_to.get(f).read(),
+                self.check_transport_contents(transport_to.get_bytes(f),
                                               transport_from, f)
 
         t = self.get_transport()
@@ -669,7 +673,7 @@
         files = ['a', 'b', 'c', 'd']
         t.copy_to(iter(files), temp_transport)
         for f in files:
-            self.check_transport_contents(temp_transport.get(f).read(),
+            self.check_transport_contents(temp_transport.get_bytes(f),
                                           t, f)
         del temp_transport
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-01-12 22:20:59 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-01-13 01:04:35 +0000
@@ -73,6 +73,13 @@
   trying to set the tags in the master branch. This had been broken by the
   bug fix for bug #603395.  (John Arbash Meinel, #701212)
 
+* Per-transport tests now prefer to use ``Transport.get_bytes()`` rather
+  than ``Transport.get().read()``. The SFTP code uses an async message to
+  close the file handle if you let the handle die from refcounting, while
+  it uses a synchronous message if you close it directly. This should help
+  prevent random test suite failures from race conditions.
+  (John Arbash Meinel, #681047)
+
 * Stop using ``bzrlib.tuned_gzip.GzipFile``. It is incompatible with
   python-2.7 and was only used for Knit format repositories, which haven't
   been recommended since 2007. The file itself will be removed in the next