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

[Guilherme Salgado]

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 at 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,


-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110427/7a3c5996/attachment.pgp>