Re: [PATCH v7 07/11] iotests: add findtests.py

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com,
	den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v7 07/11] iotests: add findtests.py
Date: Fri, 22 Jan 2021 14:34:50 +0100	[thread overview]
Message-ID: <20210122133450.GF15866@merkur.fritz.box> (raw)
In-Reply-To: <f2db3922-6832-25a5-7bb9-ac24dbb32286@virtuozzo.com>

Am 22.01.2021 um 14:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.01.2021 15:45, Kevin Wolf wrote:
> > Am 22.01.2021 um 12:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 22.01.2021 14:48, Kevin Wolf wrote:
> > > > Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > +    def add_group_file(self, fname: str) -> None:
> > > > > +        with open(fname) as f:
> > > > > +            for line in f:
> > > > > +                line = line.strip()
> > > > > +
> > > > > +                if (not line) or line[0] == '#':
> > > > > +                    continue
> > > > > +
> > > > > +                words = line.split()
> > > > > +                test_file = self.parse_test_name(words[0])
> > > > > +                groups = words[1:]
> > > > 
> > > > The previous version still had this:
> > > > 
> > > > +                if test_file not in self.all_tests:
> > > > +                    print(f'Warning: {fname}: "{test_file}" test is not found.'
> > > > +                          ' Skip.')
> > > > +                    continue
> > > > 
> > > > Why did you remove it? I found this useful when I had a wrong test name
> > > > in my group.local. Now it's silently ignored.
> > > 
> > > 
> > > Because now we use parse_test_name which will raise ValueError, so we
> > > will not go to this if anyway.
> > > 
> > > So, wrong name will not be silently ignored, check will fail, and
> > > you'll have to fix group file.. It is suitable?
> > 
> > It doesn't, though.
> > 
> > Oh, wait... Is it possible that you lost support for group.local
> > altogether? grep for "group.local" comes up empty, and add_group_file()
> > is only defined, but never called.
> > 
> > So the reason for the behaviour seems to be that it doesn't even try to
> > parse the group file.
> 
> Ooops, you are right :( I've dropped an extra layer of indirection to
> make things simpler and group.local was lost. It's the reason to send
> v8, I'll do it now.

You can wait with sending v8 until I've completed review in case
something else comes up. So far I'm done with the changes to the part
that I reviewed last time and apart from this bug they look good to me.
Now it's the remaining patches.

> In a mean time, reverting 06 for now is OK for me.

Not a big deal if we get it fixed soon, it only becomes a problem if the
rest of this series gets shelved for a longer time. Maybe we can
complete it today, maybe on Monday, and then I'll send a pull request
right away.

Kevin