Code review comment for lp:~allenap/maas-test/configure-abstract

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

The mere fact that we can have a misunderstanding about which of the definitions of the same option I'm talking about speaks volumes. That complexity is a steep price to pay when there is no particularly profound divide between program options and test parameters which we need to enforce or document.

And aside from that divide between options and parameters, what does it protect us from? If we stick to a single args object, there is really just one mistake you can make with a test parameter: inconsistency between its use and its definition in the args parser. Simple, lightweight, flexible, obvious.

With the "wide interface" to parameters which you're creating here, there's a lot more to go wrong. Now it's the argparser, abstract properties, initialisations, and uses that all need to be consistent. There are now several failure modes for different (combinations of) inconsistencies. How many of those are improvements? You're increasing the odds of failing early, but at the cost of increasing the odds of failing at all.

I suppose the real problem is that it can be slow and difficult to discover mistakes in accessing those options from the main code. A current-day linter won't tell us when we're going to access undefined parameters. The abstract properties won't help with that either; they only provide a redundancy check on the copying of the parameters. But this is properly a matter of test coverage. Isn't the logical solution to address that, rather than add red tape in the hope that it will trip up mistakes? For example, I see no test to cover the instantiation of ConfiguredTestMAAS that would report any missing initialisations. If we could just pass realistic args objects to unit tests, mistakes in parameter access should be easy to beat.

« Back to merge proposal