bzrlib.tests.commands uses some convoluted test support
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Bazaar |
Confirmed
|
Medium
|
Unassigned |
Bug Description
For vincent ;)
affects bzr
status confirmed
importance medium
done
20:24 < lifeless> vila: do you recall what bzrlib/
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, InstrumentedTra
better as a decorator I think.
20:27 < lifeless> HookedTransport
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:
'file:/
'/tmp/testbzr-
'file:/
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@
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
tags: | added: check-for-breezy |