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

[Vladimir Sementsov-Ogievskiy]

22.01.2021 14:48, Kevin Wolf wrote:
> Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Add python script with new logic of searching for tests:
> >
> > Current ./check behavior:
> >   - tests are named [0-9][0-9][0-9]
> >   - tests must be registered in group file (even if test doesn't belong
> >     to any group, like 142)
> >
> > Behavior of findtests.py:
> >   - group file is dropped
> >   - tests are all files in tests/ subdirectory (except for .out files),
> >     so it's not needed more to "register the test", just create it with
> >     appropriate name in tests/ subdirectory. Old names like
> >     [0-9][0-9][0-9] (in root iotests directory) are supported too, but
> >     not recommended for new tests
> >   - groups are parsed from '# group: ' line inside test files
> >   - optional file group.local may be used to define some additional
> >     groups for downstreams
> >   - 'disabled' group is used to temporary disable tests. So instead of
> >     commenting tests in old 'group' file you now can add them to
> >     disabled group with help of 'group.local' file
> >   - selecting test ranges like 5-15 are not supported more
> >     (to support restarting failed ./check command from the middle of the
> >      process, new argument is added: --start-from)
> >
> > Benefits:
> >   - no rebase conflicts in group file on patch porting from branch to
> >     branch
> >   - no conflicts in upstream, when different series want to occupy same
> >     test number
> >   - meaningful names for test files
> >     For example, with digital number, when some person wants to add some
> >     test about block-stream, he most probably will just create a new
> >     test. But if there would be test-block-stream test already, he will
> >     at first look at it and may be just add a test-case into it.
> >     And anyway meaningful names are better.
> >
> > This commit don't update check behavior (which will be done in further
> > commit), still, the documentation changed like new behavior is already
> > here.  Let's live with this small inconsistency for the following few
> > commits, until final change.
> >
> > The file findtests.py is self-executable and may be used for debugging
> > purposes.
>
> As Eric mentioned, this isn't accurate any more.
>
> You mentioned using it as a way to debug things. I assume this is now
> covered by the dry run option?

yes

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   docs/devel/testing.rst          |  50 +++++++++-
> >   tests/qemu-iotests/findtests.py | 159 ++++++++++++++++++++++++++++++++
> >   2 files changed, 208 insertions(+), 1 deletion(-)
> >   create mode 100644 tests/qemu-iotests/findtests.py
>
> > +    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?

> > +                for g in groups:
> > +                    self.groups[g].add(test_file)
>
> Kevin


--
Best regards,
Vladimir