diff mbox

[V2] New factory class to create arbitrary model objects, to be used in tests

Message ID 20110414194210.29037.80843.stgit@localhost6.localdomain6
State Superseded
Headers show

Commit Message

Guilherme Salgado April 14, 2011, 7:42 p.m. UTC 
Signed-off-by: Guilherme Salgado <guilherme.salgado@linaro.org>
---

I think there's value in having this factory because some tests will need
either more stuff than is provided by the basic fixture or just different
stuff, and changing the fixture to accomodate the needs of every possible test
is a well known cause of obscure tests: 
    <http://xunitpatterns.com/Obscure%20Test.html#General%20Fixture>

I already have some tests using this and I'm planning to write more for the
'my patches' page that I'm working on, which I hope to submit soon.

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

Comments

Jeremy Kerr April 27, 2011, 3:39 a.m. UTC | #1 
Hi Guilherme,

> I think there's value in having this factory because some tests will need
> either more stuff than is provided by the basic fixture or just different
> stuff, and changing the fixture to accomodate the needs of every possible test
> is a well known cause of obscure tests: 
>     <http://xunitpatterns.com/Obscure%20Test.html#General%20Fixture>
> 
> I already have some tests using this and I'm planning to write more for the
> 'my patches' page that I'm working on, which I hope to submit soon.

OK, this is looking pretty good, but I have a couple of concerns though:

> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2011 Jeremy Kerr <jk@ozlabs.org>

Probably best to add your name here :)

> +
> +    def __init__(self):
> +        # Initialise the unique identifier.
> +        self.integer = count(randint(0, 1000000))

As a general rule, I'm averse to using random data in test cases. We may
see non-repeatable failures here, if we create multiple ObjectFactories
that happen to have colliding unique IDs.

So, perhaps we should make ObjectFactory a singleton (referenced though
an exported module variable, something like 'factory'), and just use:

    self.integer = count()

How does that sound?

> +    def makePatch(self, project=None, submitter=None, date=None):
> +        if date is None:
> +            date = datetime.now()
> +        if project is None:
> +            project = self.makeProject()
> +        if submitter is None:
> +            submitter = self.makePerson()
> +        msgid = email.utils.make_msgid(idstring=self.getUniqueString())
> +        patch = Patch(
> +            project=project, msgid=msgid, name=self.getUniqueString(),
> +            submitter=submitter, date=date,
> +            state=State.objects.get(name='New'))
> +        patch.save()
> +        return patch

Might be good to put a minimal patch in patch.content here.

Cheers,


Jeremy
Guilherme Salgado April 27, 2011, 2:10 p.m. UTC | #2 
Hi Jeremy,

On Wed, 2011-04-27 at 11:39 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > I think there's value in having this factory because some tests will need
> > either more stuff than is provided by the basic fixture or just different
> > stuff, and changing the fixture to accomodate the needs of every possible test
> > is a well known cause of obscure tests: 
> >     <http://xunitpatterns.com/Obscure%20Test.html#General%20Fixture>
> > 
> > I already have some tests using this and I'm planning to write more for the
> > 'my patches' page that I'm working on, which I hope to submit soon.
> 
> OK, this is looking pretty good, but I have a couple of concerns though:
> 
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2011 Jeremy Kerr <jk@ozlabs.org>
> 
> Probably best to add your name here :)

Should I add myself as the author and keep you as the copyright holder?

> 
> > +
> > +    def __init__(self):
> > +        # Initialise the unique identifier.
> > +        self.integer = count(randint(0, 1000000))
> 
> As a general rule, I'm averse to using random data in test cases. We may
> see non-repeatable failures here, if we create multiple ObjectFactories
> that happen to have colliding unique IDs.
> 
> So, perhaps we should make ObjectFactory a singleton (referenced though
> an exported module variable, something like 'factory'), and just use:
> 
>     self.integer = count()
> 
> How does that sound?

I can't think of any reason for not having ObjectFactory as a singleton,
and I think your concern is valid, so consider this done. :)

> 
> > +    def makePatch(self, project=None, submitter=None, date=None):
> > +        if date is None:
> > +            date = datetime.now()
> > +        if project is None:
> > +            project = self.makeProject()
> > +        if submitter is None:
> > +            submitter = self.makePerson()
> > +        msgid = email.utils.make_msgid(idstring=self.getUniqueString())
> > +        patch = Patch(
> > +            project=project, msgid=msgid, name=self.getUniqueString(),
> > +            submitter=submitter, date=date,
> > +            state=State.objects.get(name='New'))
> > +        patch.save()
> > +        return patch
> 
> Might be good to put a minimal patch in patch.content here.

Indeed. I've added 'content' as an optional argument and when it's not
passed I set it to a minimal patch.

I'll submit a third version with these changes as soon as I have an
answer from you about the copyright/authorship question. Oh, and I'll
also submit new versions of the patches that use this as they now use
the factory singleton.

Cheers,
Jeremy Kerr April 29, 2011, 1:51 a.m. UTC | #3 
Hi Guilherme,

> > > +# Patchwork - automated patch tracking system
> > > +# Copyright (C) 2011 Jeremy Kerr <jk@ozlabs.org>
> > 
> > Probably best to add your name here :)
> 
> Should I add myself as the author and keep you as the copyright holder?

Since I haven't written anything in that file, best just to put your
name there.

> > 
> > > +
> > > +    def __init__(self):
> > > +        # Initialise the unique identifier.
> > > +        self.integer = count(randint(0, 1000000))
> > 
> > As a general rule, I'm averse to using random data in test cases. We may
> > see non-repeatable failures here, if we create multiple ObjectFactories
> > that happen to have colliding unique IDs.
> > 
> > So, perhaps we should make ObjectFactory a singleton (referenced though
> > an exported module variable, something like 'factory'), and just use:
> > 
> >     self.integer = count()
> > 
> > How does that sound?
> 
> I can't think of any reason for not having ObjectFactory as a singleton,
> and I think your concern is valid, so consider this done. :)

OK, great.

> Indeed. I've added 'content' as an optional argument and when it's not
> passed I set it to a minimal patch.
> 
> I'll submit a third version with these changes as soon as I have an
> answer from you about the copyright/authorship question. Oh, and I'll
> also submit new versions of the patches that use this as they now use
> the factory singleton.

Sounds good, thanks. When you re-roll this patch, could you avoid
breaking lines right after the opening bracket? So instead of:

        person = Person(
            email=self.getUniqueEmailAddress(), name=self.getUniqueString())

We get:

        person = Person(email=self.getUniqueEmailAddress(),
                        name=self.getUniqueString())

Also, most of the codebase has spaces around the '=' in kwargs:

        person = Person(email = self.getUniqueEmailAddress(),
                        name = self.getUniqueString())

- I'm aware that this is not what PEP-8 says, but I'd prefer to keep it
consistent, and perhaps do a global change at some point.

Also, the initial state is defined as one with ordering = 0:

+        patch = Patch(
+            project=project, msgid=msgid, name=self.getUniqueString(),
+            submitter=submitter, date=date,
+            state=State.objects.get(name='New'))

This should be:

	state = State.objects.get(ordering = 0)

- as there may not be a 'New' state in a particular patchwork setup.
Even better, you can leave 'state' unset, and the Patch.save() will set
it to this default.

Cheers,


Jeremy
Guilherme Salgado April 29, 2011, 1:07 p.m. UTC | #4 
Hi Jeremy,

On Fri, 2011-04-29 at 09:51 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > > > +# Patchwork - automated patch tracking system
> > > > +# Copyright (C) 2011 Jeremy Kerr <jk@ozlabs.org>
> > > 
> > > Probably best to add your name here :)
> > 
> > Should I add myself as the author and keep you as the copyright holder?
> 
> Since I haven't written anything in that file, best just to put your
> name there.

Ok, I've done that.

> 
> > > 
> > > > +
> > > > +    def __init__(self):
> > > > +        # Initialise the unique identifier.
> > > > +        self.integer = count(randint(0, 1000000))
> > > 
> > > As a general rule, I'm averse to using random data in test cases. We may
> > > see non-repeatable failures here, if we create multiple ObjectFactories
> > > that happen to have colliding unique IDs.
> > > 
> > > So, perhaps we should make ObjectFactory a singleton (referenced though
> > > an exported module variable, something like 'factory'), and just use:
> > > 
> > >     self.integer = count()
> > > 
> > > How does that sound?
> > 
> > I can't think of any reason for not having ObjectFactory as a singleton,
> > and I think your concern is valid, so consider this done. :)
> 
> OK, great.
> 
> > Indeed. I've added 'content' as an optional argument and when it's not
> > passed I set it to a minimal patch.
> > 
> > I'll submit a third version with these changes as soon as I have an
> > answer from you about the copyright/authorship question. Oh, and I'll
> > also submit new versions of the patches that use this as they now use
> > the factory singleton.
> 
> Sounds good, thanks. When you re-roll this patch, could you avoid
> breaking lines right after the opening bracket? So instead of:
> 
>         person = Person(
>             email=self.getUniqueEmailAddress(), name=self.getUniqueString())
> 
> We get:
> 
>         person = Person(email=self.getUniqueEmailAddress(),
>                         name=self.getUniqueString())
> 
> Also, most of the codebase has spaces around the '=' in kwargs:
> 
>         person = Person(email = self.getUniqueEmailAddress(),
>                         name = self.getUniqueString())
> 
> - I'm aware that this is not what PEP-8 says, but I'd prefer to keep it
> consistent, and perhaps do a global change at some point.

Ok, I've changed this and will keep it in mind for future patches.

> 
> Also, the initial state is defined as one with ordering = 0:
> 
> +        patch = Patch(
> +            project=project, msgid=msgid, name=self.getUniqueString(),
> +            submitter=submitter, date=date,
> +            state=State.objects.get(name='New'))
> 
> This should be:
> 
> 	state = State.objects.get(ordering = 0)
> 
> - as there may not be a 'New' state in a particular patchwork setup.
> Even better, you can leave 'state' unset, and the Patch.save() will set
> it to this default.

Cool, I've removed that, then.  Will send an updated patch shortly.

Cheers,
diff mbox

Patch

diff --git a/apps/patchwork/tests/factory.py b/apps/patchwork/tests/factory.py
new file mode 100644
index 0000000..a750e85
--- /dev/null
+++ b/apps/patchwork/tests/factory.py
@@ -0,0 +1,111 @@ 
+# 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 datetime import datetime
+import email
+from itertools import count
+from random import randint
+
+from django.contrib.auth.models import User
+
+from patchwork.models import Bundle, 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):
+        person = Person(
+            email=self.getUniqueEmailAddress(), name=self.getUniqueString())
+        person.save()
+        if is_user:
+            # Must create the user after the person is created or else
+            # create_user() will trigger the creation of a person.
+            user = User.objects.create_user(
+                person.name, person.email, password=None)
+            user.save()
+        # Re-fetch the person object so that our callsite sees the link to the
+        # newly created user.
+        person = Person.objects.get(email=person.email)
+        return person
+
+    def makeBundle(self, patches=None):
+        if patches is None:
+            patches = [self.makePatch()]
+        bundle = Bundle(
+            owner=self.makeUser(), project=self.makeProject(),
+            name=self.getUniqueString())
+        bundle.save()
+        for patch in patches:
+            bundle.append_patch(patch)
+        bundle.save()
+        return bundle
+
+    def makePatch(self, project=None, submitter=None, date=None):
+        if date is None:
+            date = datetime.now()
+        if project is None:
+            project = self.makeProject()
+        if submitter is None:
+            submitter = self.makePerson()
+        msgid = email.utils.make_msgid(idstring=self.getUniqueString())
+        patch = Patch(
+            project=project, msgid=msgid, name=self.getUniqueString(),
+            submitter=submitter, date=date,
+            state=State.objects.get(name='New'))
+        patch.save()
+        return patch