Code review comment for lp:~gary/launchpad/bug650343

Revision history for this message
Māris Fogels (mars) wrote :

Hi Gary,

The code for this change looks good to land. r=mars. I have a few questions about the tests though:

  * Do we want to silence the log warnings in the doctest output on line 26 of the diff? They look really odd. If we can not silence them, we could at least explain why they were left in there, and why there are four of them.

  * We need some way to move tests like this into lib/lp. The problem is that this test lives in canonical/launchpad/doc, which runs in a different layer than the pagetests (LaunchpadFunctionalLayer I believe), and I do not think we have a place yet in the lib/lp that runs tests at this level.

This looks good to me. r=mars.

Maris

review: Approve

« Back to merge proposal