I see the bikeshedding has already been done.  Good—you won't be needing much from me then.  Thanks for putting your weight behind this project!

My notes are essentially about superficialities.  See below.  What they mostly boil down to is that you seem to be missing an abstraction layer to translate parsed WADL into meaningful information.  In practice it looks like a matter of extracting lots of little helpers to separate the parsing nitty-gritty from the real meaning of the Fake* code.


=== added file 'src/launchpadlib/testing/launchpad.py'

—8<—

+class FakeResource(object):
+    """
+    Fake resource validates and represents sample data set on L{FakeLaunchpad}
+    instances.

Found this a bit hard to read.  Maybe leave out the “Fake resource” at the beginning?  The “set” could be a verb or a noun, so maybe just say “Valides and represents sample data on a L{FakeLaunchpad}”?  Has the added benefit of fitting on a single line.


+    @param application: A C{waddlib.application.Application} instance.
+    @param resource_type: A C{wadllib.application.ResourceType} instance for
+        this resource.
+    @param values: Optionally, a dict representing attribute key/value pairs
+        for this resource.

You're using a bunch of documentation conventions I'm not familiar with.  Do constructor parameters normally go into the class docstring then?


+    def __setattr__(self, name, value):
+        """Set sample data.
+
+        @param name: The name of the attribute.
+        @param values: A dict representing an object matching a resource
+            defined in Launchpad's WADL definition.

Which is it—“value” or “values”?  The code suggests that it can be a dict or… an atomic value I suppose:


+        if isinstance(value, dict):
+            self._children[name] = self._create_child_resource(name, value)
+        else:
+            values = {}
+            if self._values is not None:
+                values.update(self._values)
+            values[name] = value
+            self._check_resource_type(self._resource_type, values)
+            self.__dict__["_values"] = values

A comment might be helpful.  Does it make sense to run a full modified copy of self._values through _check_resource_type?  It checks each item individually anyway, right?


+    def __getattr__(self, name):
+        """Get sample data.
+
+        @param name: The name of the attribute.
+        """
+        result = self._children.get(name)

What kind of items can I expect to find in _children?


+        if not result:

 What exactly are you testing for?  Can result be, say, an integer that might be zero, or a string that might be empty?


+            if isinstance(self._values, dict):
+                result = self._values.get(name)
+                if callable(result):
+                    return self._wrap_method(name, result)

What does it mean for self._values not to be a dict?  One case we've seen is that it might be None… are there other possibilities?  If no, then please harmonize the test and maybe wrap it in a meaningfully-named method.


+    def _create_child_resource(self, name, values):
+        """
+        Ensure that C{values} is a valid object for the C{name} attribute and
+        return a resource object to represent it as API data.
+
+        @param name: The name of the attribute to check the C{values} object
+            against.
+        @param values: A dict with key/value pairs representing attributes and
+            methods of an object matching the C{name} resource's definition.
+        @return: A L{FakeEntry} for an ordinary resource or a
+            L{FakeCollection} for a resource that represents a collection.
+        @raises IntegrityError: Raised if C{name} isn't a valid attribute for
+            this resource or if C{values} isn't a valid object for the C{name}
+            attribute.
+        """
+        root_resource = self._application.get_resource_by_path("")
+        is_link = False
+        param = root_resource.get_parameter(name + "_collection_link",
+                                            JSON_MEDIA_TYPE)
+        if param is None:
+            is_link = True
+            param = root_resource.get_parameter(name + "_link", JSON_MEDIA_TYPE)
+        if param is not None:
+            resource_type = self._get_resource_type(param)
+            if is_link:
+                self._check_resource_type(resource_type, values)
+                return FakeEntry(self._application, resource_type, values)
+            else:
+                name, child_resource_type = (
+                    self._check_collection_type(resource_type, values))
+                return FakeCollection(self._application, resource_type, values,
+                                      name, child_resource_type)
+        else:
+            raise IntegrityError("%s isn't a valid property." % (name,))

If you switch the “if” and “else” clauses around here, the error becomes an obvious escape path and you won't need the “else” block.  Saves you a negation in the condition and an indentation.


+    def _check_attribute(self, resource_type, name, value):
+        """
+        Ensure that C{value} is a valid C{name} attribute on C{resource_type}.
+
+        @param resource_type: The resource type to check the attribute
+            against.
+        @param name: The name of the attribute.
+        @param value: The value to check.
+        """
+        get_methods = [method for method in resource_type.tag.getchildren()
+                       if method.get("name") == "GET"]
+        for response in get_methods[0].getchildren():

It looks like you're looking for a single entry.  Maybe assign to [get_method] instead, and then use get_method instead of get_methods[0]?


+            for representation in response.getchildren():
+                representation_url = representation.get("href")
+                if representation_url is not None:
+                    xml_id = self._application.lookup_xml_id(representation_url)
+                    self._check_attribute_representation(xml_id, name, value)
+                    break

A “break” like this can easily lead to maintenance problems—this code may become clearer if you can wrap the inner loop or a portion of it in a separate method.


+    def _create_resource(self, resource_type, name, result):
+        """Create a new L{FakeResource} for C{resource_type} method call result.
+
+        @param resource_type: The resource type of the method.
+        @param name: The name of the method on C{resource_type}.
+        @param result: The result of calling the method.
+        @raises IntegrityError: Raised if C{result} is an invalid return value
+            for the method.
+        @return: A L{FakeResource} for C{result}.
+        """
+        resource_name = resource_type.tag.get("id")
+        if resource_name == name:
+            name = "get"
+        xml_id = "%s-%s" % (resource_name, name)
+        [get_method] = [method for method in resource_type.tag.getchildren()
+                        if method.get("id") == xml_id]

My eyes are starting to glaze over.  A lot of this code hard to read; part of that is tough and dense subject matter, part is too much text crammed against the right margin, and part is because of little patterns like this one.  If you had that extra little layer of abstraction, I wouldn't need to “decompile” all these menial fragments to get to the meaning of the code.  This one seems to be a “find child tag by id.”  Could have gone into a helpfully-named function!


+        [response] = [element for element in get_method.getchildren()
+                      if element.tag.split("}")[-1] == "response"]

There's more unexplained low-level logic here.  Move it somewhere out of the way and give it a meaningful name!


+        for representation in response.getchildren():
+            representation_url = representation.get("href")
+            if representation_url is not None and isinstance(result, dict):
+                xml_id = self._application.lookup_xml_id(representation_url)
+                if xml_id.endswith("-full"):
+                    xml_id = xml_id[:-5]

This seems to mean “if there's a -full suffix on xml_id, strip it off.”  Replacing it with a one-liner function call would remove the clutter and the cyclomatic complexity from view.


+                if xml_id in self._application.resource_types:
+                    result_resource_type = (
+                        self._application.resource_types[xml_id])
+                else:
+                    result_resource_type = (
+                        self._application.resource_types[xml_id + "-resource"])
+                self._check_resource_type(result_resource_type, result)

This looks like more or less the opposite of the “strip a suffix” pattern we saw earlier.  I think shorter could be clearer:

    if xml_id not in self._application.resource_types:
        # Mysterious reason explained here.
        xml_id += "-resource"
    result_resource_type = self._application.resource_types[xml_id]


+    def _get_child_resource_type(self, resource_type):
+        """
+        Ensure that C{value} is a valid C{name} attribute on C{resource_type}.

The docstring does not appear to match the name.  Are you getting or checking?


+        @param resource_type: The resource type to check the attribute
+            against.
+        @param name: The name of the attribute.
+        @param value: The value to check.
+        """
+        representation_url = None
+        get_methods = [method for method in resource_type.tag.getchildren()
+                       if method.get("name") == "GET"]

I've seen this fragment before.  Please extract it.


+        for response in get_methods[0].getchildren():
+            for representation in response.getchildren():
+                representation_url = representation.get("href")
+                if representation_url is not None:
+                    break

Is this meant to be a “find first child with an href”?  Please extract.


+        xml_id = self._application.lookup_xml_id(representation_url)
+        representation_definition = (
+            self._application.representation_definitions[xml_id])
+
+        [entry_links] = [
+            param for param in representation_definition.tag.getchildren()
+            if param.get("name") == "entry_links"]

Another “find child tag by name”?  Please extract.


+    def _check_entries(self, resource_type, entries):
+        """Ensure that C{entries} are valid for a C{resource_type} collection.
+
+        @param resource_type: The resource type of the collection the entries
+            are in.
+        @param entries: A list of dicts representing objects in the
+            collection.
+        """
+        name, child_resource_type = self._get_child_resource_type(resource_type)
+        for entry in entries:
+            self._check_resource_type(child_resource_type, entry)
+        return name, child_resource_type

Where did that return come from?  The docstring doesn't mention it and the method name doesn't suggest that there is one.


+class FakeRoot(FakeResource):
+
+    def __init__(self, application):
+        """Create a L{FakeResource} for the service root of C{application}.
+
+        @param application: A C{wadllib.application.Application} instance.
+        """
+        resource_type = application.get_resource_type(
+            application.markup_url + "#service-root")
+        super(FakeRoot, self).__init__(application, resource_type)

Here the class has no docstring but the __init__ has one containing the stuff that FakeResource had in the class docstring.  Was one of them a mistake?


=== added file 'src/launchpadlib/testing/resources.py'

+# Copyright 2008 Canonical Ltd.

Why 2008?


That's all I have time for tonight, I'm afraid.  I'll have to look into the tests tomorrow, or if you're in a rush, someone else may be able to do it.  They look strikingly different: short, punchy, well-explained.  Looking forward to them!


Jeroen