Re: [PATCH] iotests: drop group file
Am 24.03.2020 um 11:21 hat Daniel P. Berrangé geschrieben:
> On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 24.03.2020 12:36, Kevin Wolf wrote:
> > > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > When sending iotests to upstream or do patch porting from one branch
> > > > to another we very often have to resolve conflicts in group file, as
> > > > many absolutely independent features are intersecting by this file.
> > > > These conflicts are simple, but imagine how much time we all have
> > > > already spent on resolving them? Let's finally get rid of group file.
> > > >
> > > > This patch transposes group info: instead of collecting it in one file,
> > > > let each test define its groups by itself.
> > > >
> > > > Several steps are done to achive it:
> > > >
> > > > 1. Define groups in test files automatically:
> > > >
> > > > grep '^[0-9]\{3\} ' group | while read line; do
> > > > file=$(awk '{print $1}' <<< "$line");
> > > > groups=$(sed -e 's/^... //' <<< "$line");
> > > > awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > > > cat tmp > $file;
> > > > done
> > > >
> > > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > > has a section about iotests.
> > > >
> > > > 3. Modify check script to work without group file.
> > > >
> > > > Here is a logic change: before, even if test do not belong to any
> > > > group (only iotest 142 currently) it should be defined in group
> > > > file. Now, test is not forced to define any group. Instead check
> > > > considers all files with names matching [0-9][0-9][0-9] as tests.
> > >
> > > This has both a positive and a negative effect: Now you don't have to
> > > modify another file when you add a new test, but it will be picked up
> > > automatically. However, if you want to disable a test, you could
> > > previously just remove it from groups (or comment it out), and now you
> > > have to delete the test instead.
> >
> > Hmm. Probably, you could add it to group "disabled", and run check -x
> > disabled.
>
> As a developer you don't really want to be making changes to git tracked
> files in order to temporarily skip a test, as then git reports them as
> modified & you risk accidentally committing throwaway changes.
>
> Better to have a separate groups.local file to record local overrides
> in a non-tracked file.
For locally disabling tests, we have that 'expunged' file that nobody
knows about and that I just found when we were reformatting the ./check
output...
I wouldn't want to use that for disabling the tests in a downstream
repository, though, it should stay a local thing even there.
Kevin
Re: [PATCH] iotests: drop group file
On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 24.03.2020 12:36, Kevin Wolf wrote:
> > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > When sending iotests to upstream or do patch porting from one branch
> > > to another we very often have to resolve conflicts in group file, as
> > > many absolutely independent features are intersecting by this file.
> > > These conflicts are simple, but imagine how much time we all have
> > > already spent on resolving them? Let's finally get rid of group file.
> > >
> > > This patch transposes group info: instead of collecting it in one file,
> > > let each test define its groups by itself.
> > >
> > > Several steps are done to achive it:
> > >
> > > 1. Define groups in test files automatically:
> > >
> > > grep '^[0-9]\{3\} ' group | while read line; do
> > > file=$(awk '{print $1}' <<< "$line");
> > > groups=$(sed -e 's/^... //' <<< "$line");
> > > awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > > cat tmp > $file;
> > > done
> > >
> > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > has a section about iotests.
> > >
> > > 3. Modify check script to work without group file.
> > >
> > > Here is a logic change: before, even if test do not belong to any
> > > group (only iotest 142 currently) it should be defined in group
> > > file. Now, test is not forced to define any group. Instead check
> > > considers all files with names matching [0-9][0-9][0-9] as tests.
> >
> > This has both a positive and a negative effect: Now you don't have to
> > modify another file when you add a new test, but it will be picked up
> > automatically. However, if you want to disable a test, you could
> > previously just remove it from groups (or comment it out), and now you
> > have to delete the test instead.
>
> Hmm. Probably, you could add it to group "disabled", and run check -x
> disabled.
As a developer you don't really want to be making changes to git tracked
files in order to temporarily skip a test, as then git reports them as
modified & you risk accidentally committing throwaway changes.
Better to have a separate groups.local file to record local overrides
in a non-tracked file.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] iotests: drop group file
24.03.2020 12:51, Daniel P. Berrangé wrote:
On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:
Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.
This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.
Several steps are done to achive it:
1. Define groups in test files automatically:
grep '^[0-9]\{3\} ' group | while read line; do
file=$(awk '{print $1}' <<< "$line");
groups=$(sed -e 's/^... //' <<< "$line");
awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
cat tmp > $file;
done
2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.
3. Modify check script to work without group file.
Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.
This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.
Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.
So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.
Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?
Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?
check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.
This one would also require us to be able to distinguish test case files
from arbitrary other files.
A test-X.sh or a XX.test naming convention ?
-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse
We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)
The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.
If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.
Hm. You are right, better to solve all these problems together.
--
Best regards,
Vladimir
Re: [PATCH] iotests: drop group file
On 3/24/20 12:36 PM, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> When sending iotests to upstream or do patch porting from one branch
>> to another we very often have to resolve conflicts in group file, as
>> many absolutely independent features are intersecting by this file.
>> These conflicts are simple, but imagine how much time we all have
>> already spent on resolving them? Let's finally get rid of group file.
>>
>> This patch transposes group info: instead of collecting it in one file,
>> let each test define its groups by itself.
>>
>> Several steps are done to achive it:
>>
>> 1. Define groups in test files automatically:
>>
>> grep '^[0-9]\{3\} ' group | while read line; do
>> file=$(awk '{print $1}' <<< "$line");
>> groups=$(sed -e 's/^... //' <<< "$line");
>> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
>> cat tmp > $file;
>> done
>>
>> 2. Copy groups documentation into docs/devel/testing.rst, which already
>>has a section about iotests.
>>
>> 3. Modify check script to work without group file.
>>
>>Here is a logic change: before, even if test do not belong to any
>>group (only iotest 142 currently) it should be defined in group
>>file. Now, test is not forced to define any group. Instead check
>>considers all files with names matching [0-9][0-9][0-9] as tests.
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
>
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
>
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.
+1 for blacklist.local file
>
>>check script is also refactored to make it simple to do next cool
>>thing about iotests: allow meaningful names for test-case files.
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.
>
>> -#
>> -# test-group association ... one line per test
>> -#
>> -001 rw auto quick
>> -002 rw auto quick
>> -003 rw auto
>> -004 rw auto quick
>> -005 img auto quick
>> -# 006 was removed, do not reuse
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)
we could generate this file with a starter script with proper
option.
Den
Re: [PATCH] iotests: drop group file
24.03.2020 12:36, Kevin Wolf wrote:
Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.
This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.
Several steps are done to achive it:
1. Define groups in test files automatically:
grep '^[0-9]\{3\} ' group | while read line; do
file=$(awk '{print $1}' <<< "$line");
groups=$(sed -e 's/^... //' <<< "$line");
awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
cat tmp > $file;
done
2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.
3. Modify check script to work without group file.
Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.
This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.
Hmm. Probably, you could add it to group "disabled", and run check -x disabled.
Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.
I came to fixing all test in downstream, correctly skipping test-cases which
needs compiled-out feature. We don't comment out tests in group, all tests
should pass or be correctly skipped by check.
So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.
What about "disabled" group?
check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.
This one would also require us to be able to distinguish test case files
from arbitrary other files.
I think, something like "all files started with 'test-' prefix"
-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse
We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)
Hmm.. Okay, if we had this test in past, than drop, than reuse number for the
another test, we possibly may break bisecting.
I can add notes about it into documentation (nobody will read it). Anyway, I
don't think it is too bad.
And if we move to textual file-names for tests, we will not reuse old removed
numbers anyway.
--
Best regards,
Vladimir
Re: [PATCH] iotests: drop group file
On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > When sending iotests to upstream or do patch porting from one branch
> > to another we very often have to resolve conflicts in group file, as
> > many absolutely independent features are intersecting by this file.
> > These conflicts are simple, but imagine how much time we all have
> > already spent on resolving them? Let's finally get rid of group file.
> >
> > This patch transposes group info: instead of collecting it in one file,
> > let each test define its groups by itself.
> >
> > Several steps are done to achive it:
> >
> > 1. Define groups in test files automatically:
> >
> > grep '^[0-9]\{3\} ' group | while read line; do
> > file=$(awk '{print $1}' <<< "$line");
> > groups=$(sed -e 's/^... //' <<< "$line");
> > awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > cat tmp > $file;
> > done
> >
> > 2. Copy groups documentation into docs/devel/testing.rst, which already
> >has a section about iotests.
> >
> > 3. Modify check script to work without group file.
> >
> >Here is a logic change: before, even if test do not belong to any
> >group (only iotest 142 currently) it should be defined in group
> >file. Now, test is not forced to define any group. Instead check
> >considers all files with names matching [0-9][0-9][0-9] as tests.
>
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
>
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
>
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.
Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?
Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?
> >check script is also refactored to make it simple to do next cool
> >thing about iotests: allow meaningful names for test-case files.
>
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.
A test-X.sh or a XX.test naming convention ?
> > -#
> > -# test-group association ... one line per test
> > -#
> > -001 rw auto quick
> > -002 rw auto quick
> > -003 rw auto
> > -004 rw auto quick
> > -005 img auto quick
> > -# 006 was removed, do not reuse
>
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)
The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.
If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] iotests: drop group file
Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> When sending iotests to upstream or do patch porting from one branch
> to another we very often have to resolve conflicts in group file, as
> many absolutely independent features are intersecting by this file.
> These conflicts are simple, but imagine how much time we all have
> already spent on resolving them? Let's finally get rid of group file.
>
> This patch transposes group info: instead of collecting it in one file,
> let each test define its groups by itself.
>
> Several steps are done to achive it:
>
> 1. Define groups in test files automatically:
>
> grep '^[0-9]\{3\} ' group | while read line; do
> file=$(awk '{print $1}' <<< "$line");
> groups=$(sed -e 's/^... //' <<< "$line");
> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> cat tmp > $file;
> done
>
> 2. Copy groups documentation into docs/devel/testing.rst, which already
>has a section about iotests.
>
> 3. Modify check script to work without group file.
>
>Here is a logic change: before, even if test do not belong to any
>group (only iotest 142 currently) it should be defined in group
>file. Now, test is not forced to define any group. Instead check
>considers all files with names matching [0-9][0-9][0-9] as tests.
This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.
Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.
So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.
>check script is also refactored to make it simple to do next cool
>thing about iotests: allow meaningful names for test-case files.
This one would also require us to be able to distinguish test case files
from arbitrary other files.
> -#
> -# test-group association ... one line per test
> -#
> -001 rw auto quick
> -002 rw auto quick
> -003 rw auto
> -004 rw auto quick
> -005 img auto quick
> -# 006 was removed, do not reuse
We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)
Kevin
Re: [PATCH] iotests: drop group file
24.03.2020 10:57, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20200324074156.5330-1-vsement...@virtuozzo.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === CC s_normRoundPackToF64.o CC s_addMagsF64.o CC s_subMagsF64.o make: *** [check-tests/check-block.sh] Error 1 make: *** Waiting for unfinished jobs CC s_mulAddF64.o CC s_normSubnormalExtF80Sig.o --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ot7uydwt/src/docker-src.2020-03-24-03.54.58.28935:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ot7uydwt/src' make: *** [docker-run-test-quick@centos7] Error 2 real2m41.224s user0m7.941s The full log is available at http://patchew.org/logs/20200324074156.5330-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com Doesn't look as something related to my patch. -- Best regards, Vladimir
Re: [PATCH] iotests: drop group file
Patchew URL: https://patchew.org/QEMU/20200324074156.5330-1-vsement...@virtuozzo.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === CC s_normRoundPackToF64.o CC s_addMagsF64.o CC s_subMagsF64.o make: *** [check-tests/check-block.sh] Error 1 make: *** Waiting for unfinished jobs CC s_mulAddF64.o CC s_normSubnormalExtF80Sig.o --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ot7uydwt/src/docker-src.2020-03-24-03.54.58.28935:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=2b582229add741f387fcf0a88ff2a3ff make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ot7uydwt/src' make: *** [docker-run-test-quick@centos7] Error 2 real2m41.224s user0m7.941s The full log is available at http://patchew.org/logs/20200324074156.5330-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
