[RFC][PATCH]

[Robert Collins]

On Wed, 2006-01-04 at 00:03 -0600, John A Meinel wrote:
> Robert Collins wrote:
> > Hi, http://people.ubuntu.com/~robertc/baz2.0/test-providers is ready for
> > a review.
> > 
> > I think its ready for merge with one caveat, which I need some input on:
> > the new mode tests.
> 
> Well, other than the fact that fancy_rename was borked causing *many* of
> the tests to fail (which was fixed in the same manner, just catch
> ENOTDIR and ENOENT).
> It seems reasonable.

Ah, thats the merge from my integration branch, when I merge you
tomorrow it will come good ;).

> I would like to see it easier to add new transports, since right now all
> of the transports have to be instantiated directly by the TransportAdapter.

I'm not sure what you mean here - adding a transport with tests is
simple - write the transport and one or more servers, then stash the
permutations you want in transport/__init__.py. We can push this down
further into each treansport by having the transports advertise what
permutations they need tested I guess - I'm pro that in fact. But I was
concerned about the split between having the tests always run, and
having transports that are not importable. (Myself, I'd rather nuke the
lazy import stuff and -require- all known things to be available, but
thats a different discussion).

> also, you mix where the test code is, and where real code is. I thought
> we wanted to keep them apart.

How so? The Server classes are real code - they get tested as much as
the transport client that uses them. One can for instance run up a sftp
server for bzr by instantiation of bzrlib.transport.sftp.SFTPServer. The
TestCases for testing the implementations, and the TestCaseWithFOOServer
lasses are still in bzrlib.tests. I think these should really be in
bzrlib.transport.tests.* - grouping them by the package they are in. 

> There were a couple of new exceptions that weren't in errors.py, though
> it didn't seem bad.

hmm, I dont recall defining *any* new exceptions in this branch - they
already existed elsewhere, just got moved to the relevant transports.
Fair comment that we should address them though.


> > 
> > 
> > The problem is discriminating between transports that can set modes, and
> > transports that cant set modes. 
> > 
> > For instance, HTTP can never do modes - the protocol does not 'get' it.
> > SFTP can - and we could make stub_server talk to a transport instance
> > with MemoryTransport backing it, to get a sftp server with mode support
> > on windows.
> > MemoryTransport can do modes, but does not write to the fs.
> 
> Well, you would have to have the local transport server also use a
> memory backed stat on windows if you want mode support across the board.

Sure - but that wasn't the scope ;). The question was, how to test the
sftp client with a sftp server that supported modes, on windows.

> > 
> > So right now Johns new transport tests are inactive, because they are
> > all in the defunct test_transport TransportTestsMixIn class. 
> > 
> > I can see several routes out:
> > 
> > a) test mode setting as part of the Transport interface: check the modes
> > are set via stat(), and skip those tests on win32 and for read only
> > transports.
> > b) have two implementation tests for transport - one for the primary
> > interface, and one for mode aware transports 
> > c) test that the mode is set on the backend correctly on a per transport
> > basis (basically c, but along the lines of the old transport tests.)
> > 
> > My preference is a), but I wanted input first..
> > 
> > and separately to that, I'd just -love- a +1 for the rest of the branch.
> > 
> > Rob
> > 
> 
> I would go for a) as well. I'm +0.5 right now, since a 3k diff is a lot
> to go through.

a) - right, will do that tomorrow.

> Can you reexplain why you prefer to Adapt rather than Mixin?

several reasons. One is that multiple inheritance is harder than single
inheritance (not a LOT harder, but enough harder). The second is that
this is the first half of a two-step change. The second step is to use
the same infrastructure to allow running all of the tests across the
board to use one of the servers in the permutations list. I.e. run all
the tests with sftp as the access mechanism for branch & repository, or
a memorytransport or .... Thirdly there is less dead chicken code here -
we have one set of code to setup a style of server, one to test a
transport class, server pair, and one to tie them together. Before we
had the server setup and the test logic conflated, which lead to IMO
nasty hierarchies as we can see elsewhere where we have mixins:
TestMixin
->ExtendedTestMixin
  ->ConcreteTest(TestCase, ExtendedTestMixin, OtherTests)

Ewww!

There there is the general rule of thumb in OO code, that if two things
are not a strict extension of one another, composition is a better
approach than derivation. That is - The logic for running a transport
server, and that for testing a transport server are separate, we should
not conflate them, which is what our deriving from
(TestCaseWithSFTPServer, TransportMixin) idiom was doing. Transport
tests are not an extension of servers, nor are servers an extension of
transport tests.

Oh and most importantly, for tests where we need to change global state
before and after the test - i.e. to override the default format of a
branch, an adapter is an ideal way to allow us to do that in various
permutations, reliably, and mixing in to get that facility will not work
nearly as well, as you'll be mixing in essentially conflicting things. I
think for programmer sanity, trying to have only one idiom for a style
of thing is good - and this transport proof of concept is the where I'd
like the idiom to change to so that these up coming format provider
tests are really easy to manage.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060104/d6a1dfb4/attachment.pgp