[PATCH V2] New factory class to create arbitrary model objects, to be used in tests
guilherme.salgado at linaro.org
Fri Apr 29 23:07:10 EST 2011
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 at 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(),
> 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.
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 198 bytes
Desc: This is a digitally signed message part
More information about the Patchwork