Merge lp:~jkakar/launchpadlib/fake-launchpad into lp:launchpadlib

You can get a readable diff with this command:

bzr diff -r ancestor:lp:launchpadlib `bzr st -S -r ancestor:lp:launchpadlib|cut -d ' ' -f 3|grep -v xml`|less

NB: the adding of the lp wadl for testing makes the diff useless - look at this in a local branch :P

Oh, and I'd like to suggest a related thing, that perhaps could be done as a pre-requisite branch.

If shipping a wadl, why not include the date-time when it was included, so that the very first http call to get a wadl can be an IMS :)

Does anyone have time to look at this? It'd be great to get it
landed.

> Oh, and I'd like to suggest a related thing, that perhaps could be done as a
> pre-requisite branch.
>
> If shipping a wadl, why not include the date-time when it was included, so
> that the very first http call to get a wadl can be an IMS :)

What does IMS stand for?

We could use the packaged WADL to prepopulate a client cache, but it would only give any benefit until the next time we revved launchpad. Never more than a month, expected value two weeks (since the launchpadlib release cycle has nothing to do with the launchpad release cycle) and most users would never benefit since they use old packaged versions.

> What does IMS stand for?

Never mind, it stands for If-Modified-Since.

I've gone through this branch. It's pretty complicated and I'm not prepared to do a full review yet, but I can tell you that it's good enough to build on. Go ahead and make your improvements in a branch based on this one.

I do have some suggestions and questions, from large-scale to small-scale:

1. I would like most of this code to eventually live in lazr.restfulclient, since it mostly has nothing to do with Launchpad per se. The only code that needs to live in launchpadlib is a) the credential code, b) the Launchpad WADL file itself, c) any code for Launchpad-specific index lookup operations like launchpad.people['leonardr']. This doesn't need to happen right now, but I would like it to be easy to move that code out.

2. Similarly, I would like to see most of the tests use a small, custom-designed WADL file (or several small files) rather than the Launchpad WADL file. This will make the tests easy to understand and will ensure that they don't break when the Launchpad WADL changes. It will also make it easy to put the tests in lazr.restfulclient. wadllib takes the "one WADL file per feature" approach for most of the tests added since the original release, and I think it's quite comprehensible.

3. I don't like the way you use "FakeResource" to cover the service root resource and entry resource, but not the collection resource. It seems like "FakeResource" should be a generic superclass and you should specialize. Even if the FakeServiceRootResource and FakeEntryResource are just empty subclasses of FakeResource, the code would be easier to read.

4.

+ # This is a crappy heuristic that attempts to guess the name
+ # of the key to use for index lookups.

Put the index lookups in a later branch, when you can do them properly. What is "properly"? I thought you might be able to do introspection on the CollectionWithKeyBasedLookup subclasses like PersonSet and BugSet, but I'm a little doubtful now. You might have to re-implement the _get_url_from_id implementations.

5. I don't understand why "The result of a fake method" is always a FakeResource. Your test_callable_object_return_type defines a method that returns a collection. Shouldn't it be a FakeCollectionResource? Between this and #3 I am getting the impression that FakeResource really _is_ an all-in-one resource class and that FakeCollectionResource is only used in certain circumstances. But I still don't have a good grasp of the difference.

I'll work on a more detailed review tomorrow.

This might be a stupid idea, but maybe it would be nice to build up the wadl with Python expressions and then have a convenience function that says "make me a pretend webservice here". This would be somewhat in line with 2, except programmers wouldn't have to learn wadl.

Leonard:

[1]

Agreed. I didn't really realize that was the right place for this
when I started, but it would be great if lazr.restfulclient using
applications had access to something like this out-of-the-box.

Okay, so the index-based lookups are something special layered on
top of the WADL and not something that can be determined directly
from it, right?

[2]

Using a small purpose-built WADL file is a good idea. As for
Jonathan's suggestion to use Python, I think anyone hacking on this
code probably ought to know how WADL works or or at least make
random guesses like I did. That said, maybe a WADL builder would be
useful in other places and could be used here.

Two things would have been very helpful:

1. Having a model-based representation of the model, so that I
   didn't have to deal with XML directly.

2. Documentation describing, in simple terms, how the WADL file
   works. I mostly guessed and, I think, mostly got things right.

[3]

That's a good idea, it is confusing right now.

[4]

I'll have to check this out, thanks for the hint.

[5]

The FakeCollection is only used for parameters that end in
_collection_link. I think you're right though, the logic that wraps
method call return types in FakeResource should be smarter and
determine if the return type is a collection.

Thanks for taking the time to look at this.

Leonard:

Do you have time to look at this? I'd like to get it landed sooner
than later, but I'm still not quite sure what the blockers are.

Hi Jamu, Leonard: Is this branch still under development?

Graham:

I keep thinking about this but I've also kind of lost steam on it
since it's been sitting here for ages. I'd love for it to be merged
because the lack of testability in launchpadlib is a big problem. My
feeling is that the API exposed to users of the test class is good but
that the internal details could be improved, as Leonard points out.
I'd like to see it landed and improved with subsequent branches, since
it's already useful.

I've merged trunk and run the tests. The WADL file still needs to be
updated (or changed to get pulled whenever tests run).

Thanks for writing this, Jamu. This is such a big gap for anyone writing Launchpad clients. I do feel getting some thing into this gap, however imperfect, is better than nothing. It's a real shame this has been sitting here for 20 months.

Perhaps this should go into a separate project where it can evolve separately of lazr.restful/launchpadlib and be perhaps cross a lower barrier? As far as I can see it doesn't actually patch anything in launchpadlib; it only adds new classes.

The main thing I would like to see both as a reviewer and would-be user is just a README demonstrating how one could use it, and what kind of thing it's supposed to help you with.

People trying to catch up on the history of this should read https://code.edge.launchpad.net/~jkakar/launchpadlib/testing-support/+merge/14444

I'm not very familiar with the internals of the existing implementation but the code for this looks reasonable to me, at least to the point where I would not be afraid to change it. I would like to see the xml-parsing stuff split out a bit from the FakeResource, and perhaps changed to use lxml, but there's no reason that needs to be done in the first iteration.

+ def test_callable_object_return_type(self):
+ """
+ The result of a fake method is a L{FakeResource}, automatically
+ created from the object used to define the return object.
+ """
+ branches = dict(total_size="8")
+ self.launchpad.me = dict(getBranches=lambda statuses: branches)
+ branches = self.launchpad.me.getBranches([])
+ self.assertTrue(isinstance(branches, FakeResource))
+ self.assertEqual("8", branches.total_size)
+

I feel I would like this to make a bigger distinction between code that's running "behind the scenes" to set up the mock object, vs things being done on the front end of the mock object once they're set up. It seems like assigning to "me" is a behind-the-scenes setup thing and not something you can do on a real Launchpad, whereas accessing its attributes is pretending to be talking to a real one.

Maybe this is not a meaningful distinction for this library?

.. you've probably thought about this much more than me, but it seems to me you'd want

1- interfaces that can potentially be used against either a real launchpad or a fake one, assuming the code calling them makes no assumptions about the existence or nonexistence of particular data - for instance making a person and setting some attributes on them should be in that class

2- interfaces that simulate behaviour or state we could assume would already be present on the other end of a remote launchpad, like the existence of 'me' or searchTasks

3- interfaces that populate sample data more quickly/efficiently than talking to Launchpad

Why do I think I want this?

+ self.launchpad.me = dict(getBranches=lambda statuses: branches)
+ branches = self.launchpad.me.getBranches([])

This seems oddly nonpythonic. I can guess there is a getattr hack there, and that's fine for simulating the behaviour on 'me.getBranches', but it seems odd to also load things into it by assigning to a dict.

Secondly, I feel that if just assigning to things works, there is a risk people will have code that accidentally passes, when assigning to lplib proxy objects won't behave the same way. Maybe this isn't a real problem, and we just need to write some actual tests using this to see what happens.

> Thanks for writing this, Jamu. This is such a big gap for anyone writing
> Launchpad clients. I do feel getting some thing into this gap, however
> imperfect, is better than nothing. It's a real shame this has been sitting
> here for 20 months.
>
> Perhaps this should go into a separate project where it can evolve separately
> of lazr.restful/launchpadlib and be perhaps cross a lower barrier? As far as
> I can see it doesn't actually patch anything in launchpadlib; it only adds new
> classes.

That's an interesting idea... though it feels like a compromise, but
maybe one worth making to get the code out and more easily accessible.

> The main thing I would like to see both as a reviewer and would-be user is
> just a README demonstrating how one could use it, and what kind of thing it's
> supposed to help you with.

Good call.

> People trying to catch up on the history of this should read
> https://code.edge.launchpad.net/~jkakar/launchpadlib/testing-
> support/+merge/14444
>
> I'm not very familiar with the internals of the existing implementation but
> the code for this looks reasonable to me, at least to the point where I would
> not be afraid to change it. I would like to see the xml-parsing stuff split
> out a bit from the FakeResource, and perhaps changed to use lxml, but there's
> no reason that needs to be done in the first iteration.
>
> + def test_callable_object_return_type(self):
> + """
> + The result of a fake method is a L{FakeResource}, automatically
> + created from the object used to define the return object.
> + """
> + branches = dict(total_size="8")
> + self.launchpad.me = dict(getBranches=lambda statuses: branches)
> + branches = self.launchpad.me.getBranches([])
> + self.assertTrue(isinstance(branches, FakeResource))
> + self.assertEqual("8", branches.total_size)
> +
>
> I feel I would like this to make a bigger distinction between code that's
> running "behind the scenes" to set up the mock object, vs things being done on
> the front end of the mock object once they're set up. It seems like assigning
> to "me" is a behind-the-scenes setup thing and not something you can do on a
> real Launchpad, whereas accessing its attributes is pretending to be talking
> to a real one.
>
> Maybe this is not a meaningful distinction for this library?

Yeah, so the way it works, and the reason for the weird dict()-based
interface is that you set fake data on the FakeLaunchpad
object... when that happens, it validates it against the WADL file to
make sure that what you're setting is actually possible. Each of
those dict()'s being set on the FakeLaunchpad instance represent
objects that would be exposed on the real Launchpad instance. When
you set an object, say to represent a person, you don't have to
provide all the attributes and methods the real version would have,
just the ones you care about for your test.

It really is a bit of a hack, but the best one I could think of. I
haven't used this code in a real project, so I can only guess that the
WADL validation will prevent silly problems.

> .. you've probably thought about this much more than me, but it seems to me
> you'd want
>
> 1- interfaces that can potentially be used against either a real launchpad or
> a fake one, assuming the code calling them makes no assumptions about the
> existence or nonexistence of particular data - for instance making a person
> and setting some attributes on them should be in that class

Yes. If I understand correctly, that's basically what is in place,
except that the interface is some XML in a WADL file.

> 2- interfaces that simulate behaviour or state we could assume would already
> be present on the other end of a remote launchpad, like the existence of 'me'
> or searchTasks

Yes, these are basically defined in the WADL file.

> 3- interfaces that populate sample data more quickly/efficiently than talking
> to Launchpad

I don't think I really understand this...?

> Why do I think I want this?
>
> + self.launchpad.me = dict(getBranches=lambda statuses: branches)
> + branches = self.launchpad.me.getBranches([])
>
> This seems oddly nonpythonic. I can guess there is a getattr hack there, and
> that's fine for simulating the behaviour on 'me.getBranches', but it seems odd
> to also load things into it by assigning to a dict.
>
> Secondly, I feel that if just assigning to things works, there is a risk
> people will have code that accidentally passes, when assigning to lplib proxy
> objects won't behave the same way. Maybe this isn't a real problem, and we
> just need to write some actual tests using this to see what happens.

As above, the WADL validation should help. The real point of the
FakeLaunchpad instance is just to create a place to hang fake data
off. You then hand the fake into your application, as though it were
a real Launchpad instance, and, if you've provided all the right bits,
everything should work.

It's a bit tricky because launchpadlib doesn't have a set of
pre-defined interfaces. Almost everything it exposes is built based
on the WADL file.

The signatures for some methods has changed in launchpadlib, so the
fakes are a little out of date.

For those methods with optional arguments I think it's okay if the
fake accepts a subset of the arguments - it will still be useful - but
they still must be in-sync with the real methods. We could add some
tests to automatically compare using inspect.getargspec.

If this small amount of work is done I think this code should be in
launchpadlib rather than going stale outside of it. Even if it's not
perfect (when are fakes perfect?) I think it's useful. It also
self-selects contributors: anyone using it will be perfectly capable
of contributing to it, and motivated to do so.

On Thu, Apr 14, 2011 at 2:54 PM, Gavin Panella
<email address hidden> wrote:
...
> (when are fakes perfect?)

When the tests for the real thing also run against the fakes.

jml

It's been awhile since I looked at this, so not surprised there's some
bitrot... I guess one issue is that there's an old WADL here, whereas
the WADL should probably be pulled dynamically. Does anyone know what
we need to do to get this landed...? It's not clear to me anymore.

I think my preference would be to fix the most important issues and
get this merged. After that, we can iterate to improve the logic
(it's a bit tricky in places).

On Fri, Apr 15, 2011 at 2:13 AM, Jamu Kakar <email address hidden> wrote:
> It's been awhile since I looked at this, so not surprised there's some
> bitrot... I guess one issue is that there's an old WADL here, whereas
> the WADL should probably be pulled dynamically.  Does anyone know what
> we need to do to get this landed...?  It's not clear to me anymore.
>
> I think my preference would be to fix the most important issues and
> get this merged.  After that, we can iterate to improve the logic
> (it's a bit tricky in places).

+1

> It's been awhile since I looked at this, so not surprised there's some
> bitrot... I guess one issue is that there's an old WADL here, whereas
> the WADL should probably be pulled dynamically. Does anyone know what
> we need to do to get this landed...? It's not clear to me anymore.

I think the signatures of the methods that are faked need to be
updated where they're out of date. It would be nice to have a test to
keep them in sync, but I don't think that should block landing.

Perhaps the WADL could be updated too. Not being able to get it
dynamically should not block landing either, IMO.

It might be worth issuing a warning when importing the new modules,
just to tell people that it's an alpha feature for now.

Gavin:

The signatures of fakes are validated against the WADL, so the issue
isn't really in FakeLaunchpad as much as it's because the WADL is old.
Basically, FakeLaunchpad doesn't know anything about the data
model--it just verifies that fakes you create match the WADL.