location of non-test test code

[Martin Pool]

On 17 Jul 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:

> In my wild imaginings, we would have a way to automatically determine
> which test packages need to be run, so that it defaults to only running
> the set that is expected to be effected. With an option to run
> everything. Rather than our current "run everything, unless I specify a
> specific subset".

To go into that a bit more, the possibilties include:

 - Exclude some long-running tests from the default set, perhaps
   the transport tests.  But on the other hand the pressure to keep
   everything reasonably fast is good - the sftp tests could probably be
   sped up.

 - Have some control on ordering, either automatically or manually.

 - Magically determine which tests relate to code that's been changed --
   perhaps by looking at a call graph or import statements, etc..

> By the way, what do people think about making our current regex match be
> case insensitive? I've taken to doing:
> 
> bzr selftest -v "(?i)http"
> 
> Which sets the insensitive flag in the regex engine, but it isn't a very
> nice thing to always have to type.

That sounds reasonable.  Is it more for ease of typing, or so that you
can match both classes and modules?

> >>> So, heres a two part proposal.
> >>>
> >>> Part 1:
> >>>  Lets move all the non-test test code currently in bzrlib.tests to
> >>> bzrlib.selftest. If there is a clear home for it that is outside
> >>> bzrlib.selftest - i.e. if its os lock specific, then I would encourage
> >>> us to put it in lock.py.
> >> I really don't like 'bzrlib.selftest'. Maybe 'bzrlib.test_support'?
> >>
> >> I actually put some helper stuff into the files themselves. But after
> >> discussing with Martin, I really don't think it should go there.

As I said to John in a recent review, I'd rather not have code used only
in testing present in e.g. lock.py, for a few reasons:

 - api users might depend on it when we didn't intend to commit to it

 - it makes it harder to judge the real complexity or size of the module

 - it can't help load time

 - it avoids people being distracted by "what is this here for?" when 
   reading the code

> > Its a lot more than support logic. Theres the root TestCase classes,
> > theres the test runner, the test result object, and more. I think
> > test_support is misleading by incompleteness.
> 
> Those all sound like support code for running tests. but if you have a
> better idea...

I'd classify it as 

bzrlib.tests - actual test code

bzrlib.testsupport - code that's not actually a test, but used only
    while testing (e.g.  instrumented classes).  If they only apply to one
    test module, they should probably just be in there.

bzrlib.selftest - root test classes - TestCase, etc

bzrlib.selftest - command-line selftest stuff, specific to our command line and output  
   format.  If anything deserveds bzrlib.selftest it's probably this.

I'm not set on those names.

On the whole I like just buttedtogether words rather than
underscore_joined in filenames, with 'test_' as a special prefix.

Separately I'd suggest to move all the transport servers into
bzrlib.server.*, parallel to the transport clients.

It would also be nice to just scan the library for tests, rather than
making them be listed in tests/__init__.  This must have been written
many times in Python, so perhaps we can lift it from somewhere though I
suppose it's pretty trivial.

I've noticed that reviewing patches is harder when there are many
classes in the same file, as Python doesn't give you much context to
which class was modified.

> >> It really is nicer to keep the code itself simple, and just have a
> >> logical place to look for the associated tests. Right now it is hard to
> >> grep through Transports to look for a specific function/variable,
> >> because you tend to hit on the Transport.Server, which is only used in
> >> testing.
> >>
> >> I really think we're combining too much stuff into a single file as it
> >> is. Especially with all the implementations mixed into the same file as
> >> the interface, when you are trying to find a specific function, you have
> >> to search for the name, and then page up to see if you are in the
> >> implementation you *think* you are in.

I agree with both of those.

> >>> Part 2:
> >>>  Lets split out the test suite for better locality of reference.
> >>>
> >>> I'm much more interested in Part 1 than Part 2, so if Part 2 is
> >>> problematic, just ignore it ;).

Let's leave it for now.

> You seem to be suggesting to change:
>   bzrlib/foo.py
> into
>   bzrlib/foo/__init__.py
> and then test it with
>   bzrlib/foo/tests.py
> 
> Which is okay, but right now we have 77 source files in bzrlib/ (and 83
> .py files in tests/, interesting...).
> 
> Anyway, I don't think we want 77 directories. And I don't *think* we
> want to use:
> 
>   bzrlib/foo.py
>   bzrlib/test_foo.py
> 
> Because that makes it too hard to find the real source code.

I think robert was talking of 

  bzrlib/foo/foofrob.py

being tested by

  bzrlib/foo/tests/test_foofrob.py

But it doesn't really appeal to me.

> I honestly can't think of a better layout than our current bzrlib/tests/
> directory, which mirrors the source tree. As long as we are consistent,
> the tests are reasonably easy to find, but tucked away so they don't get
> in the way of looking at the source.

I like it too.  The only other thing for me would be parameterized tests
vs base-class tests vs specific implementation tests - sometimes I find
it hard to know whether to look in test_foo.py or
test_foo_implementations or foo/*

-- 
Martin