Merge lp:~jkakar/launchpadlib/testing-support 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

This branch introduces a new launchpadlib.testing.launchpad module
that contains a FakeLaunchpad. This is a fake version of
launchpadlib.launchpad.Launchpad for use in unit tests. The basic
idea is that you set sample data on the fake API in the setup phase
of your test, for example:

    def test_me(self):
        credentials = object()
        launchpad = FakeLaunchpad(credentials)
        launchpad.me = dict(name="foo", display_name="Foo")
        self.assertEqual("foo", launchpad.me.name)
        self.assertEqual("Foo", launchpad.me.display_name)

Objects that would normally be returned by the API are represented
as dicts, as can be seen above with the person set on
'launchpad.me'. The 'name' and 'display_name' attributes are
validated against the WADL definition to make sure that they are
allowed to exist and to make sure they contain valid values. Only
partial objects need to be supplied, to make it easy to write tests
without creating complete sample data sets. There are tests in the
branch that should help you make sense of the implementation.

Other notes:

- Method parameters are not validated at all.

- There's no support for deleting sample data set on the fake API
  instance.

- The implementation is a bit hackish, hard to understand and, in
  some places, relies on heuristics.

- I've tried as much as possible to mimic the behaviour of the real
  Launchpad instance, but there are no tests that ensure the fake
  and real versions of the API behave the same way. It'd be worth
  adding this kind of thing in the future.

- I haven't tried to use this to write tests for a real application
  yet, so there may some obvious things missing.

- There is no user documentation. It'd be good to add a doctest in
  the future that describes and demonstrates the behaviour of a
  FakeLaunchpad instance.

I've just realized that I've forgotten to provide handling of the
lp_save method. I'll hack on that, but in the meantime I would
appreciate a review of what's here.

Jamu,

This is a branch I have often thought about writing and actually started work on recently. I think about writing this branch every time I have to check out a Launchpad instance just so I can run the launchpadlib tests. Your implementation of this branch looks much better than mine would have been. Jonathan Lange has also talked about writing a similar branch. But the reason I never wrote this branch is the reason I must regretfully vote "disapprove" on this code review: its promise of additional test coverage is mostly an illusion.

As you put it, "there are no tests that ensure the fake and real versions of the API behave the same way." It's generally safe to assume that the Launchpad WADL is an accurate description of the Launchpad web service, but 1) the WADL changes every month, and 2) WADL only describes the HTTP interface, not the server-side logic. (As you put it, "[m]ethod parameters are not validated at all.")

So, you can test your script against a fake Launchpad that has the basic structure of an unknown old version of Launchpad. If you want to reproduce the basic structure of the most current version of Launchpad, you can download a fresh WADL every time, but it's not a good idea to introduce a network dependency into your tests. Or, you can check out a Launchpad instance and download its WADL through a local connection.

But once you've checked out a Launchpad instance, you can "just" run your tests within the Launchpad instance, the way launchpadlib's tests are run. I put "just" in quotes because this whole process is currently quite annoying--but you are running against a full Launchpad instance with all the features of Launchpad and a well-known data set. If your test passes here, you know it will pass against the real Launchpad.

If this alternative didn't exist (eg. because Launchpad was not open source), then your branch would make a lot of sense. It lets you make sure your script doesn't violate the web service's published contract, which is better than nothing. But because Launchpad is open source, you can do better by going through some extra trouble.

In short, I think it's more promising to work on making it easier to run tests in a Launchpad instance. I'd like to hear your thoughts on this. Maybe you think it'll never be easy enough to test your script against a locally running Launchpad and that this is the next best thing. Even if I accepted that, I'd like to see a better solution than including an old version of the Launchpad WADL in launchpadlib. (We have an old version of the Launchpad WADL in wadllib, but it's not used to test features of Launchpad. It was just a nice complicated WADL file we happened to have lying around.)

On November 17, 2009, Leonard Richardson wrote:
> But the reason I never wrote this branch is the reason I must regretfully
> vote "disapprove" on this code review: its promise of additional test
> coverage is mostly an illusion.

I disagree with this assessment. It is true that this doesn't help at all in
ensuring integration-level tests, but it does help in ensuring unit-test
coverage.

This is basically an easy way to write stubs that are at least to some extent
share the external interface with the Launchpad web service.

Unit-tests are there to assert that your code works as you expect it to work
_as a unit_. The fact that the code might not work when used against the real
web service is an integration problem and should be covered by a separate
testing strategy (integration testing).

But unit-tests are very useful for a fast TDD cycle, which is very hard to
achieve when you are doing integration testing and have to pay the Launchpad
start-up cost.

I agree thought that the WADL moving target is the biggest problem here. Maybe
if the build step could try to download the latest one, that would be less of
a problem.

--
Francis J. Lacoste
<email address hidden>

Thanks for the comments! I have some opinions/ideas about the
branch:

- I agree that keeping FakeLaunchpad's behaviour in sync with a real
  Launchpad instance is tricky. I haven't done anything about it in
  this branch mostly because of time, but writing a set of tests to
  ensure that FakeLaunchpad correctly mirrors Launchpad is not
  impossible.

- The benefit of being able to write unit tests is massive. For
  example, it's particularly difficult to write a robust
  launchpadlib-using application because accessing (many) attributes
  can result in HTTP exceptions being raised. Dealing with such a
  fragile environment without tests is pretty much impossible. I
  would rather deal with problems that come up due to fake/real
  mismatches and be able to write tests than not.

- Running Launchpad locally is painful. Asking users of the API to
  setup a local Launchpad installation creates a barrier to entry
  and an ongoing maintenance headache. If this turns out to be the
  answer I suspect users will opt not to write non-trivial
  applications with launchpadlib. I will be much less excited about
  using the library if I have to maintain a local Launchpad setup.

- I included a WADL file in the branch because it was the easiest
  way for me to get going with the idea. I've envisioned making it
  possible to download a WADL file as part of a test run from the
  beginning. Adding a network dependency to a test suite is an
  annoyance, but one I'd be happy to live with.

- The note about validating method parameters was not about it being
  impossible, just about it not being done yet. The WADL file
  includes enough data to do some validation. It's true that the
  WADL file doesn't provide all the information necessary to do a
  perfect job, but I feel it provides enough to be useful.

Obviously I feel that this branch (or the idea, at least) is useful,
otherwise I wouldn't have implemented it. I'm even curious about
taking it farther and seeing if something like this can be pushed
into lazr.restful[,client], but I don't know enough about either of
those things to know how realistic that might be.

Regardless of whether or not this branch is accepted, I feel that
the testing story for launchpadlib is rather poor at present and
that something needs to be done about it. I want to write
applications using launchpadlib that I can depend on and, at
present, I can't do that.

There's another important point that I think is obvious, but I'll
mention it anyway: it's important for tests to be fast. The tests
I've written for FakeLaunchpad itself demonstrate that this method
can result in fast tests. The 40 or so tests I've added run in 0.1s
locally, though they achieve this speed by caching the wadllib
Application object and reusing it for each test. Even still,
reloading the WADL file isn't too terrible.

launchpadlib is quite slow because of all the round trips. Writing
a test suite for a non-trivial application that needs a real
Launchpad API server will eventually result in a slow test suite
that will be annoying to work with.

Another important point is that using a real Launchpad API server
for unit tests will cause difficulty when it comes to simulating
failures. Doing so with FakeLaunchpad will be much easier.

Leonard:

Before you go, can you please comment on this? I realize it's not
quite merge-ready but I'd like to know if you still disapprove of
the idea entirely or if the arguments that have been made have
swayed your opinion. I'd like to finish up the missing bits
(primarily handling lp_save) and get this merged, hopefully before
the end of the year.

Attention conservation notice: go ahead.

When I tried to write a branch like this in the past, I was always trying to test launchpadlib itself. My goal was to set up a dummy Launchpad web service and make sure that the Launchpad object sent the right HTTP requests when you manipulated it, and did the right thing with different HTTP responses from the dummy service.

I was judging your branch against these criteria, but you're doing something different. You don't dummy the Launchpad web service to test the Launchpad object; you dummy the Launchpad object to test other stuff. Your dummy object can't be used to test the Launchpad object except on a superficial level, but it is very useful for testing code that uses the Launchpad class. So I think it's good enough for what it does.

We all agree it would be better if tests ran against a real Launchpad instance, but that's slow and complicated, and running against an up-to-date WADL gets us most of the way there. So please continue with this work.

> When I tried to write a branch like this in the past, I was always trying to
> test launchpadlib itself. My goal was to set up a dummy Launchpad web service
> and make sure that the Launchpad object sent the right HTTP requests when you
> manipulated it, and did the right thing with different HTTP responses from the
> dummy service.

My initial attempts at solving the problem I have (make it easy to
test applications that use launchpadlib) involved this kind of
thing, but I quickly realized that it was going to be very painful.

> I was judging your branch against these criteria, but you're doing something
> different. You don't dummy the Launchpad web service to test the Launchpad
> object; you dummy the Launchpad object to test other stuff. Your dummy object
> can't be used to test the Launchpad object except on a superficial level, but
> it is very useful for testing code that uses the Launchpad class. So I think
> it's good enough for what it does.

Right, FakeLaunchpad can't be used to test Launchpad in any
meaningful way. I would still like to explore the idea of writing
some set of integration tests to ensure that the FakeLaunchpad and
Launchpad objects behave the same way, but it's not exactly clear to
me what the best way to do that is yet.

> We all agree it would be better if tests ran against a real Launchpad
> instance, but that's slow and complicated, and running against an up-to-date
> WADL gets us most of the way there. So please continue with this work.

Great, thanks a lot!