Code review comment for lp:~jkakar/launchpadlib/testing-support

Revision history for this message
Leonard Richardson (leonardr) wrote :

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.)

review: Disapprove

« Back to merge proposal