Launchpad itself

Merge lp:~benji/launchpad/wadl-refactoring into lp:launchpad

  1. wadl-refactoring
  2. Merge into devel
Proposed by Benji York on 2010-09-23
Status: Merged
Approved by: Brad Crittenden on 2010-09-24
Approved revision: no longer in the source branch.
Merged at revision: 11755
Proposed branch: lp:~benji/launchpad/wadl-refactoring
Merge into: lp:launchpad
Diff against target: 365 lines (+223/-53)
6 files modified
Makefile (+1/-1)
lib/canonical/launchpad/ftests/test_wadl_generation.py (+35/-0)
lib/canonical/launchpad/rest/wadl.py (+62/-0)
lib/lp/testing/matchers.py (+35/-0)
lib/lp/testing/tests/test_matchers.py (+35/-0)
utilities/create-lp-wadl-and-apidoc.py (+55/-52)
To merge this branch: bzr merge lp:~benji/launchpad/wadl-refactoring
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2010-09-23 Approve on 2010-09-24
Brad Crittenden (community) code Approve on 2010-09-24
Review via email: mp+36452@code.launchpad.net

Description of the Change

This branch extracts the refactorings and tests from an earlier branch
that we've decided to scrap. All of the code in this branch has been
previously reviewed and approved.

The theme of the refactorings is to provide APIs to generate the WADL
and the web service documentation. The branch also includes smoke tests
to be sure that something resembling HTML and WADL are generated and no
exceptions are raised.

Some whitespace lint was fixed in this branch.

To run the added tests: bin/test -c test_wadl_generation

To post a comment you must log in.
Gavin Panella (allenap) wrote :

Nice clean up :)

I have a few fairly minor comments. [4] needs fixing before this
branch is safe to land.

[1]

+ self.assert_(wadl.startswith('<?xml '))

I think this might be better as:

   from lp.testing.matchers import Startswith
   self.assertThat(wadl, StartsWith('<?xml '))

You'll need to inherit from lp.testing.TestCase (or, more accurately,
testtools.TestCase) to get assertThat().

[2]

+ self.assert_('<html ' in html)

Same as [1].

[3]

+ return subprocess.Popen(
+ ['xsltproc', stylesheet, wadl_filename],
+ stdout=subprocess.PIPE,
+ stderr=subprocess.PIPE).communicate()[0]

Unless it's useless and/or noisy, I think stderr should be left alone,
so:

    return subprocess.Popen(
        ['xsltproc', stylesheet, wadl_filename],
        stdout=subprocess.PIPE).communicate()[0]

I also think the exit code should be checked, so:

    args = ['xsltproc', stylesheet, wadl_filename]
    process = subprocess.Popen(args, stdout=subprocess.PIPE)
    out, err = process.communicate()
    if process.returncode == 0:
        return out
    else
        raise subprocess.CalledProcessError(
            process.returncode, args)

This is similar to how subprocess.check_call() behaves.

[4]

-import _pythonpath
-

It's ugly, looks like lint, but you'll need this.

It may be worth adding a test to show that the script can run
successfully, even if it's only to show the help.

review: Needs Fixing (code)
Benji York (benji) wrote :

On Thu, Sep 23, 2010 at 12:34 PM, Gavin Panella
<email address hidden> wrote:
> Review: Needs Fixing code
> Nice clean up :)
>
> I have a few fairly minor comments. [4] needs fixing before this
> branch is safe to land.
>
>
> [1]
>
> +            self.assert_(wadl.startswith('<?xml '))
>
> I think this might be better as:
 [snip]

Done.
> [2]
>
> +            self.assert_('<html ' in html)
>
> Same as [1].

I had to implement a Contains matcher, but that wasn't hard.

> [3]
>
> Unless it's useless and/or noisy, I think stderr should be left alone,

Agreed.

> I also think the exit code should be checked, so:

Done.

> [4]
>
> -import _pythonpath
> -
>
> It's ugly, looks like lint, but you'll need this.

Fixed.

> It may be worth adding a test to show that the script can run
> successfully, even if it's only to show the help.

Since we can be sure that the script runs because it is run (almost)
every time make is run, I decided not to implement this.
--
Benji York

Brad Crittenden (bac) wrote :

Hi Benji,

Your changes as suggested by Gavin look good. We discussed the following on IRC:

1) Capitalize and punctuate

import _pythonpath # not lint, actually needed

2) alphabetize:

 20 from lp.testing.matchers import (
 21 StartsWith,
 22 Contains,
 23 )

3) Use a tuple instead of a list:
54 args = ['xsltproc', stylesheet, wadl_filename]

Thanks for the branch. I enjoyed seeing how matchers work and are extended.

review: Approve (code)
Gavin Panella (allenap) wrote :

Looks good :)

review: Approve
Brad Crittenden (bac) wrote :

Hi Benji,

I notice this branch has not landed yet. Is there something blocking you or has the work been reconsidered?

The reason I saw that it has not landed is I remember you implementing the contains matcher and I wanted to use it.

Benji York (benji) wrote :

On Mon, Oct 4, 2010 at 2:15 AM, Brad Crittenden <email address hidden> wrote:
> I notice this branch has not landed yet.  Is there something blocking you or has the work been reconsidered?
>
> The reason I saw that it has not landed is I remember you implementing the contains matcher and I wanted to use it.

There have been some EC2 hitches that have kept it from landing. As
soon as the current EC2 issue is resolved, I'll be landing the branch.
--
Benji York

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile'
2--- Makefile 2010-10-19 15:56:07 +0000
3+++ Makefile 2010-10-19 18:54:00 +0000
4@@ -60,7 +60,7 @@
5
6 $(API_INDEX): $(BZR_VERSION_INFO)
7 mkdir -p $(APIDOC_DIR).tmp
8- LPCONFIG=$(LPCONFIG) $(PY) ./utilities/create-lp-wadl-and-apidoc.py "$(WADL_TEMPLATE)"
9+ LPCONFIG=$(LPCONFIG) $(PY) ./utilities/create-lp-wadl-and-apidoc.py --force "$(WADL_TEMPLATE)"
10 mv $(APIDOC_DIR).tmp $(APIDOC_DIR)
11
12 apidoc: compile $(API_INDEX)
13
14=== added file 'lib/canonical/launchpad/ftests/test_wadl_generation.py'
15--- lib/canonical/launchpad/ftests/test_wadl_generation.py 1970-01-01 00:00:00 +0000
16+++ lib/canonical/launchpad/ftests/test_wadl_generation.py 2010-10-19 18:54:00 +0000
17@@ -0,0 +1,35 @@
18+# Copyright 2009 Canonical Ltd. This software is licensed under the
19+# GNU Affero General Public License version 3 (see the file LICENSE).
20+
21+"""Tests for the web service WADL and HTML generation APIs."""
22+
23+__metaclass__ = type
24+
25+import pkg_resources
26+import shutil
27+import subprocess
28+import tempfile
29+
30+from zope.component import getUtility
31+
32+from canonical.launchpad.rest.wadl import generate_wadl, generate_html
33+from canonical.launchpad.systemhomes import WebServiceApplication
34+from canonical.testing import LaunchpadFunctionalLayer
35+from lazr.restful.interfaces import IWebServiceConfiguration
36+from lp.testing import TestCase
37+from lp.testing.matchers import (
38+ Contains,
39+ StartsWith,
40+ )
41+
42+
43+class SmokeTestWadlAndDocGeneration(TestCase):
44+ """Smoke test the WADL and HTML generation front-end functions."""
45+
46+ layer = LaunchpadFunctionalLayer
47+
48+ def test_wadl(self):
49+ config = getUtility(IWebServiceConfiguration)
50+ for version in config.active_versions:
51+ wadl = generate_wadl(version)
52+ self.assertThat(wadl[:40], StartsWith('<?xml '))
53
54=== added file 'lib/canonical/launchpad/rest/wadl.py'
55--- lib/canonical/launchpad/rest/wadl.py 1970-01-01 00:00:00 +0000
56+++ lib/canonical/launchpad/rest/wadl.py 2010-10-19 18:54:00 +0000
57@@ -0,0 +1,62 @@
58+# Copyright 2010 Canonical Ltd. This software is licensed under the
59+# GNU Affero General Public License version 3 (see the file LICENSE).
60+
61+"""APIs to generate the web sercice WADL and documentation HTML."""
62+
63+__metaclass__ = type
64+
65+import pkg_resources
66+import subprocess
67+import urlparse
68+
69+from canonical.launchpad.webapp.interaction import (
70+ ANONYMOUS,
71+ setupInteractionByEmail,
72+ )
73+from canonical.launchpad.webapp.servers import (
74+ WebServicePublication,
75+ WebServiceTestRequest,
76+ )
77+from canonical.launchpad.webapp.vhosts import allvhosts
78+
79+
80+def generate_wadl(version):
81+ """Generate the WADL for the given version of the web service."""
82+ url = urlparse.urljoin(allvhosts.configs['api'].rooturl, version)
83+ # Since we want HTTPS URLs we have to munge the request URL.
84+ url = url.replace('http://', 'https://')
85+ request = WebServiceTestRequest(version=version, environ={
86+ 'SERVER_URL': url,
87+ 'HTTP_HOST': allvhosts.configs['api'].hostname,
88+ 'HTTP_ACCEPT': 'application/vd.sun.wadl+xml',
89+ })
90+ # We then bypass the usual publisher processing by associating
91+ # the request with the WebServicePublication (usually done by the
92+ # publisher) and then calling the root resource - retrieved
93+ # through getApplication().
94+ request.setPublication(WebServicePublication(None))
95+ setupInteractionByEmail(ANONYMOUS, request)
96+ return request.publication.getApplication(request)(request)
97+
98+
99+def generate_html(wadl_filename, suppress_stderr=True):
100+ """Given a WADL file generate HTML documentation from it."""
101+ # If we're supposed to prevent the subprocess from generating output on
102+ # stderr (like we want to do during test runs), we reassign the subprocess
103+ # stderr file handle and then discard the output. Otherwise we let the
104+ # subprocess inherit stderr.
105+ stylesheet = pkg_resources.resource_filename(
106+ 'launchpadlib', 'wadl-to-refhtml.xsl')
107+ if suppress_stderr:
108+ stderr = subprocess.PIPE
109+ else:
110+ stderr = None
111+ args = ('xsltproc', stylesheet, wadl_filename)
112+ process = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=stderr)
113+
114+ output = process.communicate()[0]
115+ if process.returncode != 0:
116+ raise subprocess.CalledProcessError(process.returncode, args)
117+
118+ return output
119+
120
121=== modified file 'lib/lp/testing/matchers.py'
122--- lib/lp/testing/matchers.py 2010-09-19 23:52:49 +0000
123+++ lib/lp/testing/matchers.py 2010-10-19 18:54:00 +0000
124@@ -227,6 +227,41 @@
125 return None
126
127
128+class DoesNotContain(Mismatch):
129+
130+ def __init__(self, matchee, expected):
131+ """Create a DoesNotContain Mismatch.
132+
133+ :param matchee: the string that did not match.
134+ :param expected: the string that `matchee` was expected to contain.
135+ """
136+ self.matchee = matchee
137+ self.expected = expected
138+
139+ def describe(self):
140+ return "'%s' does not contain '%s'." % (
141+ self.matchee, self.expected)
142+
143+
144+class Contains(Matcher):
145+ """Checks whether one string contains another."""
146+
147+ def __init__(self, expected):
148+ """Create a Contains Matcher.
149+
150+ :param expected: the string that matchees should contain.
151+ """
152+ self.expected = expected
153+
154+ def __str__(self):
155+ return "Contains '%s'." % self.expected
156+
157+ def match(self, matchee):
158+ if self.expected not in matchee:
159+ return DoesNotContain(matchee, self.expected)
160+ return None
161+
162+
163 class IsConfiguredBatchNavigator(Matcher):
164 """Check that an object is a batch navigator."""
165
166
167=== modified file 'lib/lp/testing/tests/test_matchers.py'
168--- lib/lp/testing/tests/test_matchers.py 2010-08-20 20:31:18 +0000
169+++ lib/lp/testing/tests/test_matchers.py 2010-10-19 18:54:00 +0000
170@@ -20,6 +20,8 @@
171 from lp.testing import TestCase
172 from lp.testing._webservice import QueryCollector
173 from lp.testing.matchers import (
174+ Contains,
175+ DoesNotContain,
176 DoesNotCorrectlyProvide,
177 DoesNotProvide,
178 DoesNotStartWith,
179@@ -257,3 +259,36 @@
180 matcher = StartsWith("bar")
181 mismatch = matcher.match("foo")
182 self.assertEqual("bar", mismatch.expected)
183+
184+
185+class DoesNotContainTests(TestCase):
186+
187+ def test_describe(self):
188+ mismatch = DoesNotContain("foo", "bar")
189+ self.assertEqual(
190+ "'foo' does not contain 'bar'.", mismatch.describe())
191+
192+
193+class ContainsTests(TestCase):
194+
195+ def test_str(self):
196+ matcher = Contains("bar")
197+ self.assertEqual("Contains 'bar'.", str(matcher))
198+
199+ def test_match(self):
200+ matcher = Contains("bar")
201+ self.assertIs(None, matcher.match("foo bar baz"))
202+
203+ def test_mismatch_returns_does_not_start_with(self):
204+ matcher = Contains("bar")
205+ self.assertIsInstance(matcher.match("foo"), DoesNotContain)
206+
207+ def test_mismatch_sets_matchee(self):
208+ matcher = Contains("bar")
209+ mismatch = matcher.match("foo")
210+ self.assertEqual("foo", mismatch.matchee)
211+
212+ def test_mismatch_sets_expected(self):
213+ matcher = Contains("bar")
214+ mismatch = matcher.match("foo")
215+ self.assertEqual("bar", mismatch.expected)
216
217=== modified file 'utilities/create-lp-wadl-and-apidoc.py'
218--- utilities/create-lp-wadl-and-apidoc.py 2010-10-18 21:53:28 +0000
219+++ utilities/create-lp-wadl-and-apidoc.py 2010-10-19 18:54:00 +0000
220@@ -1,90 +1,93 @@
221 #! /usr/bin/python -S
222 #
223-# Copyright 2009 Canonical Ltd. This software is licensed under the
224+# Copyright 2010 Canonical Ltd. This software is licensed under the
225 # GNU Affero General Public License version 3 (see the file LICENSE).
226
227 """Create a static WADL file describing the current webservice.
228
229-Usage hint:
230+Example:
231
232-% LPCONFIG="edge" utilities/create-lp-wadl.py launchpad-%(version)s.wadl
233+ % LPCONFIG=development bin/py utilities/create-lp-wadl-and-apidoc.py \\
234+ "lib/canonical/launchpad/apidoc/wadl-development-%(version)s.xml"
235 """
236-
237-import _pythonpath
238-
239+import _pythonpath # Not lint, actually needed.
240+
241+import optparse
242 import os
243-import pkg_resources
244-import subprocess
245 import sys
246-import urlparse
247
248 from zope.component import getUtility
249 from zope.pagetemplate.pagetemplatefile import PageTemplateFile
250
251+from canonical.launchpad.rest.wadl import generate_wadl, generate_html
252 from canonical.launchpad.scripts import execute_zcml_for_scripts
253-from canonical.launchpad.webapp.interaction import (
254- ANONYMOUS, setupInteractionByEmail)
255-from canonical.launchpad.webapp.servers import (
256- WebServicePublication, WebServiceTestRequest)
257-from canonical.launchpad.webapp.vhosts import allvhosts
258 from canonical.launchpad.systemhomes import WebServiceApplication
259 from lazr.restful.interfaces import IWebServiceConfiguration
260
261
262-def main(path_template):
263+def write(filename, content):
264+ """Replace the named file with the given string."""
265+ f = open(filename, 'w')
266+ f.write(content)
267+ f.close()
268+
269+
270+def main(path_template, force=False):
271 WebServiceApplication.cached_wadl = None # do not use cached file version
272 execute_zcml_for_scripts()
273 config = getUtility(IWebServiceConfiguration)
274- directory, ignore = os.path.split(path_template)
275-
276- stylesheet = pkg_resources.resource_filename(
277- 'launchpadlib', 'wadl-to-refhtml.xsl')
278+ directory = os.path.dirname(path_template)
279
280 # First, create an index.html with links to all the HTML
281 # documentation files we're about to generate.
282 template_file = 'apidoc-index.pt'
283 template = PageTemplateFile(template_file)
284- f = open(os.path.join(directory, "index.html"), 'w')
285+ index_filename = os.path.join(directory, "index.html")
286+ print "Writing index:", index_filename
287+ f = open(index_filename, 'w')
288 f.write(template(config=config))
289
290- # Request the WADL from the root resource.
291- # We do this by creating a request object asking for a WADL
292- # representation.
293 for version in config.active_versions:
294- url = urlparse.urljoin(allvhosts.configs['api'].rooturl, version)
295- request = WebServiceTestRequest(version=version, environ={
296- 'SERVER_URL': url,
297- 'HTTP_HOST': allvhosts.configs['api'].hostname,
298- 'HTTP_ACCEPT': 'application/vd.sun.wadl+xml'
299- })
300- # We then bypass the usual publisher processing by associating
301- # the request with the WebServicePublication (usually done by the
302- # publisher) and then calling the root resource - retrieved through
303- # getApplication().
304- request.setPublication(WebServicePublication(None))
305- setupInteractionByEmail(ANONYMOUS, request)
306- filename = path_template % {'version' : version}
307- print "Writing WADL for version %s to %s." % (version, filename)
308- f = open(filename, 'w')
309- content = request.publication.getApplication(request)(request)
310- f.write(content)
311- f.close()
312+ wadl_filename = path_template % {'version': version}
313+ # If the WADL file doesn't exist or we're being forced to regenerate
314+ # it...
315+ if (not os.path.exists(wadl_filename) or force):
316+ print "Writing WADL for version %s to %s." % (
317+ version, wadl_filename)
318+ write(wadl_filename, generate_wadl(version))
319+ else:
320+ print "Skipping already present WADL file:", wadl_filename
321
322 # Now, convert the WADL into an human-readable description and
323 # put the HTML in the same directory as the WADL.
324 html_filename = os.path.join(directory, version + ".html")
325- print "Writing apidoc for version %s to %s" % (
326- version, html_filename)
327- stdout = open(html_filename, "w")
328- subprocess.Popen(['xsltproc', stylesheet, filename], stdout=stdout)
329- stdout.close()
330+ # If the HTML file doesn't exist or we're being forced to regenerate
331+ # it...
332+ if (not os.path.exists(html_filename) or force):
333+ print "Writing apidoc for version %s to %s" % (
334+ version, html_filename)
335+ write(html_filename, generate_html(wadl_filename,
336+ suppress_stderr=False))
337+ else:
338+ print "Skipping already present HTML file:", html_filename
339
340 return 0
341
342+
343+def parse_args(args):
344+ usage = "usage: %prog [options] PATH_TEMPLATE"
345+ parser = optparse.OptionParser(usage=usage)
346+ parser.add_option(
347+ "--force", action="store_true",
348+ help="Replace any already-existing files.")
349+ parser.set_defaults(force=False)
350+ options, args = parser.parse_args(args)
351+ if len(args) != 2:
352+ parser.error("A path template is required.")
353+
354+ return options, args
355+
356+
357 if __name__ == '__main__':
358- if len(sys.argv) != 2:
359- print "Usage: %s [WADL path template]" % sys.argv[0]
360- print " Example: %s path/to/wadl/wadl-%%(version).xml" % (
361- sys.argv[0])
362- sys.exit(-1)
363- sys.exit(main(sys.argv[1]))
364+ options, args = parse_args(sys.argv)
365+ sys.exit(main(args[1], options.force))