Fix regression in exclude and include

[asmorodskyi]

This patch is follow up for 13ff090 and c0f37f2 which were trying to fix support for lists in exclude and include but at the same time broke support for regexs Current version keep support for lists and fixing regex string support for exclude and include.

[grisu48]

This addresses https://github.com/metan-ucw/runltp-ng/issues/32

[pevik]

@metan-ucw @wangli5665 I wonder if we should simply revert 13ff090eeb9ad69e02531649847c9adabe1c8e48 c0f37f2fa4507540f8bd47f23376fcb5914bf632, because including/excluding more tests should be done with original approach using |, e.g.: --exclude="access03|access04" or --exclude="access0[34]". Maybe we should add an example to help output, as we all three included in the original commits got confused :).

If we take this approach, we should do the same for include. And there could be git tags:

Fixes: c0f37f2 ("utils: to support excluding more tests")
Reported-by: Felix Niederwanger <felix.niederwanger@suse.com>

[metan-ucw]

Well I think that it would be nice to have both, since the original runltp script worked with a lists, so this would make transition easier.

Also shouldn't we do this for the include option as well?

[pevik]

Also shouldn't we do this for the include option as well?

Yes (I already mentioned that here and in https://github.com/metan-ucw/runltp-ng/issues/32).

[pevik]

Well I think that it would be nice to have both, since the original runltp script worked with a lists, so this would make transition easier.

So is that your ack to this commit? I thought 2 options would be a bit confusing, but have compatibility with the old script is important thus ack from me. But I'd add examples to help. BTW we should probably have some tests for options.

[richiejp]

Instead we could add a flag to enable regex on the include and exclude options. Or have a --filter which is just regex and leave include and exclude as simple lists.

regex is powerfull enough to emulate include exclude and lists, but then you have the problem of not being able to understand it xD

[pevik]

Instead we could add a flag to enable regex on the include and exclude options. Or have a --filter which is just regex and leave include and exclude as simple lists.

Both sounds reasonable, I slightly prefer the second one.

regex is powerfull enough to emulate include exclude and lists, but then you have the problem of not being able to understand it xD

Obviously :).

[wangli5665]

Well I think that it would be nice to have both, since the original runltp script worked with a lists, so this would make transition easier.

Okay, I didn't aware that has regex support before, so just considering for general list excluding like the original script does. Sorry for the broken this (actually we have not deployed this in our automation job, only a quick try from my site).

Also shouldn't we do this for the include option as well?

Yes, we should.

[wangli5665]

I thought 2 options would be a bit confusing, but have compatibility with the old script is important thus ack from me. But I'd add examples to help.

Agree, it looks indeed confusing to have two options for excluding, I also think we can consider Richard's suggetion.

And, +1 for adding examples to help.

BTW we should probably have some tests for options.

+1

[wangli5665]

Considering that $tid is only a single word/string, it's safe to reverse the order of the binding operators( =~ ) to combine regex and list support. So another simplest way, I can think of, is to filter the $tid in exclude/include list first, if fail (that means not match list or it's a regex string), then just reverse the order, do regular regex filtering, that will satisfy what we expect on both sides.

--- a/utils.pm
+++ b/utils.pm
@@ -411,8 +411,8 @@ sub run_ltp
                chomp;

                my ($tid, $c) = parse_test($runtest, $_);
-               next unless (!$include || $include =~ $tid);
-               next if ($exclude && $exclude =~ $tid);
+               next unless (!$include || ($include =~ $tid || $tid =~ $include));
+               next if ($exclude && ($exclude =~ $tid || $tid =~ $exclude));

                print("Executing $tid\n");
                my $test_start_time = clock_gettime(CLOCK_MONOTONIC);

I have confirmed it works well on below usage:

--exclude="access0[13]"
--exclude="access01 access03"
--exclude="access01, access03"
--exclude '_16\$|^(move_pages|access0[13]|msg(ctl|get|rcv|snd|stress)|sem(ctl|get|op|snd)|shm(at|ctl|dt|get)|fallocate06|syslog|inotify07|inotify08|epoll_wait02|getrusage04)'

@grisu48 @asmorodskyi Can you help test this from your side?

[asmorodskyi]

Considering that $tid is only a single word/string, it's safe to reverse the order of the binding operators( =~ ) to combine regex and list support. So another simplest way, I can think of, is to filter the $tid in exclude/include list first, if fail (that means not match list or it's a regex string), then just reverse the order, do regular regex filtering, that will satisfy what we expect on both sides.

--- a/utils.pm
+++ b/utils.pm
@@ -411,8 +411,8 @@ sub run_ltp
                chomp;

                my ($tid, $c) = parse_test($runtest, $_);
-               next unless (!$include || $include =~ $tid);
-               next if ($exclude && $exclude =~ $tid);
+               next unless (!$include || ($include =~ $tid || $tid =~ $include));
+               next if ($exclude && ($exclude =~ $tid || $tid =~ $exclude));

                print("Executing $tid\n");
                my $test_start_time = clock_gettime(CLOCK_MONOTONIC);

I have confirmed it works well on below usage:

--exclude="access0[13]"
--exclude="access01 access03"
--exclude="access01, access03"
--exclude '_16\$|^(move_pages|access0[13]|msg(ctl|get|rcv|snd|stress)|sem(ctl|get|op|snd)|shm(at|ctl|dt|get)|fallocate06|syslog|inotify07|inotify08|epoll_wait02|getrusage04)'

@grisu48 @asmorodskyi Can you help test this from your side?

1. yeah maybe current version will looks a bit confusing for the user , so we need to find something else

2. on other hand I would say that such complex logical expression would be confusing and error prone from code perspective ( for me at least )

   after reading all discussions I would suggest the following :

3. revert 3b6f649
4. check where parameter for exclude is list of regex . in case this is the list convert it to regex on the fly ( e.g. access01 access03 would become access01|access03)

[wangli5665]

after reading all discussions I would suggest the following :

1. revert 3b6f649
2. check where parameter for exclude is list of regex . in case this is the list convert it to regex on the fly ( e.g. access01 access03 would become access01|access03)

Thanks, as long as we support both list and regex, I don't care how to achieve the code much, because it'd be nice to have the list to respect the original runltp user.

I don't think my reverse is complex (or maybe I should added one line comments). But I'm OK to go with your suggestion too, that will make things easy to understand.

Let's hear more voices from peers.

[pevik]

I think Li's version is good enough, but maybe followed by inline comment (+ of course update help output). But let's @metan-ucw to decide final version.

-               next unless (!$include || $include =~ $tid);
-               next if ($exclude && $exclude =~ $tid);
+               next unless (!$include || ($include =~ $tid || $tid =~ $include));
+               next if ($exclude && ($exclude =~ $tid || $tid =~ $exclude));

[asmorodskyi]

if everyone would accept Li's version , I would take my words back and switch my PR to version provided by him.

[metan-ucw]

A was thinking about this for a while and I think that the version from Li should work fine for us, so I would go with that.

[asmorodskyi]

A was thinking about this for a while and I think that the version from Li should work fine for us, so I would go with that.

than I will change my patch to version suggested by Li and will try to run some VR's with that

[asmorodskyi]

Patch is updated and commit message too . Also I did some artificial runs using current filter which we using in openQA . Looks like new version correctly covering our case at least. @wangli5665 thanks for the help :+1: Ready to be merged from my side. Please tell me if anything else needs to be done.

[pevik]

I updated commit message and merged. Thanks! TODO: add a CI test for regexes mentioned by @wangli5665 and tested by @asmorodskyi.