Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 16:34, Kevin Wolf wrote:
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.
OK. This thing is to be fixed in [10], not here, I'll send a squash-in
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
--
Best regards,
Vladimir
Re: [PATCH v7 07/11] iotests: add findtests.py
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
Re: [PATCH v7 07/11] iotests: add findtests.py
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.
In a mean time, reverting 06 for now is OK for me.
--
Best regards,
Vladimir
Re: [PATCH v7 07/11] iotests: add findtests.py
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.
Kevin
Re: [PATCH v7 07/11] iotests: add findtests.py
22.01.2021 14:49, Kevin Wolf wrote: Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: +if 'disabled' not in groups and 'disabled' not in exclude_groups: +# Don't want to modify function argument, so create new list. +exclude_groups = exclude_groups + ['disabled'] Oops, forgot the other comment I wanted to make: This would only have been needed if you had turned exclude_groups into a Sequence. Now that it's still a list, copying the list isn't strictly necessary. I just think that such side effects (changing function arguments when it is not part of function intention) are better to avoid. -- Best regards, Vladimir
Re: [PATCH v7 07/11] iotests: add findtests.py
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
---
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
Re: [PATCH v7 07/11] iotests: add findtests.py
Am 16.01.2021 um 14:44 hat Vladimir Sementsov-Ogievskiy geschrieben: > +if 'disabled' not in groups and 'disabled' not in exclude_groups: > +# Don't want to modify function argument, so create new list. > +exclude_groups = exclude_groups + ['disabled'] Oops, forgot the other comment I wanted to make: This would only have been needed if you had turned exclude_groups into a Sequence. Now that it's still a list, copying the list isn't strictly necessary. Kevin
Re: [PATCH v7 07/11] iotests: add findtests.py
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?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> 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.
> +for g in groups:
> +self.groups[g].add(test_file)
Kevin
Re: [PATCH v7 07/11] iotests: add findtests.py
21.01.2021 19:21, Eric Blake wrote: On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote: Add python script with new logic of searching for tests: The file findtests.py is self-executable and may be used for debugging purposes. Signed-off-by: Vladimir Sementsov-Ogievskiy --- diff --git a/tests/qemu-iotests/findtests.py b/tests/qemu-iotests/findtests.py new file mode 100644 index 00..065ec13125 --- /dev/null +++ b/tests/qemu-iotests/findtests.py @@ -0,0 +1,159 @@ +# TestFinder class, define set of tests to run. Per the commit message, the file is self-executable; doesn't that mean it should have 755 permissions instead of 644, and have a #! python line? +return sequence I guess it would also need a __main__ blurb at the end? Or maybe I'm misunderstanding the intent of the commit message line. These things are dropped recently and I forget to updated commit-msg. So, what should be done is just drop this sentence from commit message. -- Best regards, Vladimir
Re: [PATCH v7 07/11] iotests: add findtests.py
On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote: > Add python script with new logic of searching for tests: > > The file findtests.py is self-executable and may be used for debugging > purposes. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > diff --git a/tests/qemu-iotests/findtests.py b/tests/qemu-iotests/findtests.py > new file mode 100644 > index 00..065ec13125 > --- /dev/null > +++ b/tests/qemu-iotests/findtests.py > @@ -0,0 +1,159 @@ > +# TestFinder class, define set of tests to run. Per the commit message, the file is self-executable; doesn't that mean it should have 755 permissions instead of 644, and have a #! python line? > +return sequence > I guess it would also need a __main__ blurb at the end? Or maybe I'm misunderstanding the intent of the commit message line. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v7 07/11] iotests: add findtests.py
On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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
doesn't
> 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.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> 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
>
> +++ b/tests/qemu-iotests/findtests.py
> +class TestFinder:
> +def __init__(self, test_dir: Optional[str] = None) -> None:
> +self.groups = defaultdict(set)
> +
> +with chdir(test_dir):
> +self.all_tests = glob.glob('[0-9][0-9][0-9]')
> +self.all_tests += [f for f in glob.iglob('tests/*')
> + if not f.endswith('.out') and
> + os.path.isfile(f + '.out')]
Interesting that 'NNN' is a test even if 'NNN.out' is not present, but
'tests/NNN' is not. Not sure if it is worth tweaking, though.
> +def parse_test_name(self, name: str) -> str:
> +if '/' in name:
> +raise ValueError('Paths are unsupported for test selecting, '
selection
> + f'requiring "{name}" is wrong')
> +
> +if re.fullmatch(r'\d+', name):
> +# Numbered tests are old naming convetion. We should convert them
convention
> +# to three-digit-length, like 1 --> 001.
> +name = f'{int(name):03}'
> +else:
> +# Named tests all should be in tests/ subdirectory
> +name = os.path.join('tests', name)
> +
> +if name not in self.all_tests:
> +raise ValueError(f'Test "{name}" is not found')
> +
> +return name
> +
> +def find_tests(self, groups: Optional[List[str]] = None,
> + exclude_groups: Optional[List[str]] = None,
> + tests: Optional[List[str]] = None,
> + start_from: Optional[str] = None) -> List[str]:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v7 07/11] iotests: add findtests.py
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. Signed-off-by: Vladimir Sementsov-Ogievskiy --- 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 diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 0aa7a13bba..454b706141 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -111,7 +111,7 @@ check-block --- ``make check-block`` runs a subset of the block layer iotests (the tests that -are in the "auto" group in ``tests/qemu-iotests/group``). +are in the "auto" group). See the "QEMU iotests" section below for more information. GCC gcov support @@ -224,6 +224,54 @@ another application on the host may have locked the file, possibly leading to a test failure. If using such devices are explicitly desired, consider adding ``locking=off`` option to disable image locking. +Test case groups + + +"Tests may belong to one or more test groups, which are defined in the form +of a comment in the test source file. By convention, test groups are listed +in the second line of the test file, after the "#!/..." line, like this: + +.. code:: + + #!/usr/bin/env python3 + # group: auto quick + # + ... + +Another way of defining groups is creating the tests/qemu-iotests/group.local +file. This should be used only for downstream (this file should never appear +in upstream). This file may be used for defining some downstream test groups +or for temporarily disabling tests, like this: + +.. code:: + + # groups for some company downstream process + # + # ci - tests to run on build + # down - our downstream tests, not for upstream + # + # Format of each line is: + # TEST_NAME TEST_GROUP [TEST_GROUP ]... + + 013 ci + 210 disabled + 215 disabled + our-ugly-workaround-test down ci + +Note that the following group names have a special meaning: + +- quick: Tests in this group should finish within a few seconds. + +- auto: Tests in this group are used during "make check" and should be + runnable in any case. That means they should run with every QEMU binary + (also non-x86), with every QEMU configuration (i.e. must not fail if + an optional feature is not compiled in - but reporting a "skip" is ok), + work at least with the qcow2 file format, work with all kind of host + filesystems and users (e.g. "nobody" or "root") and must not take too + much memory and disk space (since CI pipelines tend to fail otherwise). + +- disabled: Tests in this group are disabled and ignored by check. + .. _docker-ref: Docker based tests diff --git a/tests/qemu-iotests/findtests.py b/tests/qemu-iotests/findtests.py new file mode 100644 index 00..065ec13125 --- /dev/null +++ b/tests/qemu-iotests/findtests.py @@ -0,0 +1,159 @@ +# TestFinder class, define set of tests to run. +# +# Copyright (c) 2020-2021 Virtuozzo International GmbH +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of
