maas: improved tests with new gomaasapi

maas: improved tests with new gomaasapi

Improved provider/maas testing using new gomaasapi test server features.

1, New tests:

- TestGetNetworkMACs - getNetworkMACs function should read connected MACs to the
network

- TestGetInstanceNetworkInterfaces - getInstanceNetworkInterfaces should read
list of network interfaces together with their MACs using data from
commissioning stage

- TestSetupNetworks - setupNetworks function should get []NetworkInfo data
combining the data from getNetworkMacs and getInstanceNetworkInterfaces
functions

- TestSupportNetworks - verify that gomaasapi supports network operations

2. gomaasapi was advanced to new revision in dependencies.tsv

https://code.launchpad.net/~klyachin/juju-core/102-new-tests-with-new-gomaasapi/+merge/216055

(do not edit description out of merge proposal)

[vladislav.klyachin, 2014-04-16 10:26:56]

Please take a look.

[dimitern, 2014-04-17 08:50:14]

Looking good, but needs some polishing I think.

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
File provider/maas/environ_whitebox_test.go (left):

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:452: func (suite *environSuite)
TestBootstrapIntegratesWithEnvirons(c *gc.C) {
Why is this removed?

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
File provider/maas/environ_whitebox_test.go (right):

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:155: const lshwXMLNode0 = `
I'd prefer to have this as a template (maybe broken down in chunks like "common
boilerplate" and "network interface element" that can be added multiple times.
Then, you can have a function to render the complete XML for a node, passing a
list of NICs to include in the XML (along with interface name and MAC for each
one).

Does that make sense?

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:388: // vlanTag is either number (e.g.
"1") or "null"
Either document all arguments or none. I don't think we need to document vlanTag
specifically.

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:587: sort.Strings(networks)
Instead of sorting the slice, you can use jc.SameContents checker.

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:740: // check for jc.IsTrue, because
gomaasapi testing server now supports networks
No need for this comment anymore.

[vladislav.klyachin, 2014-04-17 14:32:48]

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
File provider/maas/environ_whitebox_test.go (left):

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:452: func (suite *environSuite)
TestBootstrapIntegratesWithEnvirons(c *gc.C) {
On 2014/04/17 08:50:15, dimitern wrote:
> Why is this removed?

This test completely repeats TestBootstrapSucceeds.

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
File provider/maas/environ_whitebox_test.go (right):

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:155: const lshwXMLNode0 = `
On 2014/04/17 08:50:15, dimitern wrote:
> I'd prefer to have this as a template (maybe broken down in chunks like
"common
> boilerplate" and "network interface element" that can be added multiple times.
> Then, you can have a function to render the complete XML for a node, passing a
> list of NICs to include in the XML (along with interface name and MAC for each
> one).
> 
> Does that make sense?

Yes

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:388: // vlanTag is either number (e.g.
"1") or "null"
On 2014/04/17 08:50:15, dimitern wrote:
> Either document all arguments or none. I don't think we need to document
vlanTag
> specifically.

Changed vlanTag type to interger to not confuse anybody.

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:587: sort.Strings(networks)
On 2014/04/17 08:50:15, dimitern wrote:
> Instead of sorting the slice, you can use jc.SameContents checker.

Done, thanks for hint

https://codereview.appspot.com/88380044/diff/1/provider/maas/environ_whitebox...
provider/maas/environ_whitebox_test.go:740: // check for jc.IsTrue, because
gomaasapi testing server now supports networks
On 2014/04/17 08:50:15, dimitern wrote:
> No need for this comment anymore.

Done

[vladislav.klyachin, 2014-04-17 17:13:57]

Please take a look.

[dimitern, 2014-04-18 08:26:36]

Thanks! LGTM with a few trivials.

https://codereview.appspot.com/88380044/diff/20001/provider/maas/environ_whit...
File provider/maas/environ_whitebox_test.go (right):

https://codereview.appspot.com/88380044/diff/20001/provider/maas/environ_whit...
provider/maas/environ_whitebox_test.go:196: // Create node 0: it will be used as
the bootstrap node.
It seems to me this block (196-217) can be extracted in a helper func (e.g.
makeNode(systemId, macs, ...) ?), since it's called twice.

https://codereview.appspot.com/88380044/diff/20001/provider/maas/environ_whit...
provider/maas/environ_whitebox_test.go:573: func (suite *environSuite)
TestGetNetworkMACs(c *gc.C) {
Please, add some blank lines between the logical setup steps to give it some
space and make it easier to read.

https://codereview.appspot.com/88380044/diff/20001/provider/maas/environ_whit...
provider/maas/environ_whitebox_test.go:652: func (suite *environSuite)
TestGetInstanceNetworkInterfaces(c *gc.C) {
Here as well (blank lines between blocks of steps), and in the test cases below.

[vladislav.klyachin, 2014-04-21 07:38:25]

Please take a look.