Code review comment for lp:~bzr/bzr-svn/stacks

> > I still think this is not a good use of registries,
> That's the part I don't get, so, let's forget this proposal for a
> second. Can you point where their usage causes problems in
> bzrlib/test_config ?
Registries are basically lazy dictionaries. One of their key points is that it is possible to retrieve a single item by key without loading any of the rest of the registry. The way they're being used here is more as a way of delayed lists. The key is basically ignored.

The current tests also require a fair amount of setup - and that's currently done with a function that gets passed the TestCase class. This then makes life hard for bzr-svn setup, because bzr-svn has its own setup code. With e.g. a mixin this wouldn't be as problematic.

Code like:

if getattr(test, 'backing_branch', None) is None:

is too magical imho.

> I ended up introducing them because it made it easier for me to add new
> stores, stacks and so on, and get them tested instantly for the existing
> features so I could focus on the newly introduced ones only.
>
> > but having this sort of registration in the load_tests code of
> > bzr-svn would be a lot nicer than where it is now.
>
> Then, with the caveat I mentioned below (and you seem to be ok with),
> that seems do be the best way to go (see my comments below on
> alternatives).
>
> That will just mean moving the registrations in load_tests in which case
> *calling* load_tests is the only thing that needs to happen to have the
> svn-specific tests run.
I'm still not thrilled about that, but I could live with it.

The current branch is also reasonable enough for me, so I'm happy to land that.

> >> One issue I can think of is about how these tests are named, because it
> has
> >> an impact on how -s will work with them:
> >>
> >> - if they are named as bt.test_config, -s bt.test_config includes them
> >> (while respecting BZR_PLUGIN_PATH and friends)
> >>
> >> - if they are named as bp.svn, -s bp.svn includes them but -s
> bt.test_config
> >> doesn't anymore
> >>
> >> In fact, they cannot be part of -s bt.test_config without some help
> from
> >> svn/__init__.py otherwise even the load_tests() there is ignored so no
> >> tests/ file has a way to add them.
>
> > This is already the case for a lot of other things, so I don't
> > think it would be a problem if -s bt.test_config didn't execute
> > them.
>
> Well, for the bzrlib/config.py maintainer, having the plugin tests
> executed while fixing bugs or adding features is way simpler than to
> have to execute all the plugin tests (even restricted to the config
> related ones).
I don't really think the bzrlib/config.py maintainer should worry about that - the main point is that the API stays stable, plugin authors should worry about sticking to that API (and the tests can help with that). Sure, we should discuss design implications of the config.py design, but I don't think you should have to worry about bzr-svn test failures.

> > For example, compare "bzr selftest -s bt.per_interbranch" and "bzr
> > selftest bzrlib.tests.per_interbranch". The latter also includes
> > all plugins, the former does not.
> Yup, that's what I wanted to address. If the plugin is enabled, so are
> the tests.

> We still seem to talk past each other. May be it's a difference
> between bzrlib/config and plugins points of view ?
>
> Is there some plugin specific issue I fail to understand here ?
> Compatibility related may be ?

I'm not quite following either. My question was what the problem was of doing the registration in load_tests() ? It seems to have some impact on -s, but that impact is present for other tests as well (it's a limitation of -s), so I don't see why that would be such a big deal.

> >> The parametrization is about deciding which tests should be run with
> which
> >> scenario.
>
> >> [...]
>
> >> Asking for more control about the test classes requires knowledege
> about
> >> them.
> >>
> >> I don't know where you stand on that.
> >>
> >> You certainly have a great deal of experience with plugins needing to
> respect
> >> an interface, parametrized or not.
> >>
> >> My two proposals for that seems to be at the two extremes, do you have
> a
> >> preferred sweet spot between them ?
>
> > Having to know the test classes in bzr-svn is fine with me; there
> > is always going to be some churn in the interfaces bzr-svn is
> > involved in, and that's easy enough to keep up with.
>
> Then would you prefer to create specific test classes and just
> duplicate/adapt the 'scenarios=' initializations ?
>
> But why would you prefer that ?

It doesn't require test-specific code in bzr-svn's __init__.py, and it gives me more control over what tests are run.

> >> I've started defining:
> >>
> >> def multiply_store_tests(tests, result, store_builder):
> >> scenarios = [('subversion', {'get_store': store_builder,
> >> 'store_id': 'subversion'})]
> >> return tests.multiply_tests(tests, scenarios, tests)
> >>
> >>
> >> def multiply_matcher_tests(tests, store_builder, matcher_class):
> >> scenarios = [('subversion', {'get_store': store_builder,
> >> 'store_id': 'subversion',
> >> 'matcher': matcher_class})]
> >> return tests.multiply_tests(tests, scenarios, tests)
> >>
> >>
> >> def multiply_stack_tests(stack_builder, store_builder):
> >> scenarios = [('subversion', {'get_store': store_builder,
> >> 'get_stack': stack_builder})]
> >> return tests.multiply_tests(tests, scenarios, tests)
> >>
> >> in bzr-svn but that still need to know:
>
> > Wouldn't it be possible to do it the other way around? Making
> > bzrlib.tests.test_config export a bunch of multiply methods that
> > bzr-svn (and others) can call from their load_tests function to
> > get a set of tests, passing in get_store, get_stack and other
> > parameters?
>
> That would go against the 'scenarios=' approach which is the preferred
> way to do the test multiplications. And frankly, having practiced both,
> the 'scenarios=' one is really better suited.
I don't think it would go against the scenarios approach. There would still be multiple scenarios, the location in which the scenarios are applied is just different.

Anyway, I can live with the current patch and I'll see about merging it.

Cheers,

Jelmer