Message ID | 20110225193252.9630.30937.stgit@localhost6.localdomain6 |
---|---|
State | Superseded |
Headers | show |
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.
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) + 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(), + name=self.getUniqueString(), submitter=submitter, + state=State.objects.get(name='New')) + patch.save() + return patch