[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