Merge lp:~rainct/zeitgeist/922620 into lp:~zeitgeist/zeitgeist/bluebird

[Siegfried Gevatter]

[lp:~rainct/zeitgeist/922620]

388. By Siegfried Gevatter on 2012-02-07

Add test case for monitor re-connection

389. By Siegfried Gevatter on 2012-02-07

Make bus initialization lazy so tests connect to the correct D-Bus instance

390. By Siegfried Gevatter on 2012-02-07

testutils.py: randomize DISPLAY so more than one test can run simultaneously

[Seif Lotfy (seif)]

Ok I had to learn this the hard way since you said it is just going to be
more...
Anyhow looks fine for me

On Tue, Feb 7, 2012 at 10:12 PM, Siegfried Gevatter <email address hidden>wrote:

> Siegfried Gevatter has proposed merging lp:~rainct/zeitgeist/922620 into
> lp:zeitgeist.
>
> Requested reviews:
> Zeitgeist Framework Team (zeitgeist)
> Related bugs:
> Bug #922620 in Zeitgeist Framework: "Fix failing test cases:
> testBlacklistSignals and testDataSourceSignals"
> https://bugs.launchpad.net/zeitgeist/+bug/922620
>
> For more details, see:
> https://code.launchpad.net/~rainct/zeitgeist/922620/+merge/91920
> --
> https://code.launchpad.net/~rainct/zeitgeist/922620/+merge/91920
> Your team Zeitgeist Framework Team is requested to review the proposed
> merge of lp:~rainct/zeitgeist/922620 into lp:zeitgeist.
>
> === modified file 'python/client.py'
> --- python/client.py 2011-10-29 13:31:12 +0000
> +++ python/client.py 2012-02-07 21:11:18 +0000
> @@ -40,6 +40,15 @@
>
> log = logging.getLogger("zeitgeist.client")
>
> +# This is here so testutils.py can override it with a private bus
> connection
> +global session_bus
> +session_bus = dbus.SessionBus()
> +def get_bus():
> + return session_bus
> +def _set_bus(bus):
> + global session_bus
> + session_bus = bus
> +
> class _DBusInterface(object):
> """Wrapper around dbus.Interface adding convenience methods."""
>
> @@ -47,7 +56,6 @@
> # that here because otherwise all instances would share their state.
> _disconnect_callbacks = None
> _reconnect_callbacks = None
> - _generic_callbacks = None
>
> @staticmethod
> def get_members(introspection_xml):
> @@ -69,8 +77,9 @@
> def reconnect(self):
> if not self._reconnect_when_needed:
> return
> - self.__proxy = dbus.SessionBus().get_object(
> - self.__iface.requested_bus_name,
> self.__object_path)
> + self.__proxy = get_bus().get_object(
> + self.__iface.requested_bus_name,
> self.__object_path,
> + follow_name_owner_changes=True)
> self.__iface = dbus.Interface(self.__proxy,
> self.__interface_name)
> self._load_introspection_data()
>
> @@ -131,8 +140,7 @@
> self.reconnect()
> if signal not in self.__signals:
> raise TypeError("Unknown signal name: %s" % signal)
> - self._generic_callbacks.add((signal, callback))
> - self.__proxy.connect_to_signal(
> + return self.__proxy.connect_to_signal(
> signal,
> callback,
> dbus_interface=self.__interface_name,
> @@ -169,7 +177,6 @@
>
> self._disconnect_callbacks = set()
> self._reconnect_callbacks = set()
> - self._generic_callbacks = set()
>
> # Listen to (dis)connection notifications, for connect_exit
> and connect_join
> def name_owner_changed(connection_name):
> @@ -181,15 +188,9 @@
> retu...

[lp:~rainct/zeitgeist/922620]

391. By Siegfried Gevatter on 2012-02-07

Fix monitors being installed twice; python-dbus always gives us a name
owner changed notification, so we want to ignore this first one.

=== modified file 'python/client.py'
--- python/client.py	2011-10-29 13:31:12 +0000
+++ python/client.py	2012-02-07 22:51:18 +0000
@@ -40,6 +40,19 @@
 
 log = logging.getLogger("zeitgeist.client")
 
+# This is here so testutils.py can override it with a private bus connection.
+# Init needs to be lazy so tests will use the private D-Bus instance.
+global session_bus
+session_bus = None
+def get_bus():
+	global session_bus
+	if session_bus is None:
+		session_bus = dbus.SessionBus()
+	return session_bus
+def _set_bus(bus):
+	global session_bus
+	session_bus = bus
+
 class _DBusInterface(object):
 	"""Wrapper around dbus.Interface adding convenience methods."""
 
@@ -47,7 +60,6 @@
 	# that here because otherwise all instances would share their state.
 	_disconnect_callbacks = None
 	_reconnect_callbacks = None
-	_generic_callbacks = None
 
 	@staticmethod
 	def get_members(introspection_xml):
@@ -69,8 +81,9 @@
 	def reconnect(self):
 		if not self._reconnect_when_needed:
 			return
-		self.__proxy = dbus.SessionBus().get_object(
-			self.__iface.requested_bus_name, self.__object_path)
+		self.__proxy = get_bus().get_object(
+			self.__iface.requested_bus_name, self.__object_path,
+			follow_name_owner_changes=True)
 		self.__iface = dbus.Interface(self.__proxy, self.__interface_name)
 		self._load_introspection_data()
 
@@ -131,8 +144,7 @@
 			self.reconnect()
 		if signal not in self.__signals:
 			raise TypeError("Unknown signal name: %s" % signal)
-		self._generic_callbacks.add((signal, callback))
-		self.__proxy.connect_to_signal(
+		return self.__proxy.connect_to_signal(
 			signal,
 			callback,
 			dbus_interface=self.__interface_name,
@@ -167,29 +179,28 @@
 		self._reconnect_when_needed = reconnect
 		self._load_introspection_data()
 		
+		self._first_connection = True
 		self._disconnect_callbacks = set()
 		self._reconnect_callbacks = set()
-		self._generic_callbacks = set()
 		
 		# Listen to (dis)connection notifications, for connect_exit and connect_join
 		def name_owner_changed(connection_name):
 			if connection_name == "":
-				callbacks = self._disconnect_callbacks
 				self.__methods = self.__signals = None
+				for callback in self._disconnect_callbacks:
+					callback()
+			elif self._first_connection:
+				# python-dbus guarantees that it'll call NameOwnerChanged at startup
+				# (even if the service was already running). When that happens, we
+				# don't want to connect the signals a second time.
+				self._first_connection = False
 			else:
 				if not self._reconnect_when_needed:
 					return
 				self.reconnect()
-				callbacks = self._reconnect_callbacks
-				for signal, callback in self._generic_callbacks:
-					try:
-						self.connect(signal, callback)
-					except TypeError:
-						log.exception("Failed to reconnect to signal \"%s\" "
-							"after engine disconnection." % signal)
-			for callback in callbacks:
-				callback()
-		dbus.SessionBus().watch_name_owner(self.__iface.requested_bus_name,
+				for callback in self._reconnect_callbacks:
+					callback()
+		get_bus().watch_name_owner(self.__iface.requested_bus_name,
 			name_owner_changed)
 
 class ZeitgeistDBusInterface(object):
@@ -233,7 +244,8 @@
 		if not name in cls.__shared_state["extension_interfaces"]:
 			interface_name = "org.gnome.zeitgeist.%s" % name
 			object_path = "/org/gnome/zeitgeist/%s" % path
-			proxy = dbus.SessionBus().get_object(busname, object_path)
+			proxy = get_bus().get_object(busname, object_path,
+				follow_name_owner_changes=True)
 			iface = _DBusInterface(proxy, interface_name, object_path)
 			iface.BUS_NAME = busname
 			iface.INTERFACE_NAME = interface_name
@@ -244,8 +256,8 @@
 	def __init__(self, reconnect=True):
 		if not "dbus_interface" in self.__shared_state:
 			try:
-				proxy = dbus.SessionBus().get_object(self.BUS_NAME,
-					self.OBJECT_PATH)
+				proxy = get_bus().get_object(self.BUS_NAME,
+					self.OBJECT_PATH, follow_name_owner_changes=True)
 			except dbus.exceptions.DBusException, e:
 				if e.get_dbus_name() == "org.freedesktop.DBus.Error.ServiceUnknown":
 					raise RuntimeError(
@@ -292,7 +304,7 @@
 		self._path = monitor_path
 		self._insert_callback = insert_callback
 		self._delete_callback = delete_callback
-		dbus.service.Object.__init__(self, dbus.SessionBus(), monitor_path)
+		dbus.service.Object.__init__(self, get_bus(), monitor_path)
 	
 	def get_path (self): return self._path
 	path = property(get_path,

=== modified file 'test/dbus/blacklist-test.py'
--- test/dbus/blacklist-test.py	2011-09-07 20:48:27 +0000
+++ test/dbus/blacklist-test.py	2012-02-07 22:51:18 +0000
@@ -110,11 +110,13 @@
 		newAllTemplates = blacklist.GetTemplates()
 		self.assertEquals(len(newAllTemplates), 0)
 
-	def testBlacklistSignals(self):
+	def testBlacklistSignals(self, mainloop=None, connect_signals=True):
 		self.blacklist = self._get_blacklist_iface()
-		mainloop = self.create_mainloop()
+		if mainloop is None:
+			mainloop = self.create_mainloop()
 
 		template1 = Event.new_for_values(
+			timestamp=0,
 			interpretation=Interpretation.ACCESS_EVENT,
 			subject_uri="http://nothingtoseehere.gov")
 
@@ -124,7 +126,6 @@
 		@asyncTestMethod(mainloop)
 		def cb_added(template_id, event_template):
 			global hit
-			print "*** cb_added with hit", hit
 			self.assertEquals(hit, 0)
 			hit = 1
 			self.assertEquals(template_id, "TestTemplate")
@@ -140,8 +141,9 @@
 			mainloop.quit()
 
 		# Connect to signals
-		self.blacklist.connect('TemplateAdded', cb_added)
-		self.blacklist.connect('TemplateRemoved', cb_removed)
+		if connect_signals:
+			self.blacklist.connect('TemplateAdded', cb_added)
+			self.blacklist.connect('TemplateRemoved', cb_removed)
 
 		def launch_tests():
 			self.blacklist.AddTemplate("TestTemplate", template1)
@@ -150,5 +152,16 @@
 
 		mainloop.run()
 
+	def testBlacklistSignalWithReconnection(self):
+		mainloop = self.create_mainloop()
+		self.testBlacklistSignals(mainloop)
+
+		# Restart the Zeitgeist daemon...
+		self.kill_daemon()
+		self.spawn_daemon()
+
+		# ... and try again without re-connecting to the signals
+		self.testBlacklistSignals(mainloop, connect_signals=False)
+
 if __name__ == "__main__":
 	unittest.main()

=== modified file 'test/dbus/monitor-test.py'
--- test/dbus/monitor-test.py	2011-09-07 20:48:27 +0000
+++ test/dbus/monitor-test.py	2012-02-07 22:51:18 +0000
@@ -286,5 +286,34 @@
 		self.assertEquals(1, len(result))
 		self.assertEquals(1, result.pop())
 
+	def testMonitorReconnection(self):
+		result = []
+		mainloop = self.create_mainloop()
+		events = parse_events("test/data/three_events.js")
+		
+		@asyncTestMethod(mainloop)
+		def notify_insert_handler(time_range, events):
+			result.extend(events)
+			mainloop.quit()
+		
+		@asyncTestMethod(mainloop)
+		def notify_delete_handler(time_range, event_ids):
+			mainloop.quit()
+			self.fail("Unexpected delete notification")
+			
+		self.client.install_monitor(TimeRange.always(), [],
+			notify_insert_handler, notify_delete_handler)
+
+		# Restart the Zeitgeist daemon to test automagic monitor re-connection
+		self.kill_daemon()
+		self.spawn_daemon()
+
+		# Insert events in idle loop to give the reconnection logic enough time
+		gobject.idle_add(lambda *args: self.client.insert_events(events))
+
+		mainloop.run()
+		
+		self.assertEquals(3, len(result))
+
 if __name__ == "__main__":
 	unittest.main()

=== modified file 'test/dbus/testutils.py'
--- test/dbus/testutils.py	2012-02-07 17:49:49 +0000
+++ test/dbus/testutils.py	2012-02-07 22:51:18 +0000
@@ -21,12 +21,14 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import unittest
+import dbus
 import os
 import time
 import sys
 import signal
 import tempfile
 import shutil
+import random
 from subprocess import Popen, PIPE
 
 # DBus setup
@@ -36,7 +38,8 @@
 
 # Import local Zeitgeist modules
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
-from zeitgeist.client import ZeitgeistDBusInterface, ZeitgeistClient
+from zeitgeist.client import ZeitgeistDBusInterface, ZeitgeistClient, \
+	get_bus, _set_bus
 from zeitgeist.datamodel import Event, Subject, Interpretation, Manifestation, \
 	TimeRange, NULL_EVENT
 
@@ -175,11 +178,18 @@
 		
 		# hack to clear the state of the interface
 		ZeitgeistDBusInterface._ZeitgeistDBusInterface__shared_state = {}
+		
+		# Replace the bus connection with a private one for each test case,
+		# so that they don't share signals or other state
+		_set_bus(dbus.SessionBus(private=True))
+		get_bus().set_exit_on_disconnect(False)
+		
 		self.client = ZeitgeistClient()
 	
 	def tearDown(self):
 		assert self.daemon is not None
 		assert self.client is not None
+		get_bus().close()
 		self.kill_daemon()
 		if 'ZEITGEIST_TESTS_KEEP_TMP' in os.environ:
 			print '\n\nAll temporary files have been preserved in %s\n' \
@@ -389,7 +399,9 @@
 		self.assertEqual(ev1, ev2)
 
 class DBusPrivateMessageBus(object):
-	DISPLAY = ":27"
+	# Choose a random number so it's possible to have more than
+	# one test running at once.
+	DISPLAY = ":%d" % random.randint(20, 100)
 
 	def _run(self):
 		os.environ.update({"DISPLAY": self.DISPLAY})