Bug 52049

Comment on attachment 64169 [details] [review]
telepathy gabble tests pass/fail status

Review of attachment 64169 [details] [review]:
-----------------------------------------------------------------

::: tests/twisted/run-test.sh.in
@@ +20,5 @@
>    sh @gabbletestsdir@/twisted/tools/with-session-bus.sh \
>      --config-file=@gabbletestsdir@/twisted/tools/servicedir/tmp-session-bus.conf \
>      -- \
>      @PYTHON@ @gabbletestsdir@/twisted/$i
> +  [ "$?" = 0 ] && echo "Result  $i: pass" || echo "Result  $i: fail"

If I was running the installed tests, I wouldn't want to have to grep the results to find out which ones passed/failed. The script should exit nonzero if a test fails.

There are two ways this could go: either exit nonzero as soon as a test fails, or carry on testing, and exit nonzero if at least one test failed.

Also, exiting with status 77 is special in Automake tests: it indicates that the test was skipped for some reason (neither success nor failure).

I'd prefer something like this:

any_failed=0
for i in $list; do
    ... run the test ...
    e=$?
    case "$e" in
        (0)
            echo "Result  $i: pass"
            ;;
        (77)
            echo "Result  $i: skip"
            ;;
        (*)
            any_failed=1
            echo "Result  $i: fail ($e)"
            ;;
    esac
done

exit $any_failed

(Or if you want to exit as soon as a test fails, $any_failed is not needed, and you can exit from the failure "case".)

(In reply to comment #1)
>         (0)
>             echo "Result  $i: pass"
>             ;;
>         (77)
>             echo "Result  $i: skip"
>             ;;
>         (*)
>             any_failed=1
>             echo "Result  $i: fail ($e)"
>             ;;

It's probably also easier to grep/search output if you use the same convention as Automake, which is:

PASS: a-test-that-passed
SKIP: a-test-that-exited-77
FAIL: a-test-that-failed

Then you can search for ^FAIL really easily, with fewer false positives from tests that happen to output "Testing recovery when GetBadger fails..." or whatever.

(In reply to comment #1)
> If I was running the installed tests, I wouldn't want to have to grep the
> results to find out which ones passed/failed. The script should exit nonzero if
> a test fails.
>

I guess this could also depend on the automated testing framework, talking more specifically about LAVA, the way it works is parsing the tests output, checking for 'pass' , 'fail' statuses and publishing meaningful information using this data to a dashboard. So I wrote this patch thinking in that case mainly. 

> There are two ways this could go: either exit nonzero as soon as a test fails,
> or carry on testing, and exit nonzero if at least one test failed.
> 

A proper exit code is always good, though from a LAVA point of view the exit code is not really important to process the tests data, so itis good and fine if you add a final non-zero exit code if any test fail, though the test should continue until the very end, a 'fail' status for a test is actually a valid result for LAVA , so even if all tests fail, they should be run and published to get this data.

> Also, exiting with status 77 is special in Automake tests: it indicates that
> the test was skipped for some reason (neither success nor failure).
> 

This is good, if you add a SKIP result to the script, we could parse this from LAVA too; basically, we can parse and map ANY_STATUS as long as it properly shows in the output of the test.

> I'd prefer something like this:
> 
> any_failed=0
> for i in $list; do
>     ... run the test ...
>     e=$?
>     case "$e" in
>         (0)
>             echo "Result  $i: pass"
>             ;;
>         (77)
>             echo "Result  $i: skip"
>             ;;
>         (*)
>             any_failed=1
>             echo "Result  $i: fail ($e)"
>             ;;
>     esac
> done
> 
> exit $any_failed
> 

This is very good++

> (Or if you want to exit as soon as a test fails, $any_failed is not needed, and
> you can exit from the failure "case".)

Let's run all the tests, after all, a fail value is a valid test data, so above script would make it very good too.

(In reply to comment #2)
> (In reply to comment #1)
> >         (0)
> >             echo "Result  $i: pass"
> >             ;;
> >         (77)
> >             echo "Result  $i: skip"
> >             ;;
> >         (*)
> >             any_failed=1
> >             echo "Result  $i: fail ($e)"
> >             ;;
> 
> It's probably also easier to grep/search output if you use the same convention
> as Automake, which is:
> 
> PASS: a-test-that-passed
> SKIP: a-test-that-exited-77
> FAIL: a-test-that-failed
> 
> Then you can search for ^FAIL really easily, with fewer false positives from
> tests that happen to output "Testing recovery when GetBadger fails..." or
> whatever.

This is also good, as long as we get a proper output stating "This TEST generated this RESULT" per line, pretty much anything could work; and basically my patch was oriented to include this format for the tests output.

(In reply to comment #3)
> I guess this could also depend on the automated testing framework, talking more
> specifically about LAVA, the way it works is parsing the tests output

Ugh, screen-scraping. I think this is a flawed design, but if it's what you have to work with, it's what you have to work with...

> This is very good++

Please put together an updated patch (potentially heavily based on what I wrote above) and I'll re-review.

Created attachment 64784 [details] [review]
New telepathy gabble tests patch

New revision for the telepathy gabble tests patch. It runs all the tests (even if one of them fails) and exit at the end with a proper status code.

Comment on attachment 64784 [details] [review]
New telepathy gabble tests patch

Review of attachment 64784 [details] [review]:
-----------------------------------------------------------------

::: tests/twisted/run-test.sh.in
@@ +24,5 @@
>      @PYTHON@ @gabbletestsdir@/twisted/$i
> +    e=$?
> +    case "$e" in
> +        (0)
> +            echo "Result  $i: pass"

Is there any reason not to use "PASS: $i", "SKIP: $i", "FAIL: $i" like Automake does? (Or "FAIL: $i ($e)" would be OK if you'd prefer to have the status included.)

I would much prefer the pass/fail/skip to be at the beginning of the line like that: "^FAIL" is an easier thing to grep for than "^Result.*fail", and much less likely to appear during normal operation as a result of a debug message or something. If you're going to screen-scrape test results like this, make them as unambiguous as possible.

(Consider a debug message that says "*** Tp-DEBUG: Result of badgering: failed to match mushroom to snake", for instance. You don't want to catch that as a false-negative failed test by grepping for "Result.*fail".)

(In reply to comment #7)
> Is there any reason not to use "PASS: $i", "SKIP: $i", "FAIL: $i" like Automake
> does? (Or "FAIL: $i ($e)" would be OK if you'd prefer to have the status
> included.)
> 

No special reason, this will work just fine.

> I would much prefer the pass/fail/skip to be at the beginning of the line like
> that: "^FAIL" is an easier thing to grep for than "^Result.*fail", and much
> less likely to appear during normal operation as a result of a debug message or
> something. If you're going to screen-scrape test results like this, make them
> as unambiguous as possible.
> 

Agreed, I will submit new patch with these changes.

Created attachment 64967 [details] [review]
New telepathy gabble tests patch (revision 2)

This patch uses the 'STATUS: test_name' format output; and even though it is not necessary to include the fail status code, I think it'd be nice to keep it for upstream.

Comment on attachment 64967 [details] [review]
New telepathy gabble tests patch (revision 2)

Review of attachment 64967 [details] [review]:
-----------------------------------------------------------------

Looks good to me, r+

Fixed in git for 0.17.1, thanks.

Next time, please provide a git commit (e.g. git format-patch and attach the result): see http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit?h=223a234ad1b30608 for what I consider a good commit message for this change to look like.