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

<snip/>

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

Right. I did my best to reduce the test related code invasion to a
minimum but I agree this still has a bad smell. I thought this minimum
was small enough to be ignored but I understand you can disagree ;)

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

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.

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

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

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

That's your call and I respect that.

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

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

While you've got the last word for bzr-svn, I don't intend to go back to
multiply_* helpers in bzrlib as they are far harder to maintain than the
'scenarios' attributes (decoupled from the class, not providing access to
the test instances if needed, the list goes on), while not providing
more flexibility. (again, see test_http history for a real life
example).

And getting multiply_ helpers is half of the solution, test classes are
still needed, that creates a lot of unneeded churn for... something I
still don't get ;)