launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/juju-core/fix-broken-test into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Fix test TestStartInstanceStartsInstance, re-enable it.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/juju-core/fix-broken-test/+merge/157632

Turns out it's completely normal to have 2 nodes started when we start an instance: both the instance in question and the bootstrap node are started.  I refactored the tests and added comments to make this clear.
-- 
https://code.launchpad.net/~rvb/juju-core/fix-broken-test/+merge/157632
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/juju-core/fix-broken-test into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-05 10:38:15 +0000
+++ environs/maas/environ.go	2013-04-08 12:04:22 +0000
@@ -185,7 +185,7 @@
 // Bootstrap is specified in the Environ interface.
 func (env *maasEnviron) Bootstrap(cons constraints.Value, stateServerCert, stateServerKey []byte) error {
 	// TODO: Fix this quick hack.  uploadTools is a now-obsolete parameter.
- 	uploadTools := false
+	uploadTools := false
 
 	// This was all cargo-culted from the EC2 provider.
 	password := env.Config().AdminSecret()

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-04-08 01:50:24 +0000
+++ environs/maas/environ_test.go	2013-04-08 12:04:22 +0000
@@ -189,46 +189,34 @@
 	return fmt.Errorf("unexpected call to writeCertAndKey")
 }
 
-func (suite *EnvironSuite) SetUpBootstrapNode(c *C, hostname string, environ *maasEnviron) {
-	input := `{"system_id": "bootstrap-sys-id", "hostname": "` + hostname + `"}`
-	node := suite.testMAASObject.TestServer.NewNode(input)
-	instance := &maasInstance{&node, suite.environ}
-	err := environ.saveState(&bootstrapState{StateInstances: []state.InstanceId{instance.Id()}})
-	c.Assert(err, IsNil)
-}
-
-// TODO: this test fails from time to time: we need to investigate what's
-// going on (also see the additional remarks below).
-func (suite *EnvironSuite) DisableTestStartInstanceStartsInstance(c *C) {
+func (suite *EnvironSuite) TestStartInstanceStartsInstance(c *C) {
 	suite.setupFakeTools(c)
 	env := suite.makeEnviron()
-	suite.setupFakeProviderStateFile(c)
+	// Create node 0: will be used as the bootstrap node.
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node0", "hostname": "host0"}`)
+	err := environs.Bootstrap(env, constraints.Value{})
+	c.Assert(err, IsNil)
+	// The bootstrap node has been started.
+	operations := suite.testMAASObject.TestServer.NodeOperations()
+	actions, found := operations["node0"]
+	c.Check(found, Equals, true)
+	c.Check(actions, DeepEquals, []string{"start"})
+
+	// Create node 1: it will be used as instance number 1.
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node1", "hostname": "host1"}`)
-	suite.SetUpBootstrapNode(c, "test", env)
-	// TODO: This node, I suspect, was here to make sure that it was *not*
-	// started but the test did the opposite (and was passing!)! 
-	// See below.
-	//suite.testMAASObject.TestServer.NewNode(`{"system_id": "node2", "hostname": "host2"}`)
-	err := environs.Bootstrap(env, constraints.Value{})
 	stateInfo, apiInfo, err := env.StateInfo()
 	c.Assert(err, IsNil)
 	stateInfo.Tag = "machine-1"
 	apiInfo.Tag = "machine-1"
-
 	series := version.Current.Series
 	instance, err := env.StartInstance("1", series, constraints.Value{}, stateInfo, apiInfo)
 	c.Assert(err, IsNil)
 	c.Check(instance, NotNil)
 
-	operations := suite.testMAASObject.TestServer.NodeOperations()
-	actions, found := operations["node1"]
+	// The instance number 1 has been started.
+	actions, found = operations["node1"]
 	c.Check(found, Equals, true)
 	c.Check(actions, DeepEquals, []string{"start"})
-	// TODO: figure out how this was passing: we're only starting one
-	// instance so the only node1 should be started!!!
-	//actions, found = operations["node2"]
-	//c.Check(found, Equals, true)
-	//c.Check(actions, DeepEquals, []string{"start"})
 }
 
 func (suite *EnvironSuite) getInstance(systemId string) *maasInstance {