| Message ID | 20230813140950.678899-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | check: fix parsing expunge file with comments | expand |
On Sun, Aug 13, 2023 at 05:09:50PM +0300, Amir Goldstein wrote: > commit 60054d51 ("check: fix excluded tests are only expunged in the > first iteration") change to use exclude_tests array instead of file. > > The check if a test is in expunge file was using grep -q $TEST_ID FILE > so it was checking if the test was a non-exact match to one of the > lines, for a common example: "generic/001 # exclude this test" would be > a match to test generic/001. > > The commit regressed this example, because the new code checks for exact > match of [ "generic/001" == "generic/001 " ]. Change the code to use > prefix match to deal with this case and any other suffix correctly. > > While the original code would also match line "exclude test generic/001" > I don't think that non-prefix match is a real use case. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > check | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/check b/check > index 549725eb..e8152f59 100755 > --- a/check > +++ b/check > @@ -592,7 +592,8 @@ _expunge_test() > local TEST_ID="$1" > > for f in "${exclude_tests[@]}"; do > - if [ "${TEST_ID}" == "$f" ]; then > + # $f may contain traling spaces and comments > + if [[ "$f" == ${TEST_ID}* ]]; then We'll have seqnum with 4 numbers in one day, likes generic/1000. At that time, generic/100[0-9] will match generic/100*. That's not what we want. I think we'd better to do exact match, or better to match the end of a word, for example local id_regex="^${TEST_ID}\b" if [[ "$f" =~ ${id_regex} ]];then ... I think this will match "generic/100" and "generic/100 # xxxxx". But won't match "generic/1001". Thanks, Zorro > echo " [expunged]" > return 0 > fi > -- > 2.34.1 >
On Mon, Aug 14, 2023 at 5:50 PM Zorro Lang <zlang@redhat.com> wrote: > > On Sun, Aug 13, 2023 at 05:09:50PM +0300, Amir Goldstein wrote: > > commit 60054d51 ("check: fix excluded tests are only expunged in the > > first iteration") change to use exclude_tests array instead of file. > > > > The check if a test is in expunge file was using grep -q $TEST_ID FILE > > so it was checking if the test was a non-exact match to one of the > > lines, for a common example: "generic/001 # exclude this test" would be > > a match to test generic/001. > > > > The commit regressed this example, because the new code checks for exact > > match of [ "generic/001" == "generic/001 " ]. Change the code to use > > prefix match to deal with this case and any other suffix correctly. > > > > While the original code would also match line "exclude test generic/001" > > I don't think that non-prefix match is a real use case. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > check | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/check b/check > > index 549725eb..e8152f59 100755 > > --- a/check > > +++ b/check > > @@ -592,7 +592,8 @@ _expunge_test() > > local TEST_ID="$1" > > > > for f in "${exclude_tests[@]}"; do > > - if [ "${TEST_ID}" == "$f" ]; then > > + # $f may contain traling spaces and comments > > + if [[ "$f" == ${TEST_ID}* ]]; then > > We'll have seqnum with 4 numbers in one day, likes generic/1000. At that time, > generic/100[0-9] will match generic/100*. That's not what we want. > Good point. > I think we'd better to do exact match, or better to match the end of a word, > for example > > local id_regex="^${TEST_ID}\b" > if [[ "$f" =~ ${id_regex} ]];then > ... > > I think this will match "generic/100" and "generic/100 # xxxxx". But won't match > "generic/1001". > Nice. Looks good to me. Thanks, Amir.
On Mon, Aug 14, 2023 at 05:54:45PM +0300, Amir Goldstein wrote: > On Mon, Aug 14, 2023 at 5:50 PM Zorro Lang <zlang@redhat.com> wrote: > > > > On Sun, Aug 13, 2023 at 05:09:50PM +0300, Amir Goldstein wrote: > > > commit 60054d51 ("check: fix excluded tests are only expunged in the > > > first iteration") change to use exclude_tests array instead of file. > > > > > > The check if a test is in expunge file was using grep -q $TEST_ID FILE > > > so it was checking if the test was a non-exact match to one of the > > > lines, for a common example: "generic/001 # exclude this test" would be > > > a match to test generic/001. > > > > > > The commit regressed this example, because the new code checks for exact > > > match of [ "generic/001" == "generic/001 " ]. Change the code to use > > > prefix match to deal with this case and any other suffix correctly. > > > > > > While the original code would also match line "exclude test generic/001" > > > I don't think that non-prefix match is a real use case. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > check | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/check b/check > > > index 549725eb..e8152f59 100755 > > > --- a/check > > > +++ b/check > > > @@ -592,7 +592,8 @@ _expunge_test() > > > local TEST_ID="$1" > > > > > > for f in "${exclude_tests[@]}"; do > > > - if [ "${TEST_ID}" == "$f" ]; then > > > + # $f may contain traling spaces and comments > > > + if [[ "$f" == ${TEST_ID}* ]]; then > > > > We'll have seqnum with 4 numbers in one day, likes generic/1000. At that time, > > generic/100[0-9] will match generic/100*. That's not what we want. > > > > Good point. > > > I think we'd better to do exact match, or better to match the end of a word, > > for example > > > > local id_regex="^${TEST_ID}\b" > > if [[ "$f" =~ ${id_regex} ]];then > > ... > > > > I think this will match "generic/100" and "generic/100 # xxxxx". But won't match > > "generic/1001". > > > > Nice. Looks good to me. I didn't test it, please check if that can help you and won't break old things, if you'd like to write that in V2 :) Thanks, Zorro > > Thanks, > Amir. >
diff --git a/check b/check index 549725eb..e8152f59 100755 --- a/check +++ b/check @@ -592,7 +592,8 @@ _expunge_test() local TEST_ID="$1" for f in "${exclude_tests[@]}"; do - if [ "${TEST_ID}" == "$f" ]; then + # $f may contain traling spaces and comments + if [[ "$f" == ${TEST_ID}* ]]; then echo " [expunged]" return 0 fi
commit 60054d51 ("check: fix excluded tests are only expunged in the first iteration") change to use exclude_tests array instead of file. The check if a test is in expunge file was using grep -q $TEST_ID FILE so it was checking if the test was a non-exact match to one of the lines, for a common example: "generic/001 # exclude this test" would be a match to test generic/001. The commit regressed this example, because the new code checks for exact match of [ "generic/001" == "generic/001 " ]. Change the code to use prefix match to deal with this case and any other suffix correctly. While the original code would also match line "exclude test generic/001" I don't think that non-prefix match is a real use case. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- check | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)