launchpad-dev team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

On Sun, Jun 26, 2011 at 10:49 AM, Jamu Kakar <jkakar@xxxxxxxx> wrote:
> Hi,
>
> On Wed, Jun 22, 2011 at 9:42 PM, Francis J. Lacoste
> <francis.lacoste@xxxxxxxxxxxxx> wrote:
>> 2) When "tuning" queries, please leave in comments in the code! There
>> was not comment here and thought naively that I should get rid of the
>> extra query to get the archive ids and use a join instead. Bad bad idea
>> it seemed. A comment explaining this non-intuitive query would have
>> saved me re-learning that already learned lesson :-)
>
> Even better is to write a test.  You can use Storm's tracing logic to
> capture queries.  For example:
>
> from cStringIO import StringIO
> from storm.tracer import debug
>
> def test_optimized_query(self):
>    """
>    A nice docstring explaining the special query you've written and why it
>    needs to be written as such.
>    """
>    stream = StringIO()
>    debug(True, stream)
>    try:
>        # Do a thing that runs your optimized query.
>    finally:
>        debug(False)
>    self.assertIn("<optimized query>", stream.getvalue())
>
> In the Landscape code base there's a helper method called
> capture_statements (or something, I can't quite remember) that makes
> this easier (you don't need the try/finally block).  There's also an
> assertSQLIn method that makes comparing the actual query to what you
> expect a bit easier (differences in whitespace can be annoying).
> Anyway, it shouldn't be hard to cook up a nice way to do this kind of
> thing.

I don't think tests for this are useful: they tend to be overly
sensitive in some ways, undersensitive in others.

They are overly sensitive because they are doing string logic on the
compiled output; changes in case, field ordering, object ids etc will
affect them.

They are under sensitive because someone that changes the
implementation won't see the comment until *after* they have made the
change. For instance, say you have a function that queries today and
someone writes a helper to consolidate some stuff. They would not see
a comment (unless you comment *and* test), and they would probably
just delete-without-deep-reading all the tests that test the exact sql
issued by the prior query: after all, they no longer query from the
function.

(Oh, and for capturing queries in LP, we've shinier facilities than
debug - we capture structured data, the serialised query, order, time,
query params... :))

-Rob