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

Sorry for taking so long to get back to you on this. I've given it some more thought.. perhaps we can unblock this.

> > Sure, I don't disagree with that. I think the parametrization isn't done
> right here.
> Not telling me in what way it's not done right, doesn't help me much ;)

Test stuff should be as much as possible isolated to the test side of the code - and not be executed as part of normal operations. I still think this is not a good use of registries, but having this sort of registration in the load_tests code of bzr-svn would be a lot nicer than where it is now.

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

"make check" in bzr-svn simply runs "bzr selftest Svn svn Subversion" and that works fine and includes all relevant tests.

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

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

Cheers,

Jelmer