|
Description
Simon McVittie
2015-03-31 16:17:51 UTC
Oops, didn't mean to press Save with no description. What I meant to say is: Many of dbus' tests use GLib's GTest, which can be asked to output its results in TAP format, a structured format originating in Perl 1. For the others, we can get at least trivial TAP output quite easily. Created attachment 114770 [details] [review] [1/9] tests: avoid noise on stdout when not --verbose This makes life easier for frameworks like LAVA that screen-scrape test results. g_test_message() is not displayed by default, but each test can be run with either --tap or --verbose to get these messages displayed. Created attachment 114771 [details] [review] [2/9] tests: provide g_test_skip() emulation for older GLib We don't hard-depend on a new enough GLib to have g_test_skip(); if our GLib is older, fake it using g_test_message() and degrade to reporting it as a pass rather than a skip. Created attachment 114772 [details] [review] [3/9] test-shell, test-printf: produce TAP output like the other installable tests Created attachment 114773 [details] [review] [4/9] installed-tests: declare that the output is in TAP format For the ones written using GLib, the output is in TAP format if we say --tap. For the one not written using GLib, the output is in TAP and the command-line is ignored. Created attachment 114774 [details] [review] [5/9] installed-tests: don't set DBUS_TEST_HOME which is misleading It doesn't do anything - the variable I was thinking of is called DBUS_TEST_HOMEDIR. Also, if I had spelled it correctly, the tests would have failed, because libdbus (quite reasonably) won't create a nonexistent $HOME to write out cookie_sha1 files in ~/.dbus_keyrings. Created attachment 114775 [details] [review] [6/9] Depend on Automake 1.13 so we can use the correct AM_TESTS_ENVIRONMENT Since Automake 1.13 (released December 2012) the correct way for a maintainer to specify environment variables has been AM_TESTS_ENVIRONMENT, with TESTS_ENVIRONMENT reserved for the user. That doesn't work in older Automake, so drop support for such old versions. Created attachment 114776 [details] [review] [7/9] Run most tests under the TAP driver, with a simple adaptor for non-TAP tests --- The bits with tap-driver.sh are according to the Automake docs. Created attachment 114777 [details] [review] [8/9] Run name-test tests under the TAP driver Created attachment 114778 [details] [review] [9/9] Move Autoconf/Automake droppings into /build-aux/ Cc pwithnall, I heard you like TAP. Comment on attachment 114770 [details] [review] [1/9] tests: avoid noise on stdout when not --verbose Review of attachment 114770 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 114771 [details] [review] [2/9] tests: provide g_test_skip() emulation for older GLib Review of attachment 114771 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 114772 [details] [review] [3/9] test-shell, test-printf: produce TAP output like the other installable tests Review of attachment 114772 [details] [review]: ----------------------------------------------------------------- ::: test/internals/printf.c @@ +98,2 @@ > > + printf ("1..%d\n", test_num); Could do with a comment here mentioning that this is TAP output rather than some kind of satanic incantation (probably not necessary for the other printf()s). Ideally, also, this line (the test plan) would be printed first, so that if the test crashes part-way through, TAP knows that it's failed rather than finished (because the test plan has not completed). ::: test/shell-test.c @@ +153,5 @@ > + test_command_line ("evolution", "mailto:pepe@cuco.com", NULL); > + test_command_line ("run", "\"a \n multiline\"", NULL); > + test_command_line_fails ("ls", "\"a wrong string'", NULL); > + > + printf ("1..%d\n", test_num); Again, it would be better if the test plan were printed first. This could be achieved easily by using an array of char**s for the test vectors, rather than a list of calls to test_command_line*() and varargs. If you can be bothered to get that working neatly, go for it; otherwise, I wouldn't worry about moving the test plan. Comment on attachment 114773 [details] [review] [4/9] installed-tests: declare that the output is in TAP format Review of attachment 114773 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 114774 [details] [review] [5/9] installed-tests: don't set DBUS_TEST_HOME which is misleading Review of attachment 114774 [details] [review]: ----------------------------------------------------------------- heh, ++ Comment on attachment 114775 [details] [review] [6/9] Depend on Automake 1.13 so we can use the correct AM_TESTS_ENVIRONMENT Review of attachment 114775 [details] [review]: ----------------------------------------------------------------- Looks like there's another use of TESTS_ENVIRONMENT in tests/name-test/Makefile.am. Looks like the reference to it in doc/dbus-run-session.1.xml.in should also be changed. Comment on attachment 114776 [details] [review] [7/9] Run most tests under the TAP driver, with a simple adaptor for non-TAP tests Review of attachment 114776 [details] [review]: ----------------------------------------------------------------- ::: test/Makefile.am @@ +40,5 @@ > +LOG_DRIVER = env AM_TAP_AWK='$(AWK)' $(SHELL) $(top_srcdir)/tap-driver.sh > +LOG_COMPILER = $(srcdir)/glib-tap-test.sh > +SH_LOG_DRIVER = $(LOG_DRIVER) > +SH_LOG_COMPILER = $(SHELL) > +EXTRA_DIST += glib-tap-test.sh EXTRA_DIST += tap-test.sh.in? @@ +68,4 @@ > > if DBUS_UNIX > +wrap_bus_tests += test-bus-launch-helper.sh > +wrap_bus_tests += test-bus-system.sh Sorry if I'm missing it — where do $(wrap_bus_tests) and $(wrap_dbus_tests) get added to $(TESTS)? @@ +76,5 @@ > + < $(srcdir)/tap-test.sh.in > $@ > + > +$(wrap_dbus_tests): test-dbus%.sh: ../dbus/test-dbus%$(EXEEXT) tap-test.sh.in > + sed -e 's![@]RUN[@]!$<!' \ > + < $(srcdir)/tap-test.sh.in > $@ Shouldn't these: • depend on Makefile; • be listed in MOSTLYCLEANFILES (ottomh; might not be the most appropriate variable)? ::: test/glib-tap-test.sh @@ +2,5 @@ > + > +set -e > +t="$1" > +shift > +exec "$t" --tap "$@" A comment in this file explaining the magic and pointing to the relevant GLib documentation page would be great. ::: test/tap-test.sh.in @@ +16,5 @@ > + ;; > + (*) > + echo "not ok 1 @RUN@ (exit status $e)" > + ;; > +esac Likewise, a brief comment in here explaining the TAP voodoo would be lovely. Comment on attachment 114777 [details] [review] [8/9] Run name-test tests under the TAP driver Review of attachment 114777 [details] [review]: ----------------------------------------------------------------- ::: test/name-test/run-test-systemserver.sh @@ -7,5 @@ > - fi > - echo $SCRIPTNAME: $* >&2 > - > - exit 1 > -} For perfection, removing this should be a separate comment, I guess, since it was unused anyway. @@ +25,4 @@ > chmod 0700 "$XDG_RUNTIME_DIR" > export XDG_RUNTIME_DIR > > +interpret_result () { A comment explaining that this is for the benefit of TAP would be good. @@ +47,5 @@ > + expected_exit="$2" > + phrase="$3" > + shift > + shift > + shift `shift 3`? Or is that a bash-ism or something? I hate shell script's lame attempt at functions. @@ +52,5 @@ > + e=0 > + echo "# running test $t" > + "${DBUS_TOP_BUILDDIR}/libtool" --mode=execute $DEBUG "$DBUS_TOP_BUILDDIR/tools/dbus-send" "$@" > output.tmp 2>&1 || e=$? > + if [ $e != $expected_exit ]; then > + interpret_result "1" "$t" "$@" "(expected exit status $expected_exit, got $e)" Could cat output.tmp here to make debugging easier. Or delete it. (And in the other failure cases.) @@ +61,5 @@ > + sed -e 's/^/# /' < output.tmp > + interpret_result "1" "$t" "$@" "(Did not see \"$phrase\" in output)" > + return > + fi > + interpret_result "0" "$t" "$@" "(Saw \"$phrase\" in output as expected)" Need to `rm output.tmp` in the success case. @@ +64,5 @@ > + fi > + interpret_result "0" "$t" "$@" "(Saw \"$phrase\" in output as expected)" > +} > + > +py_test () { The `echo "running test echo signal"` seems to have got lost somewhere? @@ +79,3 @@ > > +test_num=1 > +echo "1..2" A comment explaining this is the TAP test plan would be useful. ::: test/name-test/run-test.sh @@ +23,4 @@ > chmod 0700 "$XDG_RUNTIME_DIR" > export XDG_RUNTIME_DIR > > +interpret_result () { # My name is interpret_result() and I am here to translate between sh and TAP. shTAP. @@ +54,2 @@ > > +py_test () { The version of py_test() in run-test-systemserver.sh seems a little neater? I was about to suggest that these *_test() sh functions be factored out into a little sh library which could be sourced from the two files. Then I realised I was thinking about an 'sh library' and had to be taken out back and shot. Would it be outrageous to suggest scrapping these two sh files and rewriting them in Python? I guess that would mean a hard dependency on Python which we are already explicitly avoiding in py_test(). So this is a roundabout way of saying: I continue to dislike sh, but there doesn't seem to be much we can do about it. Comment on attachment 114778 [details] [review] [9/9] Move Autoconf/Automake droppings into /build-aux/ Review of attachment 114778 [details] [review]: ----------------------------------------------------------------- \o/ (In reply to Philip Withnall from comment #14) > Could do with a comment here mentioning that this is TAP output rather than > some kind of satanic incantation (probably not necessary for the other > printf()s). Fair. > Ideally, also, this line (the test plan) would be printed first, so that if > the test crashes part-way through, TAP knows that it's failed rather than > finished (because the test plan has not completed). Not actually necessary. A TAP test that doesn't print a plan is considered to have failed overall; putting the plan at the end avoids having to count tests and hard-code how many there are going to be. % cat > t ok 1 ok 2 ok 3 % prove -e cat t t .. All 3 subtests passed Test Summary Report ------------------- t (Wstat: 0 Tests: 3 Failed: 0) Parse errors: No plan found in TAP output Files=1, Tests=3, 0 wallclock secs ( 0.03 usr + 0.00 sys = 0.03 CPU) Result: FAIL (In reply to Philip Withnall from comment #19) > For perfection, removing this should be a separate comment, I guess, since > it was unused anyway. Separated out. > A comment explaining that this is for the benefit of TAP would be good. Reasonable. > `shift 3`? Or is that a bash-ism or something? I hate shell script's lame > attempt at functions. POSIX says it's OK, so it's OK. > Could cat output.tmp here to make debugging easier. Or delete it. (And in > the other failure cases.) It should be cat'd. This is the only failure case where we have output in a temporary file: everywhere else, we don't need to grep the output, so we just send it to stdout. > Need to `rm output.tmp` in the success case. Sure. > The `echo "running test echo signal"` seems to have got lost somewhere? It doesn't add a great deal of value, but OK. > A comment explaining this is the TAP test plan would be useful. Possibly. Added. > The version of py_test() in run-test-systemserver.sh seems a little neater? Normalized. > I was about to suggest that these *_test() sh functions be factored out into > a little sh library which could be sourced from the two files. Then I > realised I was thinking about an 'sh library' and had to be taken out back > and shot. Yeah, no. The point at which your shell script, including its "library code", doesn't all fit on-screen is the point at which you should stop writing shell script. If we're going to do anything structural to this stuff, the right things would be: * change the 7 C tests so they run their own dbus-daemon using library code, output TAP, and preferably use GTest (next best thing: run each C test individually under run-with-tmp-session-bus.sh, dbus-run-session or a modified run-test.sh, using SH_LOG_COMPILER or something) * change the 2 Python tests so they run their own dbus-daemon using library code, and output TAP (next best thing: run each Python test individually under run-with-tmp-session-bus.sh, dbus-run-session or a modified run-test.sh, using PY_LOG_COMPILER or something; one complicating factor is that one wants a session-like bus and the other wants a system-like bus) * change the dbus-send-based test to be a small shell script outputting TAP in its own right, and run it under run-with-tmp-session-bus.sh using SH_LOG_COMPILER or something But I don't want to do those right now. (In reply to Philip Withnall from comment #17) > Comment on attachment 114775 [details] [review] > [6/9] Depend on Automake 1.13 so we can use the correct AM_TESTS_ENVIRONMENT (In reply to Philip Withnall from comment #18) > Comment on attachment 114776 [details] [review] > [7/9] Run most tests under the TAP driver, with a simple adaptor for > non-TAP tests I haven't replied to your comments here point-by-point, but they are all valid. (In reply to Simon McVittie from comment #24) > http://cgit.freedesktop.org/~smcv/dbus/log/?h=tests ++ to all the fixups. (In reply to Simon McVittie from comment #21) > (In reply to Philip Withnall from comment #14) > > Ideally, also, this line (the test plan) would be printed first, so that if > > the test crashes part-way through, TAP knows that it's failed rather than > > finished (because the test plan has not completed). > > Not actually necessary. A TAP test that doesn't print a plan is considered > to have failed overall; putting the plan at the end avoids having to count > tests and hard-code how many there are going to be. Cool, thanks for the clarification. Merged all this, thanks. |
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.