Default hook state and the test suite

[Martin Pool]

On 19 October 2010 11:03, Andrew Bennetts <andrew.bennetts at canonical.com> wrote:
> John Arbash Meinel wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> I'm worried about a whole in our testing setup. Specifically, we have at
> [...]
>> For now, I just moved the "register this hook" override back into all of
>> the "bzr status" blackbox tests. But it feels like we should have a more
>> generic solution, because this is easily missed.
>
> This probably isn't the generic solution you are looking for, but it is
> what we have today...
>
> See the last two lines of TestCase._clear_hooks:
>
>        # this hook should always be installed
>        request._install_hook()
>
> ('request' here is bzrlib.smart.request)
>
> I think we should arrange for “always installed” hooks to be installed
> after TestCase._clear_hooks has run.  It would be nice to make this more
> automatic, so that future always installed hooks are automatically taken
> into account here.
>
> Part of me wonders if always-installed hooks are a bit of a code smell,
> though...

So the problem seems to be that we're conflating "hook" as meaning an
extension point that plugin or external can get in to, with a point
which only external code will use.

In general I think it's quite nice to have bzrlib itself use the same
mechanism to talk to earlier layers as other code can use to plug in
to bzr, but it does give us this problem that we shouldn't assume
'nothing installed' means 'vanilla bzr'.

The same kind of problem also crops up with registries and other types
of overriding.

I think the test isolation code probably should default to "restore
the hooks to the state they were in before the test began" rather than
clearing them?  That would probably at least make this pass cleanly
when bzr is run with no plugins.

Not quite the same, but related: getting a clean test suite in the
presence of plugins is a bit complicated.  If they just add new
commands/formats/whatever, it's easy.  If they want to modify existing
commands, like for instance adding a section to 'status' -- which is a
reasonable thing to do -- its a bit harder to test.  How are the
builtin tests for status supposed to know what modifications to it
might be done by the hooks?  There seem to be a few options:

0- write some tests that specifically disable all the hooks, and check
the output there: this should behave consistently whether you have
plugins installed or not; adding such a fixture would make it easy to
get existing failing tests going again
1- complementary to 0, let the plugin write tests that enable its
specific hook (and no others) and check the output

Those two together would be ok, except that there might be some amount
of duplication, and there might not be any checks for the interaction
of multiple hooks.

2- write some tests that leave hooks in whatever state the user has
them, and make minimal assumptions about the output, eg "status hooks
might add output but not delete the file list, and it at least
shouldn't crash."

Perhaps 0 and 1 could be unified by allowing plugins to inject new
scenarios where they control which hooks are active and what output is
expected.

-- 
Martin