Merge lp:~jameinel/loggerhead/pygment-annotate-fail into lp:loggerhead

[John A Meinel]

Fix an "annotate" test that fails when pygments is installed.

I was running the test suite on another machine, and it seems having pygments installed causes on of the annotate tests to fail.

The point of the test was to assert that we properly escape the file content on the annotate page. It was doing so by making sure the text "with&lt;htmlspecialchars" was present. However, when pygments is installed, it highlights the "&lt;" with custom <span> entries.

So this changes it to just use a regex that can match either way. I've tested it on both machines, and the test still passes.

[Benji York (benji)]

The "with.*&lt;.*htmlspecialchars" regex feels a bit too liberal. How
about something like this?

    # remove any pygments-inserted tags
    body = re.sub('<[^>]+>', '', res.body)
    self.assertContains(body, "with&lt;htmlspecialchars")

Also, I suspect judicious use of a clean Python and buildout would help
with this scenario, providing reproducible builds isolated from the
environment. If you're interested I'd be glad to help craft a buildout
for the project.

Regardless, the branch is certainly approvable as is, so I've done so.
Note that this is a mentored review so Brad will be reviewing my review
and giving the final approval.

review: Approve (code*)

[Brad Crittenden (bac)]

Hi John,

s/pigments/pygments/

Your comment at 37 is a bit misleading as it only happens if pygments is installed. Perhaps reword that statement to remove references to "now" since it isn't an on-going issue but only if pygments is installed.

I like Benji's suggestion to clean up the results rather than match something that is overly broad.

review: Approve (code)

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/11/2011 2:00 PM, Brad Crittenden wrote:
> Review: Approve code
> Hi John,
>
> s/pigments/pygments/
>
> Your comment at 37 is a bit misleading as it only happens if pygments is installed. Perhaps reword that statement to remove references to "now" since it isn't an on-going issue but only if pygments is installed.
>
> I like Benji's suggestion to clean up the results rather than match something that is overly broad.

Thanks. I went with just stripping all <span class="pyg-."> entries and
all </span> entries. Which gives the same basic results.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1Vt+cACgkQJdeBCYSNAANxEwCgu2nuYuQYgzTANHdQCN6MnbfK
r40AoMuWFmgQNBY/zVYG3WKlqKwIKUES
=5992
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/11/2011 12:29 PM, Benji York wrote:
> Review: Approve code*
> The "with.*&lt;.*htmlspecialchars" regex feels a bit too liberal. How
> about something like this?
>
> # remove any pygments-inserted tags
> body = re.sub('<[^>]+>', '', res.body)
> self.assertContains(body, "with&lt;htmlspecialchars")
>
> Also, I suspect judicious use of a clean Python and buildout would help
> with this scenario, providing reproducible builds isolated from the
> environment. If you're interested I'd be glad to help craft a buildout
> for the project.

The thing is, we *want* pygments to be installed for Loggerhead, so that
it can highlight source code when you are looking at the View content pages.

It is just a soft dependency. If we had a PQM testing loggerhead trunk,
we would want it to be there.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1VuG4ACgkQJdeBCYSNAAPu4QCg07Wxew98U8Xgsm+xIaMZzpPQ
hUoAoL7jF1N5qTOUCdeyQWz/Y/3esJoN
=G10t
-----END PGP SIGNATURE-----

=== modified file 'loggerhead/tests/test_simple.py'
--- loggerhead/tests/test_simple.py	2009-06-08 23:02:49 +0000
+++ loggerhead/tests/test_simple.py	2011-02-10 22:20:26 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2008, 2009 Canonical Ltd.
+# Copyright (C) 2007-2011 Canonical Ltd.
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -61,7 +61,7 @@
                              'with<htmlspecialchars\n')
         self.build_tree_contents(
             [('myfilename', self.filecontents)])
-        self.tree.add('myfilename')
+        self.tree.add('myfilename', 'myfile-id')
         self.fileid = self.tree.path2id('myfilename')
         self.msg = 'a very exciting commit message <'
         self.revid = self.tree.commit(message=self.msg)
@@ -71,6 +71,11 @@
         res = app.get('/changes')
         res.mustcontain(cgi.escape(self.msg))
 
+    def test_changes_for_file(self):
+        app = self.setUpLoggerhead()
+        res = app.get('/changes?filter_file_id=myfile-id')
+        res.mustcontain(cgi.escape(self.msg))
+
     def test_changes_branch_from(self):
         app = self.setUpLoggerhead(served_url="lp:loggerhead")
         res = app.get('/changes')
@@ -88,8 +93,18 @@
     def test_annotate(self):
         app = self.setUpLoggerhead()
         res = app.get('/annotate', params={'file_id': self.fileid})
-        for line in self.filecontents.splitlines():
-            res.mustcontain(cgi.escape(line))
+        # This now fails because pigments is highlighting the lines.
+        # Specifically, instead of getting:
+        # 'with&lt;htmlspecialchars' we are now getting
+        # '<span class='pyg-n'>with</span><span class='pyg-o'>&lt;</span>'
+        # '<span class='pyg-n'>htmlspecialchars</span>
+        # So pygments is breaking up the special characters with custom
+        # highlighting. The alternative is to figure out how to disable
+        # pygments for this test.
+        ## for line in self.filecontents.splitlines():
+        ##     res.mustcontain(cgi.escape(line))
+        self.assertContainsRe(res.body,
+            "with.*&lt;.*htmlspecialchars")
 
     def test_inventory(self):
         app = self.setUpLoggerhead()