Merge lp:~allenap/maas-test/configure-abstract into lp:maas-test

Proposed by Gavin Panella
Status: Rejected
Rejected by: Graham Binns
Proposed branch: lp:~allenap/maas-test/configure-abstract
Merge into: lp:maas-test
Prerequisite: lp:~allenap/maas-test/refactor-prep
Diff against target: 576 lines (+226/-136)
7 files modified
docs/man/maas-test.8.rst (+47/-17)
maastest/cases.py (+39/-16)
maastest/main.py (+20/-10)
maastest/parser.py (+75/-74)
maastest/utils.py (+3/-0)
man/maas-test.8 (+42/-17)
packages.txt (+0/-2)
To merge this branch: bzr merge lp:~allenap/maas-test/configure-abstract
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Needs Fixing
Review via email: mp+196453@code.launchpad.net

Commit message

Use abstract properties to make the test cases configuration clearer.

Description of the change

The TestMAAS.configure() call had become vestigial with the passing of the raw arguments object. I've removed it, and replaced the configurable properties with abstract properties. This way we can configure the subclass in a natural way, without extra function-calling boilerplate, but we also document the meaning of the configuration, and we will encounter errors if we forget to provide the necessary configuration.

I've also removed the prepare_parser() call as an unnecessary indirection. Argument parsers are not mutated in practice, so we need only prepare one.

I've also changed --interactive-enlistment to --interactive. Firstly it was a lot to type. More importantly it's relevant when using the daemon mode, coming in a later branch. The daemon mode will have interactive elements (it's more of a server than a "proper" daemon really). The daemon mode may be delayed - it has been de-prioritised - but I'd like to switch to this spelling of the flag now, in preparation.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

There are definitely good things here: avoiding the weird dynamic setting of a class variable, renaming --interactive-enlistment to --interactive, and the new help string for the --interactive option.

I also like having documentation for the arguments, but the abstract properties are a tail wagging its dog. Instead of defining an option in parser.py and the manpage, we'd have to define them in parser.py, cases.py, main.py and the manpage. K.I.S.S.! Forget just the abstract property and you violate the design but nothing actually happens; forget just the bit in ConfiguredTestMAAS and you get a traceback to remind you to define the abstract property; forget both and you violate the design but get a different traceback without the reminder. It's bad enough that manpages are generally like that, but in the code it's easy to avoid.

The option definitions on the arguments parser are effectively documentation in themselves, which if anything just needs to be brought to the fore. The args object encapsulates them in a way that makes them easy to identify as arguments. That's actually a pretty good setup — light, flexible, and clear. All in one place, so consistency comes naturally without enforcement. If you want to isolate and document the options that are relevant to test scenarios, I'd suggest sticking very close to the existing "args" pattern. Once you have cross-module inheritance relationships separating a field's specification, use, and definition, you're no longer any better off than with simple cross-module delegation.

Nor do I agree with inlining prepare_parser() into module-level code. A declarative list of argparse.Action as a module-level variable would make sense — though redundant if abstract properties actually serve their purpose of documenting those options! — but I fail to see any advantage in making these method calls side effects of importing the module. Without reliable policing of __all__, module-level code at zero indentation just gives me lots more items to read that may hide effective interface points for the module. Doing it this way just makes it awkward to test properties like “the parser accepts this command line specified by the original product design,” or “this particular mistake on the command line leads to this user-friendly error message.”

One other thing: main(), in main.py, has imports in unexpected places. Explain why those are necessary, or move them to the expected places.

review: Needs Fixing
59. By Gavin Panella

Merged refactor-prep into configure-abstract, resolving conflicts.

60. By Gavin Panella

Snapshot changed man page.

61. By Gavin Panella

Add argument group for image selection.

62. By Gavin Panella

Improve metavars.

63. By Gavin Panella

Not using testresources right now.

64. By Gavin Panella

Merged refactor-prep into configure-abstract, resolving conflicts.

65. By Gavin Panella

Merged refactor-prep into configure-abstract, resolving conflicts.

66. By Gavin Panella

Switch more options to using abstract properties.

67. By Gavin Panella

Move proxy options into their own argument group.

68. By Gavin Panella

Add some TODOs.

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (6.1 KiB)

Thanks for the review. I disagree with most of it! But let's see where
we can go from here, and if I can win you over :)

> There are definitely good things here: avoiding the weird dynamic setting of a
> class variable, renaming --interactive-enlistment to --interactive, and the
> new help string for the --interactive option.
>
> I also like having documentation for the arguments, but the abstract
> properties are a tail wagging its dog.  Instead of defining an option in
> parser.py and the manpage, we'd have to define them in parser.py, cases.py,
> main.py and the manpage.  K.I.S.S.!  Forget just the abstract property and you
> violate the design but nothing actually happens;

Except that your tests crash when they refer to
{self,cls}.missing_attribute.

>                                                  forget just the bit in
> ConfiguredTestMAAS and you get a traceback to remind you to define the
> abstract property;

Which happens early; no need to wait for a VM to fire up, etc.

>                    forget both and you violate the design but get a different
> traceback without the reminder.

Back to test crashes when they refer to {self,cls}.missing_attribute?

>                                  It's bad enough that manpages are generally
> like that, but in the code it's easy to avoid.

The abstract properties, once defined in TestMAAS (or a subclass), mean
that we document TestMAAS's requirements, and avert wasteful runs to
determine that we forgot to add a command-line argument to populate the
args object that we refer to in the last-but-one test case, for example.
Locality: those properties define what we need closer to where they're
needed.

>
> The option definitions on the arguments parser are effectively documentation
> in themselves, which if anything just needs to be brought to the fore.  The
> args object encapsulates them in a way that makes them easy to identify as
> arguments.  That's actually a pretty good setup — light, flexible, and clear.
> All in one place, so consistency comes naturally without enforcement.

Enforcement is useful here, and it's not clear as it is. The args object
is a pin-cushion, and does nothing more to identify arguments as such
than the abstract properties.

>                                                                       If you
> want to isolate and document the options that are relevant to test scenarios,
> I'd suggest sticking very close to the existing "args" pattern.  Once you have
> cross-module inheritance relationships separating a field's specification,
> use, and definition, you're no longer any better off than with simple cross-
> module delegation.

The abstract properties mean that we break the cross-module dependency
between maastest.cases and maastest.parser; the same names may be used
for convenience and consistency, but TestMAAS is no longer bound
directly to the command-line.

We also add a formal contract with maastest.main, in that it cannot
instantiate TestMAAS or any of its subclasses without providing values
for the missing properties.

>
> Nor do I agree with inlining prepare_parser() into module-level code.  A
> declarative list of argparse.Action as a module-level...

Read more...

69. By Gavin Panella

Update the man page from the --help output.

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

Actually I think we really just disagree about one important thing: whether forgetting to define an abstract property for one of the options will produce an appropriate error. If it does, I generally agree with your considerations. But my position is: no it doesn't, because any references the methods make to these properties are bound when they execute, not when they are defined. And by the time they execute, these properties have been set. No error, no enforcement.

But we're in luck: we can just try it! Which is what I'm doing right now. I removed the abstractproperty definitions, all of them for good measure, though I kept the base class abstract. I hope you'll agree that's a good, extreme example of forgetting to define some. Tests pass. Running maas-test seems to work just fine so far. At the time of writing it's trying to import boot images, at which point you'll agree it must have noticed if there were a problem reading the proxy, series, and architecture options if nothing else.

70. By Gavin Panella

Merge trunk, resolving conflicts.

71. By Gavin Panella

Update man page.

Revision history for this message
Gavin Panella (allenap) wrote :

> Actually I think we really just disagree about one important thing: whether
> forgetting to define an abstract property for one of the options will produce
> an appropriate error. If it does, I generally agree with your considerations.
> But my position is: no it doesn't, because any references the methods make to
> these properties are bound when they execute, not when they are defined. And
> by the time they execute, these properties have been set. No error, no
> enforcement.
>
> But we're in luck: we can just try it! Which is what I'm doing right now. I
> removed the abstractproperty definitions, all of them for good measure, though
> I kept the base class abstract. I hope you'll agree that's a good, extreme
> example of forgetting to define some. Tests pass. Running maas-test seems to
> work just fine so far. At the time of writing it's trying to import boot
> images, at which point you'll agree it must have noticed if there were a
> problem reading the proxy, series, and architecture options if nothing else.

I didn't argue that forgetting to define an abstract property would
cause an error. I argued that forgetting to set that property in a
subclass would cause an error, which it does. Removing the abstract
properties from TestMAAS doesn't demonstrate anything particularly
useful.

The purpose of the abstract properties is to define TestMAAS's
requirements close to their use. Try removing one of the command-line
options, or forgetting to set one of the properties on
ConfiguredTestMAAS, then you'll get an early warning. The args pattern
displays neither of those properties; you'll only ever get "late"
breakage.

72. By Gavin Panella

Switch over to disable_cache.

73. By Gavin Panella

Merge trunk, resolving lots of conflicts.

74. By Gavin Panella

Fix lint.

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

Now you have me utterly confused. You quoted what I said:

> Forget just the abstract property and you
> violate the design but nothing actually happens;

And your response to that was:

> Except that your tests crash when they refer to
> {self,cls}.missing_attribute.

And I'm telling you that nothing crashes, as long as you don't _also_ forget to set the property in ConfiguredTestMAAS. If you forget the latter, it crashes either way. But you can forget just the abstract property and everything still works. And that makes the enforcement rather weak!

Revision history for this message
Gavin Panella (allenap) wrote :

> Now you have me utterly confused. You quoted what I said:
>
> > Forget just the abstract property and you
> > violate the design but nothing actually happens;
>
> And your response to that was:
>
> > Except that your tests crash when they refer to
> > {self,cls}.missing_attribute.
>
> And I'm telling you that nothing crashes, as long as you don't _also_ forget
> to set the property in ConfiguredTestMAAS. If you forget the latter, it
> crashes either way. But you can forget just the abstract property and
> everything still works. And that makes the enforcement rather weak!

Okay, I misunderstood your point there; I had assumed you meant not
providing a setting for the abstract property in the subclass. If you
forget to define an abstract property you're hardly better off than you
are now.

However, the point of the abstract properties is that they're nearby the
code that needs them. Right now you must marry up all references to
cls.args.* or self.args.* in maastest.cases with the command-line
options defined in maastest.parser.

It is an improvement, and that's usually our measure for what may land.

75. By Gavin Panella

Merge trunk, resolving conflicts.

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.

Revision history for this message
Raphaël Badin (rvb) wrote :

I had a quick look at the code and read the ragging discussion.

My two cents: I tend to think Jeroen's point is justified: adding these abstract properties simply adds another thing you have to modify each time you want to add a new command which translates into a property. The argument that adding these properties would speed up the debugging when someone makes a mistake is only half valid: if, for instance, one has two mutually exclusive properties, only proper testing can detect this kind of problem.

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (4.0 KiB)

What I see as a fairly obvious improvement has been voted down because
it's too convenient to tightly couple the command-line to TestMAAS.

One of the dependent branches of this already breaks that: it adds a
--daemon option which is nothing to do with TestMAAS; it's an option to
select a different test suite class, among other things.

Now two things would be coupled closely to the command-line, and
TestMAAS will be free to use args.daemon if it so chooses, and perhaps
someone will make it do that one day, thus starting the process of
spaghettification.

Do you not see that this is bad?

> The mere fact that we can have a misunderstanding about which of the
> definitions of the same option I'm talking about speaks volumes.

I just misunderstood what you wrote, not the concepts involved.

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

That's short-sighted.

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

Simple: Briefly, while there's still a 1:1 correspondence between the
command-line and TestMAAS.

Lightweight: Indeed, but I've not proposed anything heavyweight.

Flexible: It's so flexible you can point straight ahead and still shoot
your toes off.

Obvious: As in, it's obvious it can be improved?

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

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

As I've said several times, these parameters are defined close to where
they're used. Locality is generally a desirable property. It makes us
less likely to forget or misuse, and helps our future selves and others
understand our intentions.

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

I was able to do this change in a short amount of time. Adding tests to
cover every use of the args object would have taken a lot longe...

Read more...

76. By Gavin Panella

Merge trunk, resolving conflicts.

77. By Gavin Panella

Merge trunk.

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

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.

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (5.5 KiB)

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

I think that the status-quo is worse, but even if I'm wrong, we're not
speculating: this code is done, and it's simply not "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.

We're not setting anything in stone. Decoupling TestMAAS from the
command-line is the opposite.

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

Yes, it's already started: the args object is being passed through to
ConfiguredTestMAAS.configure().

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

A run will fail eventually, true, when it tries to deference a missing
argument. That's our ultimate measure of correctness in this regard.
This proposed change moves some of these possible failures close to the
start of a run. That's an improvement.

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

The tests could also say "make me a testcase", and the abstract
properties would be there to ensure that nothing had been omitted.
That's a small improvement.

Here is the essence of the code that we've been arguing about for a
week:

{{{
=== modified file 'maastest/cases.py'
--- maastest/cases.py   2013-11-28 17:09:31 +0000
+++ maastest/cases.py   2013-12-02 09:44:27 +0000
@@ -42,1 +45,1 @@

class TestMAAS(testto...

Read more...

78. By Gavin Panella

Merge trunk, resolving conflicts.

79. By Gavin Panella

Merge trunk, resolving one conflict.

Revision history for this message
Raphaël Badin (rvb) wrote :

I have the feeling we have reached a stalemate here… I know it's not a great feeling when one has invested time in a branch to see it rejected but, well, it happens. I'm afraid the only solution here is to agree to disagree on this, mark this branch rejected, and move on. Gavin?

Revision history for this message
Graham Binns (gmb) wrote :

I'm going to mark this branch as rejected just to clear it out of the way. Sorry Gavin; it's great work but we've pretty much reached an impasse (and besides, this branch will be bit-rotten by now).

Unmerged revisions

79. By Gavin Panella

Merge trunk, resolving one conflict.

78. By Gavin Panella

Merge trunk, resolving conflicts.

77. By Gavin Panella

Merge trunk.

76. By Gavin Panella

Merge trunk, resolving conflicts.

75. By Gavin Panella

Merge trunk, resolving conflicts.

74. By Gavin Panella

Fix lint.

73. By Gavin Panella

Merge trunk, resolving lots of conflicts.

72. By Gavin Panella

Switch over to disable_cache.

71. By Gavin Panella

Update man page.

70. By Gavin Panella

Merge trunk, resolving conflicts.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'docs/man/maas-test.8.rst'
2--- docs/man/maas-test.8.rst 2013-11-28 11:06:23 +0000
3+++ docs/man/maas-test.8.rst 2013-12-05 16:31:01 +0000
4@@ -50,7 +50,7 @@
5 a route to the internet.
6
7 For the node, that means that both the node's own network interface card (NIC)
8-and its baseboard management controller (BMC) should be on the testing network.
9+and its baseboard management controller (BMC) should be on the testing network.
10 The testing system only needs one NIC on the testing network.
11
12 In addition to being on the testing network, the testing system must also have
13@@ -75,7 +75,8 @@
14 testing network. You will be passing this interface to `maas-test`.
15
16 5. Depending on caching, the test may download and store large amounts of data
17-on the testing system. Make sure you have sufficient disk space and network bandwidth.
18+on the testing system. Make sure you have sufficient disk space and network
19+bandwidth.
20
21 The data that needs downloading and/or caching consists mostly of system images
22 for the virtual machine, and for booting the node. As a rule of thumb, count
23@@ -100,9 +101,42 @@
24 Options
25 =======
26
27+Positional arguments:
28+---------------------
29+
30+interface
31+ The interface on which MAAS will provide DHCP. This is the machine's
32+ interface on which the node under test should be plugged-in.
33+
34+Optional arguments:
35+-------------------
36+
37 -h, --help
38 Show help and exit.
39
40+--interactive
41+ Interactive mode. Instead of powering up the node automatically through
42+ IPMI, prompt the user to turn it on manually. In this mode there is no need
43+ to specify BMC details; the MAAS enlistment process will discover them
44+ automatically.
45+
46+--archive=archive
47+ Optional package repository name. If given, the virtual machine may install
48+ packages from this additional archive as well as from the main Ubuntu
49+ archive. The archive is added to the virtual machine using
50+ 'add-apt-repository'; it may be either a line in the format of apt's
51+ sources.list, or a personal package archive identifier in the form
52+ 'ppa:<user>/<ppa-name>', or a distribution component that should be enabled.
53+ This is typically used to test recent versions of MAAS that are only
54+ available in a PPA such as "ppa:maas-maintainers/dailybuilds".
55+
56+
57+BMC details:
58+------------
59+
60+BMC/IPMI details for performing the first power on of the node under test.
61+Required when --interactive has not been specified.
62+
63 --bmc-mac=MAC
64 MAC address for the node's baseboard management controller. MAAS will
65 control the node's power and boot sequence through this controller.
66@@ -127,21 +161,11 @@
67 Username for IPMI authentication. Use with **--bmc-password**.
68 Not needed in interactive mode.
69
70---interactive
71- Interactive mode. Instead of powering up the node automatically through
72- IPMI, prompt the user to turn it on manually. In this mode there is no need
73- to specify BMC details; the MAAS enlistment process will discover them
74- automatically.
75-
76---archive=archive
77- Optional package repository name. If given, the virtual machine may install
78- packages from this additional archive as well as from the main Ubuntu
79- archive. The archive is added to the virtual machine using
80- 'add-apt-repository'; it may be either a line in the format of apt's
81- sources.list, or a personal package archive identifier in the form
82- 'ppa:<user>/<ppa-name>', or a distribution component that should be enabled.
83- This is typically used to test recent versions of MAAS that are only
84- available in a PPA such as "ppa:maas-maintainers/dailybuilds".
85+
86+MAAS images:
87+------------
88+
89+Series and architecture choices pertaining to the node-under-test.
90
91 --series=codename
92 Code name for the Ubuntu release series that should be run on the node during
93@@ -154,6 +178,12 @@
94 which defaults to `generic`, so e.g. `i386/generic` may be abbreviated to
95 `i386`. The default architecture is `amd64`.
96
97+
98+HTTP caching proxies:
99+---------------------
100+
101+Save bandwidth and time, especially when repeatedly invoking maas-test.
102+
103 --http-proxy=URL
104 Use the given HTTP proxy for all downloads, both on the testing system and on
105 the nodes: KVM images, MAAS boot images, and Ubuntu packages. Like
106
107=== modified file 'maastest/cases.py'
108--- maastest/cases.py 2013-11-28 17:09:31 +0000
109+++ maastest/cases.py 2013-12-05 16:31:01 +0000
110@@ -14,6 +14,10 @@
111 "TestOneNode",
112 ]
113
114+from abc import (
115+ ABCMeta,
116+ abstractproperty,
117+ )
118 import httplib
119 import json
120 import logging
121@@ -25,7 +29,6 @@
122 from maastest.maas_enums import NODE_STATUS
123 from maastest.maasfixture import MAASFixture
124 from maastest.proxyfixture import LocalProxyFixture
125-import testresources
126 import testtools
127 from testtools.testcase import gather_details
128
129@@ -49,12 +52,32 @@
130 raise
131
132
133-class TestMAAS(testtools.TestCase, testresources.ResourcedTestCase):
134+class TestMAAS(testtools.TestCase):
135 """Base class for testing machine compatibility with MAAS."""
136
137- @classmethod
138- def configure(cls, args):
139- cls.args = args
140+ __metaclass__ = ABCMeta
141+
142+ interface = abstractproperty(doc=(
143+ "The interface on which MAAS will provide DHCP."))
144+ interactive = abstractproperty(doc=(
145+ "Run interactively, prompting the user for input as necessary."))
146+
147+ bmc_mac = abstractproperty(doc=(
148+ "The MAC address of the node's BMC."))
149+ bmc_username = abstractproperty(doc=(
150+ "The username to use when authenticating with the node's BMC."))
151+ bmc_password = abstractproperty(doc=(
152+ "The password to use when authenticating with the node's BMC."))
153+
154+ series = abstractproperty(doc=(
155+ "Ubuntu series to use for enlistment and commissioning."))
156+ architecture = abstractproperty(doc=(
157+ "Architecture of the node being tested."))
158+
159+ http_proxy = abstractproperty(doc=(
160+ "Optionally, an HTTP proxy to use, defined as a URL."))
161+ disable_cache = abstractproperty(doc=(
162+ "Don't attempt to cache downloaded resources."))
163
164 fixtures = ClassFixture()
165
166@@ -64,12 +87,12 @@
167 # TODO: series and architecture should be script parameters.
168 architecture = utils.determine_vm_architecture()
169
170- if cls.args.http_proxy is not None:
171+ if cls.http_proxy is not None:
172 # The user has passed an external proxy; don't start polipo,
173 # just use that proxy for everything.
174- proxy_url = cls.args.http_proxy
175+ proxy_url = cls.http_proxy
176 logging.info("Using external proxy %s." % proxy_url)
177- elif cls.args.disable_cache:
178+ elif cls.disable_cache:
179 # The user has passed --disable-cache, so don't start polipo
180 # and don't try to use an external proxy.
181 proxy_url = ''
182@@ -83,13 +106,13 @@
183
184 cls.machine = KVMFixture(
185 series='saucy', architecture=architecture,
186- proxy_url=proxy_url, direct_interface=cls.args.interface)
187+ proxy_url=proxy_url, direct_interface=cls.interface)
188 cls.fixtures.addCleanup(delattr, cls, "machine")
189 cls.fixtures.useFixture(cls.machine)
190
191 cls.maas = MAASFixture(
192- cls.machine, proxy_url=proxy_url, series=cls.args.series,
193- architecture=cls.args.architecture)
194+ cls.machine, proxy_url=proxy_url, series=cls.series,
195+ architecture=cls.architecture)
196 cls.fixtures.addCleanup(delattr, cls, "maas")
197 cls.fixtures.useFixture(cls.maas)
198
199@@ -163,7 +186,7 @@
200 """
201 # TODO: only print this in debug/verbose mode.
202 self.print_connection_details()
203- if self.args.interactive:
204+ if self.interactive:
205 self.power_up_node_interactive()
206 else:
207 self.power_up_node_noninteractive()
208@@ -182,13 +205,13 @@
209 power_address = self.find_bmc_ip_address()
210 self.maas.kvm_fixture.run_command([
211 'ipmipower', '-h', power_address,
212- '-u', self.args.bmc_username,
213- '-p', self.args.bmc_password, '--off'],
214+ '-u', self.bmc_username,
215+ '-p', self.bmc_password, '--off'],
216 check_call=True)
217 self.maas.kvm_fixture.run_command([
218 'ipmipower', '-h', power_address,
219- '-u', self.args.bmc_username,
220- '-p', self.args.bmc_password, '--on'],
221+ '-u', self.bmc_username,
222+ '-p', self.bmc_password, '--on'],
223 check_call=True)
224
225 def find_bmc_ip_address(self):
226
227=== modified file 'maastest/main.py'
228--- maastest/main.py 2013-12-05 14:02:09 +0000
229+++ maastest/main.py 2013-12-05 16:31:01 +0000
230@@ -21,8 +21,8 @@
231
232
233 def main(argv=None):
234- from maastest.parser import prepare_parser
235- args = prepare_parser().parse_args()
236+ from maastest.parser import parser
237+ args = parser.parse_args(argv)
238
239 import logging
240 logging.basicConfig(
241@@ -41,18 +41,28 @@
242 "poorer performance than on physical hardware.")
243
244 from maastest.cases import TestOneNode
245+ from maastest.console import run_console
246
247 class ConfiguredTestMAAS(TestOneNode):
248- """A configured version of TestInteractiveOneNode.
249+ """A configured test case for testing a node's MAAS compatibility.
250
251- ConfiguredTestMAAS is a TestMAAS use to configure it by calling
252- cls.configure. We need to configure the class itself and not
253- the instance because the KVMFixture TestMAAS instanciates and needs
254- to configure is created at the class level.
255+ `TestMAAS` has a number of abstract properties; it cannot be
256+ instantiated until those properties are defined.
257 """
258- ConfiguredTestMAAS.configure(args)
259-
260- from maastest.console import run_console
261+
262+ interface = args.interface
263+ interactive = args.interactive
264+
265+ bmc_mac = args.bmc_mac
266+ bmc_username = args.bmc_username
267+ bmc_password = args.bmc_password
268+
269+ series = args.series
270+ architecture = args.architecture
271+
272+ http_proxy = args.http_proxy
273+ disable_cache = args.disable_cache
274+
275 # This try/except is a band-aid to make sure the fixtures gets cleaned
276 # up when a KeyboardInterrupt exception is raised.
277 # This does miss out on test-level and module-level tearDowns.
278
279=== modified file 'maastest/parser.py'
280--- maastest/parser.py 2013-11-29 13:22:37 +0000
281+++ maastest/parser.py 2013-12-05 16:31:01 +0000
282@@ -11,7 +11,7 @@
283
284 __metaclass__ = type
285 __all__ = [
286- "prepare_parser",
287+ "parser",
288 ]
289
290 import argparse
291@@ -23,8 +23,8 @@
292
293 class MAASTestArgumentParser(argparse.ArgumentParser):
294
295- def parse_args(self):
296- args = super(MAASTestArgumentParser, self).parse_args()
297+ def parse_args(self, argv=None):
298+ args = super(MAASTestArgumentParser, self).parse_args(argv)
299 bmc_details = (args.bmc_mac, args.bmc_username, args.bmc_password)
300 if args.interactive:
301 if bmc_details != (None, None, None):
302@@ -47,74 +47,75 @@
303 return args
304
305
306-latest_LTS_series = distro_info.UbuntuDistroInfo().lts()
307-
308-
309-def prepare_parser():
310- # TODO: This is not tested sinc how maas-test will interface with
311- # checkbox is still in flux. Right now, this is just to get the
312- # testing going.
313- parser = MAASTestArgumentParser(
314- description="Test whether or not a node is compatible with MAAS.")
315-
316- parser.add_argument(
317- 'interface', type=text_type,
318- help="Network interface that connects the testing system to the "
319- "dedicated testing network. MAAS will serve DHCP on this "
320- "interface.")
321- parser.add_argument(
322- '--archive', type=text_type, default=None,
323- help="Additional personal package archive, 'sources.list' entry, or "
324- "distribution component to install on the virtual MAAS system.")
325- parser.add_argument(
326- '--interactive', action='store_true',
327- help="Interactive mode. Prompt the user to power up the node "
328- "manually instead of doing it automatically through the BMC.")
329-
330- # BMC details.
331- parser.add_argument(
332- '--bmc-mac', type=text_type,
333- help="MAC address of the node's baseboard management controller. "
334- "Use this if the BMC is connected to the interface given as "
335- "argument. In non-interactive mode, either --bmc-mac or "
336- "--bmc-ip. Not needed in interactive mode.")
337- parser.add_argument(
338- '--bmc-ip', type=text_type,
339- help="IP address of the node's baseboard management controller. "
340- "Use this if the BMC is not connected to the inteface given as "
341- "argument. Note that the IP address must not change for the "
342- "duration of the testing. In non-interactive mode, either "
343- "--bmc-mac or --bmc-ip. Not needed in interactive mode.")
344- parser.add_argument(
345- '--bmc-username', type=text_type,
346- help="Username for authenticating to the node's BMC. "
347- "Not needed in interactive mode.")
348- parser.add_argument(
349- '--bmc-password', type=text_type,
350- help="Password for authenticating to the node's BMC. "
351- "Not needed in interactive mode.")
352-
353- # MAAS images.
354- parser.add_argument(
355- '--series', type=text_type,
356- choices=utils.get_supported_series(),
357- default=latest_LTS_series,
358- help="Code name for the Ubuntu release series that the node should "
359- "run. Defaults to the latest LTS (%(default)s).")
360- parser.add_argument(
361- '--architecture', type=text_type,
362- default='amd64',
363- help="Node's CPU architecture, e.g. armhf/highbank or amd64. "
364- "Defaults to %(default)s.")
365-
366- # Proxy.
367- parser.add_argument(
368- '--http-proxy', type=text_type,
369- help="HTTP proxy to use for all downloads. Imples --disable-cache.")
370- parser.add_argument(
371- '--disable-cache', action='store_true',
372- help="By default, maas-test uses polipo to cache all KVM images, "
373- "MAAS images and apt packages. If --disable-cache is specified, "
374- "polipo will be disabled.")
375-
376- return parser
377+# TODO: This is not tested since how maas-test will interface with
378+# checkbox is still in flux. Right now, this is just to get the
379+# testing going.
380+parser = MAASTestArgumentParser(
381+ description="Test whether or not a node is compatible with MAAS.")
382+
383+parser.add_argument(
384+ 'interface', type=text_type,
385+ help="Network interface that connects the testing system to the "
386+ "dedicated testing network. MAAS will serve DHCP on this "
387+ "interface.")
388+# TODO: Actually use the archive value.
389+parser.add_argument(
390+ '--archive', type=text_type, default=None,
391+ help="Additional personal package archive, 'sources.list' entry, or "
392+ "distribution component to install on the virtual MAAS system.")
393+parser.add_argument(
394+ '--interactive', action='store_true',
395+ help="Interactive mode. Prompt the user to power up the node "
396+ "manually instead of doing it automatically through the BMC.")
397+
398+bmc_details = parser.add_argument_group(
399+ "bmc details", "BMC/IPMI details for performing the first power on of "
400+ "the node under test. Required when --interactive has not been "
401+ "specified.")
402+bmc_details.add_argument(
403+ '--bmc-mac', type=text_type, metavar="MAC",
404+ help="MAC address of the node's baseboard management controller. "
405+ "Use this if the BMC is connected to the interface given as "
406+ "argument. In non-interactive mode, either --bmc-mac or "
407+ "--bmc-ip. Not needed in interactive mode.")
408+bmc_details.add_argument(
409+ '--bmc-ip', type=text_type, metavar="IP",
410+ help="IP address of the node's baseboard management controller. "
411+ "Use this if the BMC is not connected to the inteface given as "
412+ "argument. Note that the IP address must not change for the "
413+ "duration of the testing. In non-interactive mode, either "
414+ "--bmc-mac or --bmc-ip. Not needed in interactive mode.")
415+bmc_details.add_argument(
416+ '--bmc-username', type=text_type, metavar="USERNAME",
417+ help="Username for authenticating to the node's BMC. "
418+ "Not needed in interactive mode.")
419+bmc_details.add_argument(
420+ '--bmc-password', type=text_type, metavar="PASSWORD",
421+ help="Password for authenticating to the node's BMC. "
422+ "Not needed in interactive mode.")
423+
424+maas_images = parser.add_argument_group(
425+ "maas images", "Series and architecture choices pertaining to the "
426+ "node-under-test.")
427+maas_images.add_argument(
428+ '--series', type=text_type,
429+ choices=utils.get_supported_series(),
430+ default=distro_info.UbuntuDistroInfo().lts(),
431+ help="Code name for the Ubuntu release series that the node should "
432+ "run. Defaults to the latest LTS (%(default)s).")
433+maas_images.add_argument(
434+ '--architecture', type=text_type, default='amd64',
435+ help="Node's CPU architecture, e.g. arm/highbank or amd64. "
436+ "Defaults to %(default)s.")
437+
438+proxy_options = parser.add_argument_group(
439+ "http proxies & caching", "Save bandwidth and time, especially when "
440+ "repeatedly invoking %(prog)s.")
441+proxy_options.add_argument(
442+ '--http-proxy', type=text_type, metavar="URL",
443+ help="HTTP proxy to use for all downloads. Imples --disable-cache.")
444+proxy_options.add_argument(
445+ '--disable-cache', action='store_true',
446+ help="By default, maas-test uses polipo to cache all KVM images, "
447+ "MAAS images and apt packages. If --disable-cache is specified, "
448+ "polipo will be disabled.")
449
450=== modified file 'maastest/utils.py'
451--- maastest/utils.py 2013-11-29 12:38:02 +0000
452+++ maastest/utils.py 2013-12-05 16:31:01 +0000
453@@ -253,4 +253,7 @@
454 def get_user_input(message):
455 """A wrapper around raw_input() that allows us to sanely mock it.
456 """
457+ # TODO: Find out why it's not possible to sanely mock raw_input()
458+ # without this.
459+ # TODO: Don't use this unless --interactive has been set.
460 return input(message)
461
462=== modified file 'man/maas-test.8'
463--- man/maas-test.8 2013-11-29 05:30:32 +0000
464+++ man/maas-test.8 2013-12-05 16:31:01 +0000
465@@ -91,7 +91,8 @@
466 testing network. You will be passing this interface to \fImaas\-test\fP\&.
467 .sp
468 5. Depending on caching, the test may download and store large amounts of data
469-on the testing system. Make sure you have sufficient disk space and network bandwidth.
470+on the testing system. Make sure you have sufficient disk space and network
471+bandwidth.
472 .sp
473 The data that needs downloading and/or caching consists mostly of system images
474 for the virtual machine, and for booting the node. As a rule of thumb, count
475@@ -112,11 +113,41 @@
476 to power up the node.
477 .UNINDENT
478 .SH OPTIONS
479+.SS Positional arguments:
480+.INDENT 0.0
481+.TP
482+.B interface
483+The interface on which MAAS will provide DHCP. This is the machine\(aqs
484+interface on which the node under test should be plugged\-in.
485+.UNINDENT
486+.SS Optional arguments:
487 .INDENT 0.0
488 .TP
489 .B \-h\fP,\fB \-\-help
490 Show help and exit.
491 .TP
492+.B \-\-interactive
493+Interactive mode. Instead of powering up the node automatically through
494+IPMI, prompt the user to turn it on manually. In this mode there is no need
495+to specify BMC details; the MAAS enlistment process will discover them
496+automatically.
497+.TP
498+.BI \-\-archive\fB= archive
499+Optional package repository name. If given, the virtual machine may install
500+packages from this additional archive as well as from the main Ubuntu
501+archive. The archive is added to the virtual machine using
502+\(aqadd\-apt\-repository\(aq; it may be either a line in the format of apt\(aqs
503+sources.list, or a personal package archive identifier in the form
504+\(aqppa:<user>/<ppa\-name>\(aq, or a distribution component that should be enabled.
505+This is typically used to test recent versions of MAAS that are only
506+available in a PPA such as "ppa:maas\-maintainers/dailybuilds".
507+.UNINDENT
508+.SS BMC details:
509+.sp
510+BMC/IPMI details for performing the first power on of the node under test.
511+Required when \-\-interactive has not been specified.
512+.INDENT 0.0
513+.TP
514 .BI \-\-bmc\-mac\fB= MAC
515 MAC address for the node\(aqs baseboard management controller. MAAS will
516 control the node\(aqs power and boot sequence through this controller.
517@@ -140,22 +171,11 @@
518 .BI \-\-bmc\-user\fB= user
519 Username for IPMI authentication. Use with \fB\-\-bmc\-password\fP\&.
520 Not needed in interactive mode.
521-.TP
522-.B \-\-interactive
523-Interactive mode. Instead of powering up the node automatically through
524-IPMI, prompt the user to turn it on manually. In this mode there is no need
525-to specify BMC details; the MAAS enlistment process will discover them
526-automatically.
527-.TP
528-.BI \-\-archive\fB= archive
529-Optional package repository name. If given, the virtual machine may install
530-packages from this additional archive as well as from the main Ubuntu
531-archive. The archive is added to the virtual machine using
532-\(aqadd\-apt\-repository\(aq; it may be either a line in the format of apt\(aqs
533-sources.list, or a personal package archive identifier in the form
534-\(aqppa:<user>/<ppa\-name>\(aq, or a distribution component that should be enabled.
535-This is typically used to test recent versions of MAAS that are only
536-available in a PPA such as "ppa:maas\-maintainers/dailybuilds".
537+.UNINDENT
538+.SS MAAS images:
539+.sp
540+Series and architecture choices pertaining to the node\-under\-test.
541+.INDENT 0.0
542 .TP
543 .BI \-\-series\fB= codename
544 Code name for the Ubuntu release series that should be run on the node during
545@@ -167,6 +187,11 @@
546 architecture only. The architecture may include a sub\-architecture name,
547 which defaults to \fIgeneric\fP, so e.g. \fIi386/generic\fP may be abbreviated to
548 \fIi386\fP\&. The default architecture is \fIamd64\fP\&.
549+.UNINDENT
550+.SS HTTP caching proxies:
551+.sp
552+Save bandwidth and time, especially when repeatedly invoking maas\-test.
553+.INDENT 0.0
554 .TP
555 .BI \-\-http\-proxy\fB= URL
556 Use the given HTTP proxy for all downloads, both on the testing system and on
557
558=== modified file 'packages.txt'
559--- packages.txt 2013-11-28 07:46:00 +0000
560+++ packages.txt 2013-12-05 16:31:01 +0000
561@@ -8,7 +8,6 @@
562 python-netaddr
563 python-netifaces
564 python-six
565-python-testresources
566 python-testtools
567 python-virtualenv
568 python-xdg
569@@ -17,7 +16,6 @@
570 python3-mock
571 python3-netaddr
572 python3-six
573-python3-testresources
574 python3-testtools
575 python3-xdg
576 qemu-kvm

Subscribers

People subscribed via source and target branches