bzrlib.tests.commands uses some convoluted test support

[Robert Collins]

For vincent ;)
 affects bzr
 status confirmed
 importance medium
 done

20:24 < lifeless> vila: do you recall what bzrlib/tests/commands/ is for
20:24 < lifeless> is that == the cli package I proposed?
20:24 < lifeless> vila: it doesn't really seem to be, to me?
20:24 < vila> not cli, quite the complement in fact
20:25 < vila> ..bad word :-/
20:26 < lifeless> its not clear to me how the tests there differ from
other ones in blackbox, other than that they create the command object
directly.
20:27 < lifeless> separately, InstrumentedTransport - that would be
better as a decorator I think.
20:27 < lifeless> HookedTransportDecorator, decorate any transport you
want?
20:27 < vila> wait a minute
20:28 < vila> paging context in
20:28 < lifeless> I ask, because the convolutions it goes through look
to be precisely what chroot+ readonly+ etc all do already
20:29 < lifeless> and if it was setup like that it wouldn't be failing
with isolation errors
20:29 < lifeless> BzrError: Attempt to escape test isolation:
'hooked://foo@localhost:37295/tmp/testbzr-VVo9LS.tmp/bzrlib.tests.commands.test_branch.TestBranch.test_branch_local_remote/work/'
                  ['/tmp/testbzr-VVo9LS.tmp/',
'file:///tmp/testbzr-VVo9LS.tmp/',
'/tmp/testbzr-VVo9LS.tmp/bzrlib.tests.commands.test_branch.TestBranch.test_branch_local_remote/',

'file:///tmp/testbzr-VVo9LS.tmp/bzrlib.tests.commands.test_branch.TestBranch.test_branch_local_remote/', 'file:///tmp/testbzr-VVo9LS.tmp/', 'sftp://foo@localhost:37295/tmp/testbzr-VVo9LS.tmp/']
20:30 < vila> So commands/__init__ says: Test the internal behaviour of
the commands (the blackbox tests are intended to
20:30 < vila> test the usage of the commands).
20:30 < lifeless> well, blackbox tests internal behaviour for something
like 90% of the tests in blackbox today ;)
20:31 < lifeless> I'm not objecting to the commands test package
20:31 < lifeless> just it was a bit of a spike you did,
20:31 < lifeless> and its been left fallow since
20:31 < vila> yeah, but nobody ever understood why it was named like
that and even I am not sure :-/
20:31 < lifeless> The hooking stuff is spectacularly ugly though, and if
I can get from you what you needed to achieve I'll do a separate branch
to clean it up
20:32 < vila> pff, it's quite old and I seem to remember that I needed
to do that to minimize changes at that time
20:33 < lifeless> vila: please think a bit about this
20:33 < lifeless> from what I can see you wanted to hook into some
behaviour
20:33 < vila> may be it was to work around a bug fixed since like
redirections bringing back pycurl transports instead or urllib ones
20:34 < lifeless> you created a TransportHooks
20:34 < vila> so I had to keep a tighter control than what decorators
provided
20:34 < lifeless> which doesn't work for all transports
20:34 -!- rojanu [i=0d14890c@gateway/web/freenode/x-akdzkgmrpxtjdkun]
has quit []
20:34 < lifeless> vila: I propose the following:
20:35 < lifeless> - we make that transport hooks official and public
for all transports.
20:35 < vila> It's intended to be used for ConnectedTransport only
20:35 < lifeless> non ConnectedTransports will simply never fire
20:35 < lifeless> - we change the tests to just use SFTPTransport or
FTPTransport directly
20:36 < vila> That *sounds* good (i.e. that seem to address the initial
aims whatever they were, while being cleaner)
20:37 < lifeless> and finally, I'd be strongly inclined to consider
these tests acceptance tests
20:37 < vila> Looking at the code also seem to imply I wasn't very good
at parameterizing the tests at that time
20:37 < lifeless> if you wanted to do this overnight, I wouldn't
complain :)
20:38 < vila> hehe
20:39 < vila> lifeless: don't count too much on it especially today, I
need to finish early due to some light medical operation
20:40 < lifeless> vila: ok
20:40 < vila> lifeless: and that's really not today, tomorrow on the
other hand... :-D SO maybe file a bug with the IRC discussion