Created attachment 64169 [details] [review] telepathy gabble tests pass/fail status This update will show more meaningful results for the tests, which is also very useful for automated tests framework.
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.