[PATCH blivet/master 2/3] devicelibs.lvm: Add 'autobackup' attribute
Anne Mulhern
amulhern at redhat.com
Tue Sep 23 18:59:07 UTC 2014
----- Original Message -----
> From: "Will Woods" <wwoods at redhat.com>
> To: "anaconda patch review" <anaconda-patches at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
- mulhern
More information about the anaconda-patches
mailing list