maas-test

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

configure-abstract
Merge into trunk

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2013-11-29:

I'm sorry that feelings are flaring up about this... Yes, of course I do see that spaghettification from inappropriate mixing of program options and test parameters is bad. I'm sure we all do. I just don't see it as worse, in the near future for this limited short-term project, than the cost of detailed gatekeeping.

In other words, I think this is a case where short-sightedness is appropriate for the situation. As things change, separating program options from test parameters may become more useful. But you've got to give the patterns a chance to emerge before you set them in stone. An option like --daemon does not make a pattern, especially if you're worried that actually it may still prove relevant to the test cases. Do we have a good reason to believe that review will not be enough to stop the spaghettification?

We keep mis-understanding each other about this. That in itself worries me. This is of course not what I meant:

> Come on, this is specious. The abstract properties are provided by a
> package in the standard library, and they're fairly simple anyway. The
> failure modes are part of the design, to fail fast so that we know early
> that there's a programming error.

I'm not saying abstract properties in general have to be consistent with our option definitions. Only the abstract properties we actually define for this. It has nothing to do with the standard library.

I would love to have early catching of mistakes, but the only kind of mistake that I see being caught early here is inconsistency between the abstract property definitions and the initialisations in ConfiguredTestMAAS. But it's a mistake which the existing design doesn't make possible in the first place.

You argue that testing all uses of the args object would be costly. My view is that the real cost is in testing our code at all, which we should have been doing anyway. The tests can just say "make me an args object," with perhaps some customised fields — just like we do in our customary test factories. I imagine you had much the same in mind for testing with the abstract properties.

I'm sorry that feelings are flaring up about this...  Yes, of course I do see that spaghettification from inappropriate mixing of program options and test parameters is bad.  I'm sure we all do.  I just don't see it as worse, in the near future for this limited short-term project, than the cost of detailed gatekeeping.

In other words, I think this is a case where short-sightedness is appropriate for the situation.  As things change, separating program options from test parameters may become more useful.  But you've got to give the patterns a chance to emerge before you set them in stone.  An option like --daemon does not make a pattern, especially if you're worried that actually it may still prove relevant to the test cases.  Do we have a good reason to believe that review will not be enough to stop the spaghettification?

We keep mis-understanding each other about this.  That in itself worries me.  This is of course not what I meant:

I'm not saying abstract properties in general have to be consistent with our option definitions.  Only the abstract properties we actually define for this.  It has nothing to do with the standard library.

I would love to have early catching of mistakes, but the only kind of mistake that I see being caught early here is inconsistency between the abstract property definitions and the initialisations in ConfiguredTestMAAS.  But it's a mistake which the existing design doesn't make possible in the first place.

You argue that testing all uses of the args object would be costly.  My view is that the real cost is in testing our code at all, which we should have been doing anyway.  The tests can just say "make me an args object," with perhaps some customised fields — just like we do in our customary test factories.  I imagine you had much the same in mind for testing with the abstract properties.

« Back to merge proposal