[qmtest] [PATCH] Run metadata and first pass at SQL results storage.

[Nathaniel Smith]

On Wed, Jul 02, 2003 at 11:20:59PM -0700, Mark Mitchell wrote:
> +	* qm/extension.py (get_class_arguments): Mark's fixes to
> +	support diamond inheritance.
> 
> This is too cryptic.  If you really want to give me credit, put my
> name in the ChangeLog entry.  But there's no need for that -- just say
> "Support diamond inheritance."

Fixed.

> +    'time_secs' -- the time to be formatted, as returned by
> +                   e.g. 'time.time()'."""
> 
> We don't indent the second line: just write:
> 
>    'time_secs' -- the time to be formatted, as returned by
>    e.g. 'time.time()'."""

Hmm.  I feel like I must have done this other places, too, but I'll
watch out for it.  (Fixed here.)

> +class Connection:
> +    """A little wrapper around a DB 2.0 connection that preserves a
> +    reference to the containing module, and provides a minimal
> +    interface to said connection.  This is useful because it gives us
> +    a hook to attach our SQL-quoting code in to."""
> 
> Every class or method should have a 1-line description and then some
> details, like so:
> 
>    """A wrapper around a DB 2.0 connection.
> 
>    An instance of 'Connection' preserves a reference to the containing
>    module, and provides a minimal interface to said connection.  This
>    is useful because it gives us a hook to attach our SQL-quoting code
>    in to."""
> 
> But then, showing massive amounts of uptightness, we'd fix this
> because (a) we don't use "this" as a noun, only as an ajdective, and
> (b) we avoid the first person in comments.  Unfortunately, these
> commandments have not always been obeyed. :-(  But, we want something
> like the following for the second sentence:
> 
>   In addition, a 'Connection' quotes SQL queries as necessary for the
>   underlying DB 2.0 connection.

Whoops, right.  This file was written a few weeks ago, before I had
picked up some of this stuff, and I missed it when cleaning up the
formatting this evening.  Fixed.

> +def connect(modname, *args, **moreargs):
> 
> Why not make this the __init__ method for Connection?

Eh, why not indeed?  Fixed.

> +                        ["dummy element to make __import__ behave"])
> 
> Wasteful: should just be [""] with a comment:
> 
>   # There must be at least one element or __import__ will crash.

I'm not entirely sure how that's less wasteful (I guess a touch of
memory?), but sure.  That way I can also make it more informative,
because this is a really weird quirk of __import__.

        # Last argument must be a non-empty list or __import__ will
        # return the wrong module.

(You see, __import__("foo.bar", ...) returns a reference to foo if the
last argument is an empty list (the default), and foo.bar otherwise.
This is the only effect of the last argument.  No, really, you can
check the docs, it's true!)

> +def quotestr(string):
> +    """Quotes a string for SQL."""
> +
> +    # Replace each ' with '', then surround with more 's.  Also double
> +    # backslashes.  It'd be nice to handle things like quoting
> non-ASCII
> +    # characters (by replacing them with octal escapes), but we don't.
> +    return "'" + string.replace("'", "''").replace("\\", "\\\\") + "'"
> 
> Two comments:
> 
> (1) Does it make sense to make this a member of Connection?

Interesting question.  I think not, because it has no association with
any particular connection, it's just a way to quote SQL strings.  And
it's already in the SQL support module's namespace.

> (2) The:
> 
>     'string' -- ???
> 
>     returns -- ???
> 
>     comments are missing.

Fixed.  Also cleaned up the formatting in the rest of the module a bit
more...

> +# A nice subtlety is that because of how extension classes are loaded,
> 
> Let's not be editorial: it's a subtlety, but not a nice one.

Fair enough :-).  Fixed.

> +_annotation_sentinel = None
> 
> Shouldn't these be packaged up into a mix-in for
> PickleResult{Stream,Reader}?

That seems excessive cleverness.  They're already in a private
(module) namespace...

> +    "Version 1", and all later versions, contain a pickled version
> +    number as the first thing in the file.  In version 1, this is
> 
> Thanks for spelling this out!
> 
> +        elif version == 1:
> +            self._ReadMetadata()
> 
> I'm not sure why we read this at __init__ time rather than lazily when
> GetAnnotations is called, but that can be changed easily later if it
> needs to be.

I'm lazier than it is, I guess.  But yes, let's wait for a reason
before adding the complexity.

> +class SQLConnected(Extension):
> +    """Mixin class for classes that need a database connection."""
> 
> Shouldn't this be a module-private class? (e.g., _SQL_Connected?)

Sure, that's a bit more aesthetic.  Fixed.

> +        for result_id, result_kind, key, value in self._a_buffer:
> +            if (result_id, result_kind) != (id, kind):
> +                self._a_buffer.rewind()
> +                break
> 
> Interesting -- I see that you avoid a separate SELECT per result.

And even a separate FETCH with the buffering stuff, yeah.  Makes
things work much faster, while retaining scalability.

> +class XMLResultReader(FileResultReader):
> 
> Should this go in xml_result_stream.py?  (It seems to make sense to
> group the readers and writers together?)

I was wondering about this, since both the pickle and SQL streams went
that way.  Sure, fixed.

> +                rs.WriteAnnotation("qmtest.run.end_time", end_time_str)
> 
> That's a slight extension of the "foo.bar" format we've used until
> now; we've never had two "." characters in an annotation.  I think
> that's OK, though; it's a natural generalization and nothing depends
> on that.

Nod.  I just wanted to avoid polluting the top-level namespace too
much, considering we're likely to be stuck forevermore with whatever
we do now...

>      A 'StorageResultsStream' does not write any output.  It simply
>      stores the results for future display."""
> 
> -    def __init__(self):
> +    def __init__(self, arguments):
> 
> What's the rationale for this change?

Mostly it's because I made all the necessary changes for the first
version of the patch, when 'ResultStream' took annotations as
arguments.  But I left it in because it wasn't any work to do so, and
it makes 'StorageResultStream' consistent with every other extension
class we have.  Could take it out again if you like, but it seemed
like a zero-loss/small-win sort of thing.

> +        # Put in a note.  No software currently pays any attention to
> +        # this key, but it's useful to mark runs that were done from
> +        # the GUI, because they may be an amalgamation of multiple
> +        # runs, and therefore cannot be trusted to describe a single
> +        # version of the software under test.
> +        self.__annotations = { "qmtest.created_from_gui": "true" }
> 
> Let's leave this out for now.  It's not that I disagree, but I'm also
> not sure that I agree, and we might want some kind of more general
> "where did this run come from?" field.

Hmm.  Does it hurt to leave it in, though?  It's not intended as part
of any grand scheme, and doesn't preclude any grand schemes of the
future; in fact, as the comment says, it currently does absolutely
nothing.  It's just that I wanted to make sure that when someone's
trying to track down the history of some bug, six months after the
fact in a database that has perhaps been used by some of the less
perfect mortals among us, they would have the possiblity of saying
"wait a minute -- this data might be lying to me".  This seemed an
effective and innocuous way to achieve that.

I should sleep, I'm starting to wax lyrical.  I'll commit tomorrow...

And I suppose we should talk some about the writing of external test
suite wrappers; seems to me that the biggest problem is that there's
really no way to represent the dependencies at the moment.  The
build scripts are run as tests, but we need them to "act like
resources" when it comes time to run the tests in their test suites...
I suppose we could just ignore that for now and accept that it won't
work to run certain subsets of the tests, but we should at least
decide...

-- Nathaniel

-- 
Eternity is very long, especially towards the end.
  -- Woody Allen