Bazaar

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

2.3-per-transport-tests
Merge into bzr.dev

Proposed by John A Meinel on 2011-01-12

Status:

Merged

Approved by:

Vincent Ladeuil on 2011-01-12

Approved revision:

no longer in the source branch.

Merged at revision:

5604

Proposed branch:

lp://staging/~jameinel/bzr/2.3-per-transport-tests

Merge into:

lp://staging/bzr

Diff against target:

72 lines (+17/-6)

2 files modified

bzrlib/tests/per_transport.py (+10/-6)
doc/en/release-notes/bzr-2.3.txt (+7/-0)

To merge this branch:

bzr merge lp://staging/~jameinel/bzr/2.3-per-transport-tests

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Martin Packman (community)		2011-01-12	Approve on 2011-01-12
Review via email: mp+46047@code.staging.launchpad.net

Commit message

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

Description of the change

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.

Revision history for this message

Martin Packman (gz) wrote on 2011-01-12:

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

Revision history for this message

John A Meinel (jameinel) wrote on 2011-01-12:

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