Code review comment for lp:~cprov/uci-engine/charm-tests

Vila,

Thanks for your points, let's discuss them specifically.

> There are many issues with the MP, the main ones are:
> - it breaks --list

--list was probably broken because the problem with running run-test w/o filters, it's fixed now.

> - it uses imp.load_module() whose doc says: "This function does more than
> importing the module: if the module was already imported, it is equivalent to
> a reload()!" (The question mark is theirs). See
> https://docs.python.org/2/library/functions.html?highlight=reload#reload and
> the endless list of issues associated with that function.
>
> There is no way we can trust reload(), it's meant to be used interactively
> with a full understanding of the fallouts.
>
> The root issue that you're pointing out here is that we're using hooks.py to
> mean different things, python doesn't allow that. So that's what need to be
> changed.
>
> I had a look at it and the second similar issue is that we *also* do 'import
> charmhelpers' with various definitions across charms and also try to convince
> python that they are all the same. This one is slightly simpler to fix: as a
> project, we should use the *same* version in all charms (we do use the same
> revno but build a different python package via different charm-helpers.yaml
> files).

Charms are designed to be standalone projects, they just happen to be in the same tree because it's convenient to us, the same goes for running their tests inside a single *super-project* test suite, it's *just* way more convenient than spawning isolated `make check`s and collect results. That said, in order to keep the wanted benefits, UCI-E has to compromise.

We are all aware of the risks involved in reloading modules, but if it is what it takes to assure charm tests (and only them) are run as part of our test suite, so be it. And by all means, I agree with you on keeping this hack as isolated and instrumented as possible, so we will notice if it is behaving badly, if it ever does.

> I think it's time to stop using bigger hammers that do more harm than good,
> step back to the original decision to rely on 'import hooks' and fix that.

It sounds good, but `import hooks` & `import charmhelpers` are not going to change, they make all sense at the charm-level; and we have to ensure we run charm tests in tarmac, today, not tomorrow.

Moreover, It's not a *big hammer*, since it's well isolated (custom loaders and suite are only used for charm tests) and the behaviour can be entirely overridden by tweaking 2 lines, if we find real, not hypothetical, evidences that it's not doing what it is supposed to do.