Merge lp:~chipaca/ubuntu-push/multi-valued into lp:ubuntu-push

[John Lenton]

Made the bus support multi-valued signals (preparatory for notifications). Also added tests for TestingBus.

Made the bus support multi-valued signals (preparatory for notifications). Also added tests for TestingBus.

[lp:~chipaca/ubuntu-push/multi-valued]

25. By John Lenton on 2014-01-21

formatting

[Samuele Pedroni (pedronis)]

                err = fmt.Errorf("Got wrong number of arguments in Call: %d", len(rvs))
        }
        return 0, err

shouldn't it say something like "return message arguments", reading that description I would think that the length of args was wrong,

I'm not a big fan of not doing simply:

return 0, fmt.Errorf("Got wrong number of arguments in Call: %d", len(rvs))

without falltrhough there

[Samuele Pedroni (pedronis)]

61 + return nil, fmt.Errorf("Got wrong number of arguments in GetProperty: %d", len(variantvs))

same unclarity here

[Samuele Pedroni (pedronis)]

217 // each of them intern, irrespective of whether Call has been called.

*in turn*

NewTestingEndpoint

misses docs

[Samuele Pedroni (pedronis)]

review: Approve

[lp:~chipaca/ubuntu-push/multi-valued]

26. By John Lenton on 2014-01-22

addressed issues raised by pedronis during peer review

=== modified file 'bus/endpoint.go'
--- bus/endpoint.go	2014-01-20 19:10:21 +0000
+++ bus/endpoint.go	2014-01-22 10:49:04 +0000
@@ -19,7 +19,7 @@
 // Here we define the Endpoint, which represents the DBus connection itself.
 
 import (
-	"errors"
+	"fmt"
 	"launchpad.net/go-dbus/v1"
 	"launchpad.net/ubuntu-push/logger"
 )
@@ -30,7 +30,7 @@
 
 // bus.Endpoint represents the DBus connection itself.
 type Endpoint interface {
-	WatchSignal(member string, f func(interface{}), d func()) error
+	WatchSignal(member string, f func(...interface{}), d func()) error
 	Call(member string, args ...interface{}) (interface{}, error)
 	GetProperty(property string) (interface{}, error)
 	Close()
@@ -66,7 +66,7 @@
 // with the unpacked value. If it's unable to set up the watch it'll return an
 // error. If the watch fails once established, d() is called. Typically f()
 // sends the values over a channel, and d() would close the channel.
-func (endp *endpoint) WatchSignal(member string, f func(interface{}), d func()) error {
+func (endp *endpoint) WatchSignal(member string, f func(...interface{}), d func()) error {
 	watch, err := endp.proxy.WatchSignal(endp.iface, member)
 	if err != nil {
 		endp.log.Debugf("Failed to set up the watch: %s", err)
@@ -82,11 +82,19 @@
 // provided when creating the endpoint). The return value is unpacked before
 // being returned.
 func (endp *endpoint) Call(member string, args ...interface{}) (interface{}, error) {
-	if msg, err := endp.proxy.Call(endp.iface, member, args...); err == nil {
-		return endp.unpackOneMsg(msg, member)
-	} else {
+	msg, err := endp.proxy.Call(endp.iface, member, args...)
+	if err != nil {
 		return 0, err
 	}
+	rvs := endp.unpackOneMsg(msg, member)
+	switch len(rvs) {
+	default:
+		return 0, fmt.Errorf("Too many values in %s response: %d", member, len(rvs))
+	case 0:
+		return 0, fmt.Errorf("Not enough values in %s response: %d", member, len(rvs))
+	case 1:
+		return rvs[0], nil
+	}
 }
 
 // GetProperty uses the org.freedesktop.DBus.Properties interface's Get method
@@ -98,13 +106,18 @@
 	if err != nil {
 		return nil, err
 	}
-	variantv, err := endp.unpackOneMsg(msg, property)
-	if err != nil {
-		return nil, err
+	variantvs := endp.unpackOneMsg(msg, property)
+	switch len(variantvs) {
+	default:
+		return nil, fmt.Errorf("Too many values in Properties.Get response: %d", len(variantvs))
+	case 0:
+		return nil, fmt.Errorf("Not enough values in Properties.Get response: %d", len(variantvs))
+	case 1:
+		// carry on
 	}
-	variant, ok := variantv.(*dbus.Variant)
+	variant, ok := variantvs[0].(*dbus.Variant)
 	if !ok {
-		return nil, errors.New("Response from Properties.Get wasn't a *dbus.Variant")
+		return nil, fmt.Errorf("Response from Properties.Get wasn't a *dbus.Variant")
 	}
 	return variant.Value, nil
 }
@@ -119,27 +132,18 @@
 */
 
 // unpackOneMsg unpacks the value from the response msg
-func (endp *endpoint) unpackOneMsg(msg *dbus.Message, member string) (interface{}, error) {
-	var v interface{}
-	if err := msg.Args(&v); err != nil {
-		endp.log.Errorf("Decoding %s: %s", member, err)
-		return 0, err
-	} else {
-		return v, nil
-	}
+func (endp *endpoint) unpackOneMsg(msg *dbus.Message, member string) []interface{} {
+	return msg.AllArgs()
 }
 
 // unpackMessages unpacks the value from the watch
-func (endp *endpoint) unpackMessages(watch *dbus.SignalWatch, f func(interface{}), d func(), member string) {
+func (endp *endpoint) unpackMessages(watch *dbus.SignalWatch, f func(...interface{}), d func(), member string) {
 	for {
 		msg, ok := <-watch.C
 		if !ok {
 			break
 		}
-		if val, err := endp.unpackOneMsg(msg, member); err == nil {
-			// errors are ignored at this level
-			f(val)
-		}
+		f(endp.unpackOneMsg(msg, member)...)
 	}
 	endp.log.Errorf("Got not-OK from %s watch", member)
 	d()

=== modified file 'bus/testing/testing_bus.go'
--- bus/testing/testing_bus.go	2014-01-20 13:45:30 +0000
+++ bus/testing/testing_bus.go	2014-01-22 10:49:04 +0000
@@ -43,6 +43,13 @@
 	return &testingBus{clientTC, NewTestingEndpoint(busTC, retvals...)}
 }
 
+// Build a bus.Bus that takes a condition to determine whether it should work,
+// as well as a condition and a series of lists of return values for the
+// testing bus.Endpoint it builds.
+func NewMultiValuedTestingBus(clientTC condition.Interface, busTC condition.Interface, retvalses ...[]interface{}) bus.Bus {
+	return &testingBus{clientTC, NewMultiValuedTestingEndpoint(busTC, retvalses...)}
+}
+
 // ensure testingBus implements bus.Interface
 var _ bus.Bus = &testingBus{}
 

=== added file 'bus/testing/testing_bus_test.go'
--- bus/testing/testing_bus_test.go	1970-01-01 00:00:00 +0000
+++ bus/testing/testing_bus_test.go	2014-01-22 10:49:04 +0000
@@ -0,0 +1,75 @@
+/*
+ Copyright 2013-2014 Canonical Ltd.
+
+ This program is free software: you can redistribute it and/or modify it
+ under the terms of the GNU General Public License version 3, as published
+ by the Free Software Foundation.
+
+ This program is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranties of
+ MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ PURPOSE.  See the GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+package testing
+
+import (
+	. "launchpad.net/gocheck"
+	"launchpad.net/ubuntu-push/bus"
+	"launchpad.net/ubuntu-push/testing/condition"
+	"testing"
+)
+
+// hook up gocheck
+func BusTest(t *testing.T) { TestingT(t) }
+
+type TestingBusSuite struct{}
+
+var _ = Suite(&TestingBusSuite{})
+
+// Test Connect() on a working bus returns an endpoint that looks right
+func (s *TestingBusSuite) TestConnectWorks(c *C) {
+	addr := bus.Address{"", "", ""}
+	tb := NewTestingBus(condition.Work(true), condition.Work(false), 42, 42, 42)
+	endp, err := tb.Connect(addr, nil)
+	c.Check(err, IsNil)
+	c.Check(endp, FitsTypeOf, &testingEndpoint{})
+	c.Check(endp.(*testingEndpoint).cond.OK(), Equals, false)
+	c.Check(endp.(*testingEndpoint).retvals, HasLen, 3)
+}
+
+// Test Connect() on a working "multi-valued" bus returns an endpoint that looks right
+func (s *TestingBusSuite) TestConnectMultiValued(c *C) {
+	addr := bus.Address{"", "", ""}
+	tb := NewMultiValuedTestingBus(condition.Work(true), condition.Work(true),
+		[]interface{}{42, 17},
+		[]interface{}{42, 17, 13},
+		[]interface{}{42},
+	)
+	endpp, err := tb.Connect(addr, nil)
+	c.Check(err, IsNil)
+	endp, ok := endpp.(*testingEndpoint)
+	c.Check(ok, Equals, true)
+	c.Check(endp.cond.OK(), Equals, true)
+	c.Assert(endp.retvals, HasLen, 3)
+	c.Check(endp.retvals[0], HasLen, 2)
+	c.Check(endp.retvals[1], HasLen, 3)
+	c.Check(endp.retvals[2], HasLen, 1)
+}
+
+// Test Connect() with a non-working bus fails
+func (s *TestingBusSuite) TestConnectNoWork(c *C) {
+	addr := bus.Address{"", "", ""}
+	tb := NewTestingBus(condition.Work(false), condition.Work(true))
+	_, err := tb.Connect(addr, nil)
+	c.Check(err, NotNil)
+}
+
+// Test TestingBus stringifies sanely
+func (s *TestingBusSuite) TestStringifyBus(c *C) {
+	tb := NewTestingBus(condition.Work(false), condition.Work(true))
+	c.Check(tb.String(), Matches, ".*TestingBus.*")
+}

=== modified file 'bus/testing/testing_endpoint.go'
--- bus/testing/testing_endpoint.go	2014-01-20 18:09:57 +0000
+++ bus/testing/testing_endpoint.go	2014-01-22 10:49:04 +0000
@@ -27,25 +27,33 @@
 
 type testingEndpoint struct {
 	cond    condition.Interface
-	retvals []interface{}
+	retvals [][]interface{}
 }
 
 // Build a bus.Endpoint that calls OK() on its condition before returning
 // the provided return values.
 //
 // NOTE: Call() always returns the first return value; Watch() will provide
-// each of them intern, irrespective of whether Call has been called.
+// each of them in turn, irrespective of whether Call has been called.
+func NewMultiValuedTestingEndpoint(cond condition.Interface, retvalses ...[]interface{}) bus.Endpoint {
+	return &testingEndpoint{cond, retvalses}
+}
+
 func NewTestingEndpoint(cond condition.Interface, retvals ...interface{}) bus.Endpoint {
-	return &testingEndpoint{cond, retvals}
+	retvalses := make([][]interface{}, len(retvals))
+	for i, x := range retvals {
+		retvalses[i] = []interface{}{x}
+	}
+	return &testingEndpoint{cond, retvalses}
 }
 
 // See Endpoint's WatchSignal. This WatchSignal will check its condition to
 // decide whether to return an error, or provide each of its return values
-func (tc *testingEndpoint) WatchSignal(member string, f func(interface{}), d func()) error {
+func (tc *testingEndpoint) WatchSignal(member string, f func(...interface{}), d func()) error {
 	if tc.cond.OK() {
 		go func() {
 			for _, v := range tc.retvals {
-				f(v)
+				f(v...)
 				time.Sleep(10 * time.Millisecond)
 			}
 			d()
@@ -63,7 +71,10 @@
 		if len(tc.retvals) == 0 {
 			panic("No return values provided!")
 		}
-		return tc.retvals[0], nil
+		if len(tc.retvals[0]) != 1 {
+			panic("Wrong number of values provided -- Call only returns a single value for now!")
+		}
+		return tc.retvals[0][0], nil
 	} else {
 		return 0, errors.New("no way")
 	}

=== modified file 'bus/testing/testing_endpoint_test.go'
--- bus/testing/testing_endpoint_test.go	2014-01-20 18:09:57 +0000
+++ bus/testing/testing_endpoint_test.go	2014-01-22 10:49:04 +0000
@@ -39,6 +39,15 @@
 	c.Check(v, Equals, m)
 }
 
+// Test the same Call() but with multi-valued endpoint
+func (s *TestingEndpointSuite) TestMultiValuedCall(c *C) {
+	var m, n uint32 = 42, 17
+	endp := NewMultiValuedTestingEndpoint(condition.Work(true), []interface{}{m}, []interface{}{n})
+	v, e := endp.Call("what")
+	c.Check(e, IsNil)
+	c.Check(v, Equals, m)
+}
+
 // Test that Call() with a negative condition returns an error.
 func (s *TestingEndpointSuite) TestCallFails(c *C) {
 	endp := NewTestingEndpoint(condition.Work(false))
@@ -50,7 +59,22 @@
 // a helpful message.
 func (s *TestingEndpointSuite) TestCallPanicsWithNiceMessage(c *C) {
 	endp := NewTestingEndpoint(condition.Work(true))
-	c.Check(func() { endp.Call("") }, PanicMatches, "No return values provided!")
+	c.Check(func() { endp.Call("") }, PanicMatches, "No return values provided.*")
+}
+
+// Test that Call() with a positive condition and an empty return value panics
+// with a helpful message.
+func (s *TestingEndpointSuite) TestCallPanicsWithNiceMessage2(c *C) {
+	endp := NewMultiValuedTestingEndpoint(condition.Work(true), []interface{}{})
+	c.Check(func() { endp.Call("") }, PanicMatches, "Wrong number of values provided.*")
+}
+
+// Test Call() with positive condition and the wrong number of arguments also
+// fails with a helpful message
+func (s *TestingEndpointSuite) TestMultiValuedCallPanicsWhenWrongNumberOfValues(c *C) {
+	var m, n uint32 = 42, 17
+	endp := NewMultiValuedTestingEndpoint(condition.Work(true), []interface{}{m, n})
+	c.Check(func() { endp.Call("") }, PanicMatches, "Wrong number of values provided.*")
 }
 
 // Test that WatchSignal() with a positive condition sends the provided return
@@ -59,16 +83,33 @@
 	var m, n uint32 = 42, 17
 	endp := NewTestingEndpoint(condition.Work(true), m, n)
 	ch := make(chan uint32)
-	e := endp.WatchSignal("what", func(u interface{}) { ch <- u.(uint32) }, func() { close(ch) })
+	e := endp.WatchSignal("what", func(us ...interface{}) { ch <- us[0].(uint32) }, func() { close(ch) })
 	c.Check(e, IsNil)
 	c.Check(<-ch, Equals, m)
 	c.Check(<-ch, Equals, n)
 }
 
+// Test that WatchSignal() calls the destructor callback when it runs out values
+func (s *TestingEndpointSuite) TestWatchDestructor(c *C) {
+	endp := NewTestingEndpoint(condition.Work(true))
+	ch := make(chan uint32)
+	e := endp.WatchSignal("what", func(us ...interface{}) {}, func() { close(ch) })
+	c.Check(e, IsNil)
+	_, ok := <-ch
+	c.Check(ok, Equals, false)
+}
+
+// Test the endpoint can be closed
+func (s *TestingEndpointSuite) TestCloser(c *C) {
+	endp := NewTestingEndpoint(condition.Work(true))
+	endp.Close()
+	// ... yay?
+}
+
 // Test that WatchSignal() with a negative condition returns an error.
 func (s *TestingEndpointSuite) TestWatchFails(c *C) {
 	endp := NewTestingEndpoint(condition.Work(false))
-	e := endp.WatchSignal("what", func(u interface{}) {}, func() {})
+	e := endp.WatchSignal("what", func(us ...interface{}) {}, func() {})
 	c.Check(e, NotNil)
 }
 

=== modified file 'networkmanager/networkmanager.go'
--- networkmanager/networkmanager.go	2014-01-20 18:09:57 +0000
+++ networkmanager/networkmanager.go	2014-01-22 10:49:04 +0000
@@ -74,7 +74,7 @@
 func (nm *networkManager) WatchState() (<-chan State, error) {
 	ch := make(chan State)
 	err := nm.bus.WatchSignal("StateChanged",
-		func(n interface{}) { ch <- State(n.(uint32)) },
+		func(ns ...interface{}) { ch <- State(ns[0].(uint32)) },
 		func() { close(ch) })
 	if err != nil {
 		nm.log.Debugf("Failed to set up the watch: %s", err)