New factory which creates arbitrary model objects to be used in tests.

---
If there's interest in this I'd be happy to move the stuff from
patchwork/tests/utils.py to here and change tests to use the factory.

 apps/patchwork/tests/factory.py |   88 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 88 insertions(+), 0 deletions(-)
 create mode 100644 apps/patchwork/tests/factory.py

On Fri, Feb 25, 2011 at 04:35:51PM -0300, Guilherme Salgado wrote:
> 
> ---
> If there's interest in this I'd be happy to move the stuff from
> patchwork/tests/utils.py to here and change tests to use the factory.
> 
>  apps/patchwork/tests/factory.py |   88 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 88 insertions(+), 0 deletions(-)
>  create mode 100644 apps/patchwork/tests/factory.py
> 
> diff --git a/apps/patchwork/tests/factory.py b/apps/patchwork/tests/factory.py
> new file mode 100644
> index 0000000..9aa5ec3
> --- /dev/null
> +++ b/apps/patchwork/tests/factory.py
> @@ -0,0 +1,88 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2011 Jeremy Kerr <jk@ozlabs.org>
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +from itertools import count
> +from random import randint
> +
> +from django.contrib.auth.models import User
> +
> +from patchwork.models import Patch, Person, Project, State
> +
> +
> +class ObjectFactory(object):
> +    """Factory methods for creating basic Python objects."""
> +
> +    def __init__(self):
> +        # Initialise the unique identifier.
> +        self.integer = count(randint(0, 1000000))
> +
> +    def getUniqueEmailAddress(self):
> +        return "%s@example.com" % self.getUniqueString('email')
> +
> +    def getUniqueString(self, prefix=None):
> +        """Return a string unique to this factory instance.
> +
> +        :param prefix: Used as a prefix for the unique string. If unspecified,
> +            defaults to 'generic-string'.
> +        """
> +        if prefix is None:
> +            prefix = "generic-string"
> +        string = "%s%s" % (prefix, self.getUniqueInteger())
> +        return string.lower()
> +
> +    def getUniqueInteger(self):
> +        """Return an integer unique to this factory instance."""
> +        return self.integer.next()
> +
> +    def makeUser(self):
> +        userid = password = self.getUniqueString()
> +        user = User.objects.create_user(
> +            userid, self.getUniqueEmailAddress(), password)
> +        user.save()
> +        return user
> +
> +    def makeProject(self):
> +        name = self.getUniqueString()
> +        project = Project(
> +            linkname=name, name=name,
> +            listid=self.getUniqueString(),
> +            listemail=self.getUniqueEmailAddress())
> +        project.save()
> +        return project
> +
> +    def makePerson(self, is_user=True):
> +        if is_user:
> +            user = self.makeUser()
> +        person = Person(
> +            email=self.getUniqueEmailAddress(), name=self.getUniqueString(),
> +            user=user)
                     ^^^ might not exist


> +        person.save()
> +        return person
> +
> +    def makePatch(self, project=None, submitter=None):
> +        if project is None:
> +            project = self.makeProject()
> +        if submitter is None:
> +            submitter = self.makePerson()
> +        patch = Patch(
> +            project=project, msgid=self.getUniqueString(),
The email package has a helper to format msgids in 
    email.utils.make_msgid()

> +            name=self.getUniqueString(), submitter=submitter,
> +            state=State.objects.get(name='New'))
> +        patch.save()
> +        return patch

I guess you want to solve the problem of creating an initial db state.
I personally would prefer a fixture that creates a state with more
reasonable names like:
    TestProjectA
    TestProjectB
    TestUserA
    TestUserB
    TestMaintainer
    etc and/or similar
That would make it much easier to inspect than arbitrary numbers (eg in
test mails).
Maybe have a TestFixtureMixIn class that has a 'fixtures' attribute and
that makes those models available as properties (wrap the lookup).
I assume that would cover most of the testing needs and clients would
not have to create it themselves.

On Sat, 2011-02-26 at 10:05 +0100, Dirk Wallenstein wrote:
> On Fri, Feb 25, 2011 at 04:35:51PM -0300, Guilherme Salgado wrote:
> > 
> > ---
> > If there's interest in this I'd be happy to move the stuff from
> > patchwork/tests/utils.py to here and change tests to use the factory.
> > 
[...]
> > +    def makePerson(self, is_user=True):
> > +        if is_user:
> > +            user = self.makeUser()
> > +        person = Person(
> > +            email=self.getUniqueEmailAddress(), name=self.getUniqueString(),
> > +            user=user)
>                      ^^^ might not exist

Oops, good catch, I'll fix it.

> 
> 
> > +        person.save()
> > +        return person
> > +
> > +    def makePatch(self, project=None, submitter=None):
> > +        if project is None:
> > +            project = self.makeProject()
> > +        if submitter is None:
> > +            submitter = self.makePerson()
> > +        patch = Patch(
> > +            project=project, msgid=self.getUniqueString(),
> The email package has a helper to format msgids in 
>     email.utils.make_msgid()

Oh, cool.  I'll use that and feed a unique string to it to "strengthen
the uniqueness of the message id". :)

> 
> > +            name=self.getUniqueString(), submitter=submitter,
> > +            state=State.objects.get(name='New'))
> > +        patch.save()
> > +        return patch
> 
> I guess you want to solve the problem of creating an initial db state.
> I personally would prefer a fixture that creates a state with more
> reasonable names like:
>     TestProjectA
>     TestProjectB
>     TestUserA
>     TestUserB
>     TestMaintainer
>     etc and/or similar
> That would make it much easier to inspect than arbitrary numbers (eg in
> test mails).
> Maybe have a TestFixtureMixIn class that has a 'fixtures' attribute and
> that makes those models available as properties (wrap the lookup).
> I assume that would cover most of the testing needs and clients would
> not have to create it themselves.

There are a few reasons why I didn't go down that route:

First, having a fixture definition separated from the tests themselves
make the test less readable as you have to lookup the fixture to see
what data is being made available to the test.

Second, sharing a single fixture between multiple tests, although
probably a time-saver in the short term, will lead to less maintainable
tests in the long term. That's because most tests would certainly depend
on just a subset of the fixture and it's very hard to tell what's that
subset and whether or not the rest of the fixture affects the test in
some way. The most common annoyance you'll see when you have a shared
fixture is tests breaking when you add new stuff to the fixture.
http://xunitpatterns.com/Shared%20Fixture.html has more info about
shared fixtures and when to use them.  I think shared fixtures work fine
if you have tests that need lots of data in the DB and don't share the
fixture between too many tests, but that doesn't seem to be the case
here.

Recently I've worked on a project which had a shared fixture and it was
very painful to maintain the tests that relied on it, so we stopped
using that and started having our tests define their own fixture. It
made our tests more verbose but a lot more maintainable/readable.
That's why I avoided the shared fixture pattern this time.

Cheers,

On Mon, Feb 28, 2011 at 09:32:33AM -0300, Guilherme Salgado wrote:
> On Sat, 2011-02-26 at 10:05 +0100, Dirk Wallenstein wrote:
> > On Fri, Feb 25, 2011 at 04:35:51PM -0300, Guilherme Salgado wrote:
> > > 
> > > ---
> > > If there's interest in this I'd be happy to move the stuff from
> > > patchwork/tests/utils.py to here and change tests to use the factory.
> > > 
> [...]
> > > +    def makePerson(self, is_user=True):
> > > +        if is_user:
> > > +            user = self.makeUser()
> > > +        person = Person(
> > > +            email=self.getUniqueEmailAddress(), name=self.getUniqueString(),
> > > +            user=user)
> >                      ^^^ might not exist
> 
> Oops, good catch, I'll fix it.
> 
> > 
> > 
> > > +        person.save()
> > > +        return person
> > > +
> > > +    def makePatch(self, project=None, submitter=None):
> > > +        if project is None:
> > > +            project = self.makeProject()
> > > +        if submitter is None:
> > > +            submitter = self.makePerson()
> > > +        patch = Patch(
> > > +            project=project, msgid=self.getUniqueString(),
> > The email package has a helper to format msgids in 
> >     email.utils.make_msgid()
> 
> Oh, cool.  I'll use that and feed a unique string to it to "strengthen
> the uniqueness of the message id". :)
> 
> > 
> > > +            name=self.getUniqueString(), submitter=submitter,
> > > +            state=State.objects.get(name='New'))
> > > +        patch.save()
> > > +        return patch
> > 
> > I guess you want to solve the problem of creating an initial db state.
> > I personally would prefer a fixture that creates a state with more
> > reasonable names like:
> >     TestProjectA
> >     TestProjectB
> >     TestUserA
> >     TestUserB
> >     TestMaintainer
> >     etc and/or similar
> > That would make it much easier to inspect than arbitrary numbers (eg in
> > test mails).
> > Maybe have a TestFixtureMixIn class that has a 'fixtures' attribute and
> > that makes those models available as properties (wrap the lookup).
> > I assume that would cover most of the testing needs and clients would
> > not have to create it themselves.
> 
> There are a few reasons why I didn't go down that route:
> 
> First, having a fixture definition separated from the tests themselves
> make the test less readable as you have to lookup the fixture to see
> what data is being made available to the test.
> 
> Second, sharing a single fixture between multiple tests, although
> probably a time-saver in the short term, will lead to less maintainable
> tests in the long term. That's because most tests would certainly depend
> on just a subset of the fixture and it's very hard to tell what's that
> subset and whether or not the rest of the fixture affects the test in
> some way. The most common annoyance you'll see when you have a shared
> fixture is tests breaking when you add new stuff to the fixture.
> http://xunitpatterns.com/Shared%20Fixture.html has more info about
> shared fixtures and when to use them.  I think shared fixtures work fine
> if you have tests that need lots of data in the DB and don't share the
> fixture between too many tests, but that doesn't seem to be the case
> here.
> 
> Recently I've worked on a project which had a shared fixture and it was
> very painful to maintain the tests that relied on it, so we stopped
> using that and started having our tests define their own fixture. It
> made our tests more verbose but a lot more maintainable/readable.
> That's why I avoided the shared fixture pattern this time.

Well, in short, the idea was to provide a foundation of things one usually
doesn't care about what the specific settings are -- when you just need
a project, a user, a maintainer.  If you need specific settings you can
still make incremental changes to the settings you want to test.
Changes will be undone after each test.

On Mon, 2011-02-28 at 14:04 +0100, Dirk Wallenstein wrote:
> On Mon, Feb 28, 2011 at 09:32:33AM -0300, Guilherme Salgado wrote:
> > On Sat, 2011-02-26 at 10:05 +0100, Dirk Wallenstein wrote:
> > > On Fri, Feb 25, 2011 at 04:35:51PM -0300, Guilherme Salgado wrote:
[...]
> > > I guess you want to solve the problem of creating an initial db state.
> > > I personally would prefer a fixture that creates a state with more
> > > reasonable names like:
> > >     TestProjectA
> > >     TestProjectB
> > >     TestUserA
> > >     TestUserB
> > >     TestMaintainer
> > >     etc and/or similar
> > > That would make it much easier to inspect than arbitrary numbers (eg in
> > > test mails).
> > > Maybe have a TestFixtureMixIn class that has a 'fixtures' attribute and
> > > that makes those models available as properties (wrap the lookup).
> > > I assume that would cover most of the testing needs and clients would
> > > not have to create it themselves.
> > 
> > There are a few reasons why I didn't go down that route:
> > 
> > First, having a fixture definition separated from the tests themselves
> > make the test less readable as you have to lookup the fixture to see
> > what data is being made available to the test.
> > 
> > Second, sharing a single fixture between multiple tests, although
> > probably a time-saver in the short term, will lead to less maintainable
> > tests in the long term. That's because most tests would certainly depend
> > on just a subset of the fixture and it's very hard to tell what's that
> > subset and whether or not the rest of the fixture affects the test in
> > some way. The most common annoyance you'll see when you have a shared
> > fixture is tests breaking when you add new stuff to the fixture.
> > http://xunitpatterns.com/Shared%20Fixture.html has more info about
> > shared fixtures and when to use them.  I think shared fixtures work fine
> > if you have tests that need lots of data in the DB and don't share the
> > fixture between too many tests, but that doesn't seem to be the case
> > here.
> > 
> > Recently I've worked on a project which had a shared fixture and it was
> > very painful to maintain the tests that relied on it, so we stopped
> > using that and started having our tests define their own fixture. It
> > made our tests more verbose but a lot more maintainable/readable.
> > That's why I avoided the shared fixture pattern this time.
> 
> Well, in short, the idea was to provide a foundation of things one usually
> doesn't care about what the specific settings are -- when you just need
> a project, a user, a maintainer.  If you need specific settings you can
> still make incremental changes to the settings you want to test.

Oh, sure, that sounds fine, but as you said some tests would still need
to create more objects and that's where the factory is useful.