Re: [PATCH v3 06/10] iotests: add testfinder.py
14.05.2020 08:06, John Snow wrote: On 5/14/20 12:54 AM, Vladimir Sementsov-Ogievskiy wrote: 14.05.2020 00:58, John Snow wrote: On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote: 21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. It will make more difficult to import iotests.py.. Calling common.rc from bash tests will need to be modified too. So, we'll need additional line in all python tests: sys.path.append(os.path.join(os.path.dirname(__file__), '..')) which doesn't seem to be good practice.. So, instead we'd better call tests with PYTHONPATH set appropriately.. Just chiming in to say that it's largely bad practice because it confuses pylint, mypy and friends -- if we want to keep pushing our CI code analysis for python in that direction, this will be a barrier. Using PYTHONPATH is better, because it isolates the script itself from the environment, but requires you to now always set PYTHONPATH to execute any of the individual iotests. Not actually a big deal, because iotests already expect a large number of environment variables to be set. It's not really a huge net loss in convenience, I think. looks like that's the direction you're headed in anyway based on discussion, so that's good. Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it should be set when checking the code? So, actually developers will have to set PYTHONPATH by hand to always contain some directories within qemu source tree? pylint respects PYTHONPATH but mypy doesn't. mypy uses MYPYPATH, but I wouldn't worry about accommodating it. It's a fussy tool and we're only ever going to run it from very specific environments. Hmm, recently I installed dense-analysis/ale plugin into my vim which does mypy checking (among other things).. And most probably, I'll have to set these variables to keep it working. But it's not a big problem. You don't need to worry too much about what environment variables these tools take; it's only worth noting that "sys.path" hacks tend to make these tools harder to use. As for setting PYTHONPATH by hand ... There are a few places in the QEMU tree where we set PYTHONPATH already, and the individual iotests already don't work if they're not launched by `check`, because they're missing a ton of environment variables. It's not going to be too bad to set PYTHONPATH in the launcher script, is it? (Or are we replacing the top-level script with a python one?) Yes we do, bright future is near:) But it's not a problem to set PYTHONPATH in it. Anyway, we run all tests as executables, so passing PYTHONPATH is a valid thing to do. Really, the same is true of pylint, too. It's only annoying to deal with sys.path hacking because it can't be worked around in those CQA tools. -- Best regards, Vladimir
Re: [PATCH v3 06/10] iotests: add testfinder.py
On 5/14/20 12:54 AM, Vladimir Sementsov-Ogievskiy wrote: > 14.05.2020 00:58, John Snow wrote: >> >> >> On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote: >>> 21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: > - group file is dropped > - tests are searched by file-name instead of group file, so it's > not > needed more to "register the test", just create it with name > *-test. Old names like [0-9][0-9][0-9] are supported too, but not > recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. >>> >>> It will make more difficult to import iotests.py.. Calling common.rc >>> from >>> bash tests will need to be modified too. >>> >>> So, we'll need additional line in all python tests: >>> >>> sys.path.append(os.path.join(os.path.dirname(__file__), '..')) >>> >>> which doesn't seem to be good practice.. So, instead we'd better call >>> tests with PYTHONPATH set appropriately.. >>> >> >> Just chiming in to say that it's largely bad practice because it >> confuses pylint, mypy and friends -- if we want to keep pushing our CI >> code analysis for python in that direction, this will be a barrier. >> >> Using PYTHONPATH is better, because it isolates the script itself from >> the environment, but requires you to now always set PYTHONPATH to >> execute any of the individual iotests. >> >> Not actually a big deal, because iotests already expect a large number >> of environment variables to be set. It's not really a huge net loss in >> convenience, I think. >> >> looks like that's the direction you're headed in anyway based on >> discussion, so that's good. >> > > Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it > should > be set when checking the code? So, actually developers will have to set > PYTHONPATH by hand to always contain some directories within qemu source > tree? > pylint respects PYTHONPATH but mypy doesn't. mypy uses MYPYPATH, but I wouldn't worry about accommodating it. It's a fussy tool and we're only ever going to run it from very specific environments. You don't need to worry too much about what environment variables these tools take; it's only worth noting that "sys.path" hacks tend to make these tools harder to use. As for setting PYTHONPATH by hand ... There are a few places in the QEMU tree where we set PYTHONPATH already, and the individual iotests already don't work if they're not launched by `check`, because they're missing a ton of environment variables. It's not going to be too bad to set PYTHONPATH in the launcher script, is it? (Or are we replacing the top-level script with a python one?) Really, the same is true of pylint, too. It's only annoying to deal with sys.path hacking because it can't be worked around in those CQA tools.
Re: [PATCH v3 06/10] iotests: add testfinder.py
14.05.2020 00:58, John Snow wrote: On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote: 21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. It will make more difficult to import iotests.py.. Calling common.rc from bash tests will need to be modified too. So, we'll need additional line in all python tests: sys.path.append(os.path.join(os.path.dirname(__file__), '..')) which doesn't seem to be good practice.. So, instead we'd better call tests with PYTHONPATH set appropriately.. Just chiming in to say that it's largely bad practice because it confuses pylint, mypy and friends -- if we want to keep pushing our CI code analysis for python in that direction, this will be a barrier. Using PYTHONPATH is better, because it isolates the script itself from the environment, but requires you to now always set PYTHONPATH to execute any of the individual iotests. Not actually a big deal, because iotests already expect a large number of environment variables to be set. It's not really a huge net loss in convenience, I think. looks like that's the direction you're headed in anyway based on discussion, so that's good. Hm, does PYTHONPATH-way works good with mypy and friends? Probably, it should be set when checking the code? So, actually developers will have to set PYTHONPATH by hand to always contain some directories within qemu source tree? -- Best regards, Vladimir
Re: [PATCH v3 06/10] iotests: add testfinder.py
On 5/7/20 1:43 PM, Vladimir Sementsov-Ogievskiy wrote: > 21.04.2020 19:56, Kevin Wolf wrote: >> Am 21.04.2020 um 09:35 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 new test: >>> - group file is dropped >>> - tests are searched by file-name instead of group file, so it's not >>> needed more to "register the test", just create it with name >>> *-test. Old names like [0-9][0-9][0-9] are supported too, but not >>> recommended for new tests >> >> I wonder if a tests/ subdirectory instead of the -test suffix would >> organise things a bit better. >> > > It will make more difficult to import iotests.py.. Calling common.rc from > bash tests will need to be modified too. > > So, we'll need additional line in all python tests: > > sys.path.append(os.path.join(os.path.dirname(__file__), '..')) > > which doesn't seem to be good practice.. So, instead we'd better call > tests with PYTHONPATH set appropriately.. > Just chiming in to say that it's largely bad practice because it confuses pylint, mypy and friends -- if we want to keep pushing our CI code analysis for python in that direction, this will be a barrier. Using PYTHONPATH is better, because it isolates the script itself from the environment, but requires you to now always set PYTHONPATH to execute any of the individual iotests. Not actually a big deal, because iotests already expect a large number of environment variables to be set. It's not really a huge net loss in convenience, I think. looks like that's the direction you're headed in anyway based on discussion, so that's good. --js
Re: [PATCH v3 06/10] iotests: add testfinder.py
08.05.2020 11:49, Kevin Wolf wrote: Am 07.05.2020 um 19:43 hat Vladimir Sementsov-Ogievskiy geschrieben: 21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. It will make more difficult to import iotests.py.. Calling common.rc from bash tests will need to be modified too. So, we'll need additional line in all python tests: sys.path.append(os.path.join(os.path.dirname(__file__), '..')) Hm, yes, this is nasty. Would it be any better have a wrapper script that just imports the actual test so that the import path would still contain the main qemu-iotests/ directory? I think, that better to keep all tests self-executable without any additional preparations, just set all interface environment variables and run test (or use check to automate environment initialization). In this way we can support any executable, not even limited to python and bash (I doubt that we need more, but keeping test interface full-defined by environment variables seems a good thing). which doesn't seem to be good practice.. So, instead we'd better call tests with PYTHONPATH set appropriately.. This is another option, especially if we do want to create a lib/. and modify bash tests to do . ../common.rc . ../common.filter or again, better to export BASH_TEST_LIB directory. I don't think this is necessary because the working directory wouldn't change, so bash scripts should just keep working as before. Hmm, right. Probably same works for python tests as well? Is it worth doing? I think, there are two variants: 1) keep as is: all in one directory, add *-test notation I think it would make it rather hard to find the files that belong to the test harness implementation between all the tests. Currently, 'ls' in the qemu-iotests directory is still kind of usable because all the test cases are at the start and everything that comes later is not a test. 2) go further and restructure to something like: iotests/ iotests/tests/ iotests/lib/python/iotests.py iotests/lib/bash/ And then, check script will export PYTHONPATH and BASH_TEST_LIB variables. I think leaving everything except named tests where it is, but setting PYTHONPATH or having a wrapper script, is still a third option that's worth considering. It sounds like the most attrative option to me. Works for me too. OK, let's go this way. -- Best regards, Vladimir
Re: [PATCH v3 06/10] iotests: add testfinder.py
Am 07.05.2020 um 19:43 hat Vladimir Sementsov-Ogievskiy geschrieben: > 21.04.2020 19:56, Kevin Wolf wrote: > > Am 21.04.2020 um 09:35 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 new test: > > > - group file is dropped > > > - tests are searched by file-name instead of group file, so it's not > > > needed more to "register the test", just create it with name > > > *-test. Old names like [0-9][0-9][0-9] are supported too, but not > > > recommended for new tests > > > > I wonder if a tests/ subdirectory instead of the -test suffix would > > organise things a bit better. > > > > It will make more difficult to import iotests.py.. Calling common.rc from > bash tests will need to be modified too. > > So, we'll need additional line in all python tests: > > sys.path.append(os.path.join(os.path.dirname(__file__), '..')) Hm, yes, this is nasty. Would it be any better have a wrapper script that just imports the actual test so that the import path would still contain the main qemu-iotests/ directory? > which doesn't seem to be good practice.. So, instead we'd better call > tests with PYTHONPATH set appropriately.. This is another option, especially if we do want to create a lib/. > and modify bash tests to do > . ../common.rc > . ../common.filter > > > or again, better to export BASH_TEST_LIB directory. I don't think this is necessary because the working directory wouldn't change, so bash scripts should just keep working as before. > Is it worth doing? > > I think, there are two variants: > > 1) keep as is: all in one directory, add *-test notation I think it would make it rather hard to find the files that belong to the test harness implementation between all the tests. Currently, 'ls' in the qemu-iotests directory is still kind of usable because all the test cases are at the start and everything that comes later is not a test. > 2) go further and restructure to something like: > > iotests/ > iotests/tests/ > iotests/lib/python/iotests.py > iotests/lib/bash/ > > And then, check script will export PYTHONPATH and BASH_TEST_LIB > variables. I think leaving everything except named tests where it is, but setting PYTHONPATH or having a wrapper script, is still a third option that's worth considering. It sounds like the most attrative option to me. Kevin
Re: [PATCH v3 06/10] iotests: add testfinder.py
21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. It will make more difficult to import iotests.py.. Calling common.rc from bash tests will need to be modified too. So, we'll need additional line in all python tests: sys.path.append(os.path.join(os.path.dirname(__file__), '..')) which doesn't seem to be good practice.. So, instead we'd better call tests with PYTHONPATH set appropriately.. and modify bash tests to do . ../common.rc . ../common.filter or again, better to export BASH_TEST_LIB directory. Is it worth doing? I think, there are two variants: 1) keep as is: all in one directory, add *-test notation 2) go further and restructure to something like: iotests/ iotests/tests/ iotests/lib/python/iotests.py iotests/lib/bash/ And then, check script will export PYTHONPATH and BASH_TEST_LIB variables. In case of [2], I definitely prefer to split directory tree refactoring to the separate series. So this series just rewrite check into python and get rid of group file. What do you prefer? -- Best regards, Vladimir
Re: [PATCH v3 06/10] iotests: add testfinder.py
Am 22.04.2020 um 14:49 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 22.04.2020 14:53, Kevin Wolf wrote:
> > Am 22.04.2020 um 07:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 21.04.2020 19:56, Kevin Wolf wrote:
> > > > Am 21.04.2020 um 09:35 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > +if __name__ == '__main__':
> > > > > +if len(sys.argv) == 2 and sys.argv[1] in ['-h', '--help']:
> > > > > +TestFinder.argparser.print_help()
> > > > > +exit()
> > > > > +
> > > > > +tests, remaining_argv = find_tests(sys.argv[1:])
> > > > > +print('\n'.join(tests))
> > > > > +if remaining_argv:
> > > > > +print('\nUnhandled options: ', remaining_argv)
> > > >
> > > > I think unhandled options shouldn't be considered an error and we
> > > > shouldn't even try to find and display any tests then. I'd either print
> > > > the help text or have an error message that refers to --help.
> > >
> > > As I considered running this script as executable for testing purposes,
> > > it's
> > > good to know, which options are not handled..
> >
> > Yes, that makes sense. I just think it should be an error and not just
> > an additional hint at the end.
> >
>
> It's not and error, as usual case will leave some arguments for
> TestEnv and TestRunner. Assume you run ./check with some arguments..
>
> And it works bad. You assume that problem is in testfinder.py. Then,
> you just take the entire list of options and call ./testfinder.py with
> them. And it shows, how it parses its arguments, and what is reminded.
> Seems correct and shouldn't be an error.
Hm, I didn't consider this use case. Fair enough then.
> Still, maybe better to print unhandled options first, to be more
> noticeable.
Actually, I think it's more noticeable at the end when you execute the
script standalone, especially when it prints a long list.
Kevin
Re: [PATCH v3 06/10] iotests: add testfinder.py
22.04.2020 14:53, Kevin Wolf wrote: Am 22.04.2020 um 07:35 hat Vladimir Sementsov-Ogievskiy geschrieben: 21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. No objections. I also thought about, may be, a tests/ subtree, so we'll have something like tests/jobs/ tests/formats/ ... I thought of that, too, but then decided not to mention it because I feel it might be hard to decide where a test belongs. Many tests are qcow2 and mirror tests at the same time, every commit test is also a backing file test etc. This is essentially why we have groups and each test can be in more than one group. On the other hand, I assume that for some tests it would be quite clear where they belong. So if we're prepared to have some tests directly in tests/ and only some others in subdirectories, we can still do that. - 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 Occasionally they were useful when something went wrong during the test run and I only wanted to repeat the part after it happened. But it's a rare case and we don't have a clear order any more with arbitrary test names (which are an improvement otherwise), so we'll live with it. Yes, I've used it for same thing. Actually, we still have the order, as I just sort iotests by name. I think, we could add a parameter for testfinder (may be, as a separate step no in these series), something like --start-from TEST : parse all other arguments as usual, make sorted sequence and than drop tests from the first one to TEST (not inclusive). This may be used to rerun failed ./check command, starting from the middle of the process. True, this would be a good replacement. I don't think it's a requirement to have it in this series, but I won't complain if you implement it. :-) 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 just adds class, which is unused now, and will be used in further patches, to finally substitute ./check selecting tests logic. Maybe mention here that the file can be executed standalone, even if it'S not used by check yet. 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. Signed-off-by: Vladimir Sementsov-Ogievskiy --- docs/devel/testing.rst | 52 +- tests/qemu-iotests/testfinder.py | 167 +++ A little bit of bikeshedding: As this can be executed as a standalone tool, would a name like findtests.py be better? Hmm. I named it by class name, considering possibility to execute is just for module testing. So for module users, it's just a module with class TestFinder, and it's called testfinder.. But I don't have strict opinion in it, findtests sound more human-friendly. It was just a thought. If you prefer testfinder.py, I'm fine with it. 2 files changed, 218 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/testfinder.py diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 770a987ea4..6c9d5b126b 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -153,7 +153,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 @@ -267,6 +267,56 @@ another application on the host may have locked
Re: [PATCH v3 06/10] iotests: add testfinder.py
Am 22.04.2020 um 07:35 hat Vladimir Sementsov-Ogievskiy geschrieben: > 21.04.2020 19:56, Kevin Wolf wrote: > > Am 21.04.2020 um 09:35 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 new test: > > > - group file is dropped > > > - tests are searched by file-name instead of group file, so it's not > > > needed more to "register the test", just create it with name > > > *-test. Old names like [0-9][0-9][0-9] are supported too, but not > > > recommended for new tests > > > > I wonder if a tests/ subdirectory instead of the -test suffix would > > organise things a bit better. > > No objections. > > I also thought about, may be, a tests/ subtree, so we'll have something like > > tests/jobs/ > tests/formats/ > ... I thought of that, too, but then decided not to mention it because I feel it might be hard to decide where a test belongs. Many tests are qcow2 and mirror tests at the same time, every commit test is also a backing file test etc. This is essentially why we have groups and each test can be in more than one group. On the other hand, I assume that for some tests it would be quite clear where they belong. So if we're prepared to have some tests directly in tests/ and only some others in subdirectories, we can still do that. > > > - 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 > > > > Occasionally they were useful when something went wrong during the test > > run and I only wanted to repeat the part after it happened. But it's a > > rare case and we don't have a clear order any more with arbitrary test > > names (which are an improvement otherwise), so we'll live with it. > > Yes, I've used it for same thing. > > Actually, we still have the order, as I just sort iotests by name. I think, > we could add a parameter for testfinder (may be, as a separate step no in > these series), something like > > --start-from TEST : parse all other arguments as usual, make sorted sequence > and than drop tests from the first one to TEST (not inclusive). This may be > used to rerun failed ./check command, starting from the middle of the process. True, this would be a good replacement. I don't think it's a requirement to have it in this series, but I won't complain if you implement it. :-) > > > > > 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 just adds class, which is unused now, and will be used in > > > further patches, to finally substitute ./check selecting tests logic. > > > > Maybe mention here that the file can be executed standalone, even if > > it'S not used by check yet. > > > > > 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. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > --- > > > docs/devel/testing.rst | 52 +- > > > tests/qemu-iotests/testfinder.py | 167 +++ > > > > A little bit of bikeshedding: As this can be executed as a standalone > > tool, would a name like findtests.py be better? > > Hmm. I named it by class name, considering possibility to execute is > just for module testing. So for module users, it's just a module with > class TestFinder, and it's called testfinder.. But I don't have strict > opinion in it, findtests sound more human-friendly. It was just a thought. If you prefer testfinder.py, I'm fine with it. > > > > > 2 files changed, 218 insertions(+), 1 deletion(-) > > > create mode 100755 tests/qemu-iotests/testfinder.py > > > > > > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst > > > index 770a987ea4..6c9d5b126b 100644 > > > --- a/docs/devel/testing.rst > > > +++ b/docs/devel/testing.rst > > > @@ -153,7 +153,7 @@ check-block
Re: [PATCH v3 06/10] iotests: add testfinder.py
21.04.2020 19:56, Kevin Wolf wrote: Am 21.04.2020 um 09:35 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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] are supported too, but not recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. No objections. I also thought about, may be, a tests/ subtree, so we'll have something like tests/jobs/ tests/formats/ ... - 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 Occasionally they were useful when something went wrong during the test run and I only wanted to repeat the part after it happened. But it's a rare case and we don't have a clear order any more with arbitrary test names (which are an improvement otherwise), so we'll live with it. Yes, I've used it for same thing. Actually, we still have the order, as I just sort iotests by name. I think, we could add a parameter for testfinder (may be, as a separate step no in these series), something like --start-from TEST : parse all other arguments as usual, make sorted sequence and than drop tests from the first one to TEST (not inclusive). This may be used to rerun failed ./check command, starting from the middle of the process. 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 just adds class, which is unused now, and will be used in further patches, to finally substitute ./check selecting tests logic. Maybe mention here that the file can be executed standalone, even if it'S not used by check yet. 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. Signed-off-by: Vladimir Sementsov-Ogievskiy --- docs/devel/testing.rst | 52 +- tests/qemu-iotests/testfinder.py | 167 +++ A little bit of bikeshedding: As this can be executed as a standalone tool, would a name like findtests.py be better? Hmm. I named it by class name, considering possibility to execute is just for module testing. So for module users, it's just a module with class TestFinder, and it's called testfinder.. But I don't have strict opinion in it, findtests sound more human-friendly. 2 files changed, 218 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/testfinder.py diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 770a987ea4..6c9d5b126b 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -153,7 +153,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 @@ -267,6 +267,56 @@ 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 + + +Test may belong to some groups, you may define it in the comment inside the +test. By convention, test groups are listed in the second line of the test +file, after "#!/..." line, like this: + +.. code:: + + #!/usr/bin/env python3 + # group: auto quick + # + ... + +Additional way of defining groups is creating 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 temporary disable tests, like this: + +.. code:: + + # groups for some company downstream process + # + # ci - tests to run on build + # down - our
Re: [PATCH v3 06/10] iotests: add testfinder.py
Am 21.04.2020 um 09:35 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 new test: > - group file is dropped > - tests are searched by file-name instead of group file, so it's not >needed more to "register the test", just create it with name >*-test. Old names like [0-9][0-9][0-9] are supported too, but not >recommended for new tests I wonder if a tests/ subdirectory instead of the -test suffix would organise things a bit better. > - 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 Occasionally they were useful when something went wrong during the test run and I only wanted to repeat the part after it happened. But it's a rare case and we don't have a clear order any more with arbitrary test names (which are an improvement otherwise), so we'll live with it. > 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 just adds class, which is unused now, and will be used in > further patches, to finally substitute ./check selecting tests logic. Maybe mention here that the file can be executed standalone, even if it'S not used by check yet. > 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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > docs/devel/testing.rst | 52 +- > tests/qemu-iotests/testfinder.py | 167 +++ A little bit of bikeshedding: As this can be executed as a standalone tool, would a name like findtests.py be better? > 2 files changed, 218 insertions(+), 1 deletion(-) > create mode 100755 tests/qemu-iotests/testfinder.py > > diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst > index 770a987ea4..6c9d5b126b 100644 > --- a/docs/devel/testing.rst > +++ b/docs/devel/testing.rst > @@ -153,7 +153,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 > @@ -267,6 +267,56 @@ 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 > + > + > +Test may belong to some groups, you may define it in the comment inside the > +test. By convention, test groups are listed in the second line of the test > +file, after "#!/..." line, like this: > + > +.. code:: > + > + #!/usr/bin/env python3 > + # group: auto quick > + # > + ... > + > +Additional way of defining groups is creating 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 temporary disable 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 > + > +The following groups are defined: > + > +- quick : Tests in this group should finish within some few seconds. > + > +- img : Tests in this group can be used to excercise the qemu-img tool. > + > +- 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
[PATCH v3 06/10] iotests: add testfinder.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 new test: - group file is dropped - tests are searched by file-name instead of group file, so it's not needed more to "register the test", just create it with name *-test. Old names like [0-9][0-9][0-9] 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 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 just adds class, which is unused now, and will be used in further patches, to finally substitute ./check selecting tests logic. 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. Signed-off-by: Vladimir Sementsov-Ogievskiy --- docs/devel/testing.rst | 52 +- tests/qemu-iotests/testfinder.py | 167 +++ 2 files changed, 218 insertions(+), 1 deletion(-) create mode 100755 tests/qemu-iotests/testfinder.py diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst index 770a987ea4..6c9d5b126b 100644 --- a/docs/devel/testing.rst +++ b/docs/devel/testing.rst @@ -153,7 +153,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 @@ -267,6 +267,56 @@ 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 + + +Test may belong to some groups, you may define it in the comment inside the +test. By convention, test groups are listed in the second line of the test +file, after "#!/..." line, like this: + +.. code:: + + #!/usr/bin/env python3 + # group: auto quick + # + ... + +Additional way of defining groups is creating 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 temporary disable 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 + +The following groups are defined: + +- quick : Tests in this group should finish within some few seconds. + +- img : Tests in this group can be used to excercise the qemu-img tool. + +- 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/testfinder.py b/tests/qemu-iotests/testfinder.py new file mode 100755 index 00..1da28564c0 --- /dev/null +++ b/tests/qemu-iotests/testfinder.py @@ -0,0 +1,167 @@ +#!/usr/bin/env python3 +# +# Parse command line options to define set of tests to run. +# +# Copyright (c) 2020 Virtuozzo International GmbH +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This
