| Message ID | 20190131084959.10639-1-uwe@kleine-koenig.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | validation: Add patterns FAIL, PASS, XPASS and XFAIL to test | expand |
On 1/31/19 9:49 AM, Uwe Kleine-König wrote: > This simplifies finding the offending test when the build ended with > > KO: out of 584 tests, 527 passed, 57 failed > 56 of them are known to fail > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > validation/test-suite | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/validation/test-suite b/validation/test-suite > index f79a9023d95a..a5572d5e6965 100755 > --- a/validation/test-suite > +++ b/validation/test-suite > @@ -403,9 +403,16 @@ do_test() > if [ "$must_fail" -eq "1" ]; then > if [ "$test_failed" -eq "1" ]; then > [ -z "$vquiet" ] && \ > - echo "info: test '$file' is known to fail" > + echo "info: XFAIL: test '$file' is known to fail" > else > - echo "error: test '$file' is known to fail but succeed!" > + echo "error: XPASS: test '$file' is known to fail but succeed!" > + fi > + else > + if [ "$test_failed" -eq "1" ]; then > + echo "error: FAIL: test '$file' is failed" When reading my own patch after it returned to me in my sparse folder I noticed a fault here. We want the "is" dropped in the above line. Should I resend for that? Best regards Uwe
On Thu, Jan 31, 2019 at 10:23:44AM +0100, Uwe Kleine-König wrote: > On 1/31/19 9:49 AM, Uwe Kleine-König wrote: > > This simplifies finding the offending test when the build ended with > > > > KO: out of 584 tests, 527 passed, 57 failed > > 56 of them are known to fail > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > --- > > validation/test-suite | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/validation/test-suite b/validation/test-suite > > index f79a9023d95a..a5572d5e6965 100755 > > --- a/validation/test-suite > > +++ b/validation/test-suite > > @@ -403,9 +403,16 @@ do_test() > > if [ "$must_fail" -eq "1" ]; then > > if [ "$test_failed" -eq "1" ]; then > > [ -z "$vquiet" ] && \ > > - echo "info: test '$file' is known to fail" > > + echo "info: XFAIL: test '$file' is known to fail" > > else > > - echo "error: test '$file' is known to fail but succeed!" > > + echo "error: XPASS: test '$file' is known to fail but succeed!" > > + fi > > + else > > + if [ "$test_failed" -eq "1" ]; then > > + echo "error: FAIL: test '$file' is failed" > > When reading my own patch after it returned to me in my sparse folder I > noticed a fault here. We want the "is" dropped in the above line. Should > I resend for that? No no, a resend is not needed at all, I've directly changed it. -- Luc
On Thu, Jan 31, 2019 at 09:49:59AM +0100, Uwe Kleine-König wrote: > This simplifies finding the offending test when the build ended with > > KO: out of 584 tests, 527 passed, 57 failed > 56 of them are known to fail > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > validation/test-suite | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/validation/test-suite b/validation/test-suite > index f79a9023d95a..a5572d5e6965 100755 > --- a/validation/test-suite > +++ b/validation/test-suite > @@ -403,9 +403,16 @@ do_test() > if [ "$must_fail" -eq "1" ]; then > if [ "$test_failed" -eq "1" ]; then > [ -z "$vquiet" ] && \ > - echo "info: test '$file' is known to fail" > + echo "info: XFAIL: test '$file' is known to fail" > else > - echo "error: test '$file' is known to fail but succeed!" > + echo "error: XPASS: test '$file' is known to fail but succeed!" > + fi > + else > + if [ "$test_failed" -eq "1" ]; then > + echo "error: FAIL: test '$file' is failed" > + else > + [ -z "$vquiet" ] && \ > + echo "info: PASS: test '$file' passed" > fi > fi I can understand the motivation for this but with this the default *visual* output will be even longer and more redundant: 1) The current idea was if a test doesn't output sommething it's that everything was OK. With the patch there will be a 'PASS' line for each of these tests (more than 90% of them). 2) every FAIL tests have already of line displayed with "error: test ... failed" just before the details of the failure are displayed (which means that such a line can be displayed several time for the same test: bad) and the line with FAIL is displayed at the end of the test. 3) in the same message 'XFAIL' & 'is known to fail', 'XPASS' & 'is known to fail but succeed' and 'FAIL' & 'failed' really mean the same (and all of them are grepable); it's just that one seems to for humans and the others for some script. Point 3) doesn't bother me much. Point 2) is easily fixable (both removing the current repetition in case of multiple failure in the same test file and the repetion with FAIL). Point 1) really bothers me. I propose to: *) simply drop the 'PASS' part of the patch *) add a new flag for test-suite (for example --machine-testing) to: -) not display the 'PASS' lines when the flag is not set -) display the FAIL, XFAIL, ... tags only when the flag is set (I suppose that these FAIL, XFAIL, XPASS ... tags will be used or are already used in some concrete automatic system so it wouldn't be a problem to launch the testsuite with a flag). This will keep the current default output (human-)user friendly. Any thoughts? -- Luc
On Thu, Jan 31, 2019 at 02:45:04PM +0100, Luc Van Oostenryck wrote: > On Thu, Jan 31, 2019 at 09:49:59AM +0100, Uwe Kleine-König wrote: > > This simplifies finding the offending test when the build ended with > > > > KO: out of 584 tests, 527 passed, 57 failed > > 56 of them are known to fail > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > --- > > validation/test-suite | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/validation/test-suite b/validation/test-suite > > index f79a9023d95a..a5572d5e6965 100755 > > --- a/validation/test-suite > > +++ b/validation/test-suite > > @@ -403,9 +403,16 @@ do_test() > > if [ "$must_fail" -eq "1" ]; then > > if [ "$test_failed" -eq "1" ]; then > > [ -z "$vquiet" ] && \ > > - echo "info: test '$file' is known to fail" > > + echo "info: XFAIL: test '$file' is known to fail" > > else > > - echo "error: test '$file' is known to fail but succeed!" > > + echo "error: XPASS: test '$file' is known to fail but succeed!" > > + fi > > + else > > + if [ "$test_failed" -eq "1" ]; then > > + echo "error: FAIL: test '$file' is failed" > > + else > > + [ -z "$vquiet" ] && \ > > + echo "info: PASS: test '$file' passed" > > fi > > fi > > I can understand the motivation for this but with this the default > *visual* output will be even longer and more redundant: > 1) The current idea was if a test doesn't output sommething > it's that everything was OK. With the patch there will be a > 'PASS' line for each of these tests (more than 90% of them). For me the typical passing test looks as follows: TEST extern inline function (extern-inline.c) Using command : sparse $file $file Expecting exit value: 0 that's because the package build scripts passes V=1 to all invocations of make; without V=1 there is only the TEST line for succeeding tests. For a build log I think it is sensible to have it more verbose to make eventual failures easier to understand, so passing V=1 is good. (This is even recommended practise for Debian packages.) The output for one of the 56 tests that fail expectedly the output looks as follows (with V=1): TEST enum+mode (enum+mode.c) Using command : sparse $file Expecting exit value: 0 error: actual error text does not match expected error text. error: see enum+mode.c.error.* for further investigation. --- enum+mode.c.error.expected 2019-01-31 03:18:58.329248245 +0000 +++ enum+mode.c.error.got 2019-01-31 03:18:58.329248245 +0000 @@ -0,0 +1,5 @@ +enum+mode.c:4:50: error: don't know how to apply mode to unsigned int enum e +enum+mode.c:5:50: error: don't know how to apply mode to unsigned int enum e +enum+mode.c:6:48: error: don't know how to apply mode to unsigned int enum e +enum+mode.c:11:28: error: static assertion failed: "" +enum+mode.c:13:28: error: static assertion failed: "" info: test 'enum+mode.c' is known to fail which makes it hard to find the one unexpectedly failing test because there is no pattern to look for. I need something with "error" that isn't followd by "is known to fail". > 2) every FAIL tests have already of line displayed with > "error: test ... failed" just before the details of the > failure are displayed (which means that such a line can be > displayed several time for the same test: bad) and the line > with FAIL is displayed at the end of the test. > 3) in the same message 'XFAIL' & 'is known to fail', > 'XPASS' & 'is known to fail but succeed' and 'FAIL' & 'failed' > really mean the same (and all of them are grepable); it's just that > one seems to for humans and the others for some script. My usecase is really to check build logs (e.g. https://buildd.debian.org/status/fetch.php?pkg=sparse&arch=mipsel&ver=0.6.0-1&stamp=1548904760&raw=0). If I search for "failed" I get 36 matches. "error: test" yields 32. > Point 3) doesn't bother me much. > Point 2) is easily fixable (both removing the current repetition in > case of multiple failure in the same test file and the repetion with > FAIL). > Point 1) really bothers me. > > I propose to: > *) simply drop the 'PASS' part of the patch > *) add a new flag for test-suite (for example --machine-testing) to: > -) not display the 'PASS' lines when the flag is not set > -) display the FAIL, XFAIL, ... tags only when the flag is set > (I suppose that these FAIL, XFAIL, XPASS ... tags will be used or > are already used in some concrete automatic system so it wouldn't be > a problem to launch the testsuite with a flag). > This will keep the current default output (human-)user friendly. > > Any thoughts? If you want to keep it short by default, what about dropping the line TEST extern inline function (extern-inline.c) before starting the test and make this PASS extern inline function (extern-inline.c) at the end of the test instead? Then instead of TEST bitfield-bool-layout (bitfield-bool-layout.c) info: test 'bitfield-bool-layout.c' is known to fail we could have: XFAIL bitfield-bool-layout (bitfield-bool-layout.c) which is even shorter. And a failing test could look as follows: error: test 'preprocessor/predef-lp64.c' failed error: Pattern 'ret\..*\$0' unexpectedly absent FAIL predefined macros for LP64 (preprocessor/predef-lp64.c) (Maybe even drop the error: lines in non-verbose mode?) Best regards Uwe
On Thu, Jan 31, 2019 at 10:13:25PM +0100, Uwe Kleine-König wrote: > On Thu, Jan 31, 2019 at 02:45:04PM +0100, Luc Van Oostenryck wrote: > > On Thu, Jan 31, 2019 at 09:49:59AM +0100, Uwe Kleine-König wrote: > > > This simplifies finding the offending test when the build ended with > > > > > > KO: out of 584 tests, 527 passed, 57 failed > > > 56 of them are known to fail > > > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > > --- > > > validation/test-suite | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/validation/test-suite b/validation/test-suite > > > index f79a9023d95a..a5572d5e6965 100755 > > > --- a/validation/test-suite > > > +++ b/validation/test-suite > > > @@ -403,9 +403,16 @@ do_test() > > > if [ "$must_fail" -eq "1" ]; then > > > if [ "$test_failed" -eq "1" ]; then > > > [ -z "$vquiet" ] && \ > > > - echo "info: test '$file' is known to fail" > > > + echo "info: XFAIL: test '$file' is known to fail" > > > else > > > - echo "error: test '$file' is known to fail but succeed!" > > > + echo "error: XPASS: test '$file' is known to fail but succeed!" > > > + fi > > > + else > > > + if [ "$test_failed" -eq "1" ]; then > > > + echo "error: FAIL: test '$file' is failed" > > > + else > > > + [ -z "$vquiet" ] && \ > > > + echo "info: PASS: test '$file' passed" > > > fi > > > fi > > > > I can understand the motivation for this but with this the default > > *visual* output will be even longer and more redundant: > > 1) The current idea was if a test doesn't output sommething > > it's that everything was OK. With the patch there will be a > > 'PASS' line for each of these tests (more than 90% of them). > > For me the typical passing test looks as follows: > > TEST extern inline function (extern-inline.c) > Using command : sparse $file $file > Expecting exit value: 0 > > that's because the package build scripts passes V=1 to all invocations > of make; without V=1 there is only the TEST line for succeeding tests. OK, I see. But with your patch, doesn't it also output the PASS line? TEST extern inline function (extern-inline.c) Using command : sparse $file $file Expecting exit value: 0 info: PASS: test 'extern-inline.c' passed > For a build log I think it is sensible to have it more verbose to make > eventual failures easier to understand, so passing V=1 is good. (This is > even recommended practise for Debian packages.) Sure, it makes totally sense. > The output for one of the 56 tests that fail expectedly the output looks > as follows (with V=1): > > TEST enum+mode (enum+mode.c) > Using command : sparse $file > Expecting exit value: 0 > error: actual error text does not match expected error text. > error: see enum+mode.c.error.* for further investigation. > --- enum+mode.c.error.expected 2019-01-31 03:18:58.329248245 +0000 > +++ enum+mode.c.error.got 2019-01-31 03:18:58.329248245 +0000 > @@ -0,0 +1,5 @@ > +enum+mode.c:4:50: error: don't know how to apply mode to unsigned int enum e > +enum+mode.c:5:50: error: don't know how to apply mode to unsigned int enum e > +enum+mode.c:6:48: error: don't know how to apply mode to unsigned int enum e > +enum+mode.c:11:28: error: static assertion failed: "" > +enum+mode.c:13:28: error: static assertion failed: "" > info: test 'enum+mode.c' is known to fail > > which makes it hard to find the one unexpectedly failing test because > there is no pattern to look for. I need something with "error" that > isn't followd by "is known to fail". Yes, I agree. The handling of known-to-fail is quite bad in verbose mode. > > > 2) every FAIL tests have already of line displayed with > > "error: test ... failed" just before the details of the > > failure are displayed (which means that such a line can be > > displayed several time for the same test: bad) and the line > > with FAIL is displayed at the end of the test. > > 3) in the same message 'XFAIL' & 'is known to fail', > > 'XPASS' & 'is known to fail but succeed' and 'FAIL' & 'failed' > > really mean the same (and all of them are grepable); it's just that > > one seems to for humans and the others for some script. > > My usecase is really to check build logs (e.g. > https://buildd.debian.org/status/fetch.php?pkg=sparse&arch=mipsel&ver=0.6.0-1&stamp=1548904760&raw=0). > > If I search for "failed" I get 36 matches. "error: test" yields 32. > > > Point 3) doesn't bother me much. > > Point 2) is easily fixable (both removing the current repetition in > > case of multiple failure in the same test file and the repetion with > > FAIL). > > Point 1) really bothers me. > > > > I propose to: > > *) simply drop the 'PASS' part of the patch > > *) add a new flag for test-suite (for example --machine-testing) to: > > -) not display the 'PASS' lines when the flag is not set > > -) display the FAIL, XFAIL, ... tags only when the flag is set > > (I suppose that these FAIL, XFAIL, XPASS ... tags will be used or > > are already used in some concrete automatic system so it wouldn't be > > a problem to launch the testsuite with a flag). > > This will keep the current default output (human-)user friendly. > > > > Any thoughts? > > If you want to keep it short by default, what about dropping the line > > TEST extern inline function (extern-inline.c) > > before starting the test and make this > > PASS extern inline function (extern-inline.c) > > at the end of the test instead? Then instead of > > TEST bitfield-bool-layout (bitfield-bool-layout.c) > info: test 'bitfield-bool-layout.c' is known to fail > > we could have: > > XFAIL bitfield-bool-layout (bitfield-bool-layout.c) > > which is even shorter. And a failing test could look as follows: > > error: test 'preprocessor/predef-lp64.c' failed > error: Pattern 'ret\..*\$0' unexpectedly absent > FAIL predefined macros for LP64 (preprocessor/predef-lp64.c) > > (Maybe even drop the error: lines in non-verbose mode?) The actual use of the 'TEST' lines is partially historical and partially because I tend to like to have a line with the name of the file before any details of the error. Now that I understand your usecase and you make me remember the problem with the output of known-to-fail test, I'll cook something that should satisfy everybody (but I don't think that I'll be be able to do that tomorrow, this WE is FOSDEM, ...). Thanks for everything. -- Luc
diff --git a/validation/test-suite b/validation/test-suite index f79a9023d95a..a5572d5e6965 100755 --- a/validation/test-suite +++ b/validation/test-suite @@ -403,9 +403,16 @@ do_test() if [ "$must_fail" -eq "1" ]; then if [ "$test_failed" -eq "1" ]; then [ -z "$vquiet" ] && \ - echo "info: test '$file' is known to fail" + echo "info: XFAIL: test '$file' is known to fail" else - echo "error: test '$file' is known to fail but succeed!" + echo "error: XPASS: test '$file' is known to fail but succeed!" + fi + else + if [ "$test_failed" -eq "1" ]; then + echo "error: FAIL: test '$file' is failed" + else + [ -z "$vquiet" ] && \ + echo "info: PASS: test '$file' passed" fi fi
This simplifies finding the offending test when the build ended with KO: out of 584 tests, 527 passed, 57 failed 56 of them are known to fail Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- validation/test-suite | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)