Merge lp:~thomir-deactivatedaccount/autopilot/fix-state-not-found into lp:autopilot/1.3

[Thomi Richards]

Update StateNotFoundErrror exception to give more informative messages.

This branch makes a few trivial changes to the StateNotFoundError exception class, and adds tests that cover that class.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:332
http://jenkins.qa.ubuntu.com/job/autopilot-1.3-ci/10/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-1.3-saucy-amd64-ci/10
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-1.3-saucy-armhf-ci/10

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-1.3-ci/10/rebuild

review: Approve (continuous-integration)

[lp:~thomir-deactivatedaccount/autopilot/fix-state-not-found]

333. By Thomi Richards on 2013-09-15

Fix unicode string literals in test.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:333
http://jenkins.qa.ubuntu.com/job/autopilot-1.3-ci/11/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-1.3-saucy-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-1.3-saucy-armhf-ci/11

Click here to trigger a rebuild:
http://s-jenkins:8080/job/autopilot-1.3-ci/11/rebuild

review: Approve (continuous-integration)

[Martin Pitt (pitti)]

Nitpick: Constructions like

  lambda: StateNotFoundError()

are redundant, just saying "StateNotFoundError" should suffice.

56 + def __str__(self):
57 + return self.message

This does the wrong thing for Python 2, as you initialize .message with an unicode, so you don't actually return a str (which is bytes in Python 2).

133 + err.message,

I think we shouldn't make .message an official API. In Python 3 exceptions don't have it any more, instead you'd just do str().

175 + unicode(err),

That will fail in Python 3. It's okay for the 1.3 branch, but must be skipped for trunk when running under Py3.

review: Needs Fixing

[lp:~thomir-deactivatedaccount/autopilot/fix-state-not-found]

334. By Thomi Richards on 2013-09-16

Fixes from code review.

[Thomi Richards (thomir-deactivatedaccount)]

Fixed - this is landing in 1.3, so if it's incompatible in python 3, we'll fix that when we forward-port it to trunk.

[Martin Pitt (pitti)]

LGTM, thanks for the cleanups.

review: Approve

=== modified file 'autopilot/introspection/dbus.py'
--- autopilot/introspection/dbus.py	2013-09-13 16:26:18 +0000
+++ autopilot/introspection/dbus.py	2013-09-16 13:42:21 +0000
@@ -42,13 +42,52 @@
 
 
 class StateNotFoundError(RuntimeError):
-    """Raised when a piece of state information from unity is not found."""
-
-    message = "State not found for class with name '{}' and id '{}'."
-
-    def __init__(self, class_name, class_id):
-        super(StateNotFoundError, self).__init__(
-            self.message.format(class_name, class_id))
+
+    """Raised when a piece of state information is not found.
+
+    This exception is commonly raised when the application has destroyed (or
+    not yet created) the object you are trying to access in autopilot. This
+    typically happens for a number of possible reasons:
+
+     * The UI widget you are trying to access with
+        :py:met:`DBusIntrospectionObject.select_single` or
+        :py:met:`DBusIntrospectionObject.select_single` does not exist yet.
+    * The UI widget you are trying to access has been destroyed by the
+        application.
+
+    """
+
+    def __init__(self, class_name=None, **filters):
+        """Construct a StateNotFoundError.
+
+        :raises ValueError: if neither the class name not keyword arguments
+            are specified.
+
+        """
+        if class_name is None and not filters:
+            raise ValueError("Must specify either class name or filters.")
+
+        if class_name is None:
+            self._message = \
+                u"State not found with filters {}.".format(
+                    repr(filters)
+                )
+        elif not filters:
+            self._message = u"State not found for class '{}'.".format(
+                class_name
+            )
+        else:
+            self._message = \
+                u"State not found for class '{}' and filters {}.".format(
+                    class_name,
+                    repr(filters)
+                )
+
+    def __str__(self):
+        return self._message.encode('utf8')
+
+    def __unicode__(self):
+        return self._message
 
 
 class IntrospectableObjectMetaclass(type):
@@ -326,10 +365,10 @@
             of the appropriate type (the latter case is for overridden emulator
             classes).
 
-        :raises: **ValueError** if the query returns more than one item. *If
+        :raises ValueError: if the query returns more than one item. *If
             you want more than one item, use select_many instead*.
 
-        :raises: **TypeError** if neither *type_name* or keyword filters are
+        :raises TypeError: if neither *type_name* or keyword filters are
             provided.
 
         .. seealso::
@@ -504,7 +543,7 @@
         try:
             return self.get_state_by_path(self.get_class_query_string())[0]
         except IndexError:
-            raise StateNotFoundError(self.__class__.__name__, self.id)
+            raise StateNotFoundError(self.__class__.__name__, id=self.id)
 
     def get_class_query_string(self):
         """Get the XPath query string required to refresh this class's

=== added file 'autopilot/tests/unit/test_exceptions.py'
--- autopilot/tests/unit/test_exceptions.py	1970-01-01 00:00:00 +0000
+++ autopilot/tests/unit/test_exceptions.py	2013-09-16 13:42:21 +0000
@@ -0,0 +1,75 @@
+# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
+#
+# Autopilot Functional Test Tool
+# Copyright (C) 2013 Canonical
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY 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/>.
+#
+
+
+from testtools import TestCase
+from testtools.matchers import raises, Equals
+
+from autopilot.introspection.dbus import StateNotFoundError
+
+
+class StateNotFoundTests(TestCase):
+
+    def test_requires_class_name_to_construct(self):
+        """You must pass a class name in to the StateNotFoundError exception
+        class initialiser in order to construct it.
+
+        """
+        self.assertThat(
+            StateNotFoundError,
+            raises(ValueError("Must specify either class name or filters."))
+        )
+
+    def test_can_be_constructed_with_class_name_only(self):
+        """Must be able to construct error class with a class name only."""
+        err = StateNotFoundError("MyClass")
+        self.assertThat(
+            str(err),
+            Equals("State not found for class 'MyClass'.")
+        )
+        self.assertThat(
+            unicode(err),
+            Equals(u"State not found for class 'MyClass'.")
+        )
+
+    def test_can_be_constructed_with_filters_only(self):
+        """Must be able to construct exception with filters only."""
+        err = StateNotFoundError(foo="bar")
+        self.assertThat(
+            str(err),
+            Equals("State not found with filters {'foo': 'bar'}.")
+        )
+        self.assertThat(
+            unicode(err),
+            Equals(u"State not found with filters {'foo': 'bar'}.")
+        )
+
+    def test_can_be_constructed_with_class_name_and_filters(self):
+        """Must be able to construct with both class name and filters."""
+        err = StateNotFoundError('MyClass', foo="bar")
+        self.assertThat(
+            str(err),
+            Equals("State not found for class 'MyClass'"
+                   " and filters {'foo': 'bar'}.")
+        )
+        self.assertThat(
+            unicode(err),
+            Equals(u"State not found for class 'MyClass'"
+                   " and filters {'foo': 'bar'}.")
+        )