[RFC][PATCH]

[John Arbash Meinel]

Robert Collins wrote:
> 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).
> 

Well, it is nice to have optional dependencies that don't break things
unless you actually use them. Also, it is tricky to inherit properly.
Because while you are importing BASE, you really shouldn't import
CHILDREN, but all of the children need to be imported eventually, where
do we do that? Originally I was doing it in run_bzr(), though it
probably makes more sense to do it in bzrlib/__init__.py

Also, I really miss late loading of code. In another project, I'm using
the InsightToolkit, which takes about 5s to load on a fast machine.
Which means that everytime I run the test suite, I have to wait 5s for
it to load all of the code, 90% of which I'm not using yet.

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

I must have miss read where things are. Long patches and all.

I don't know whether it is better to have bzrlib.transport.tests.* or
bzrlib.tests.transport.* I typically consider tests/ to be an alternate
root, which mirrors the bzrlib/ directory, slapping test_ in front of
the module names.

Either way is fine with me.

> 
>>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 specific exceptions are WebserverNotAvailable, and BadWebserverPath.

BadWebserverPath should also not be inheriting from ValueError. But as
you say, this might have just been in tests/test_http.py, so we didn't
see it before.

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

Sure. And I agree that it is a good way to do it.

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

Well, I really like the concept of running all of the test suite against
different transports. I've noticed that I've started running into the
permutation problem. Specifically, with bound branches, it is important
to test against sftp, since that is the expected target, and they do
behave slightly differently.

As far as multiple inheritance, I tend to think Mixins don't have to be
too ugly, since they should be designed as add-in interfaces, not full
fledged independent classes.

But I'm okay with the adapter.

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

As long as we switch the adapter so that it is possible for plugins to
add their own tests, I'm happy enough with the system.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 256 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060105/22b49cb6/attachment.pgp