----- Original Message -----
From: "Will Woods" <wwoods(a)redhat.com>
To: "anaconda patch review" <anaconda-patches(a)lists.fedorahosted.org>
Sent: Tuesday, September 23, 2014 12:37:22 PM
Subject: Re: [PATCH blivet/master 2/3] devicelibs.lvm: Add
'autobackup' attribute
On Mon, 2014-09-22 at 08:43 -0400, Anne Mulhern wrote:
> > +# This defines whether we want automatic metadata backup or not.
> > +# You probably want this on unless you're doing image builds.
> > +autobackup = True
>
> Probably belongs in flags.py.
Reasonable.
> > +import glob
> > +
>
> Would prefer this import with other Python imports, i.e., unittest import.
Also reasonable.
> > + ##
> > + ## check if autobackup=False worked
> > + ##
> > + self.assertEqual(_old_backups, self._list_backups())
> > +
>
> This test should be in a separate method. The existing method is already
> 200 lines long and you're testing something quite specific here.
Ugh. Yes, but that entire method should be like 17 separate tests. So
should I refactor the whole thing, or just factor out my own test?
Because to do that I'm gonna end up writing extra setup and teardown
functions, and then I might as well just do the rest...
Yes, that method is way too long.
If there's a bunch of extra setUp and teardown stuff that is
necessary for what you're testing to even work that's
stuff that is useful to make explicit in the tests.
-w
_______________________________________________
anaconda-patches mailing list
anaconda-patches(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
- mulhern