Bug 89846 - use TAP for tests
Summary: use TAP for tests
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: http://cgit.freedesktop.org/~smcv/dbu...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-03-31 16:17 UTC by Simon McVittie
Modified: 2015-04-16 12:11 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/9] tests: avoid noise on stdout when not --verbose (11.47 KB, patch)
2015-03-31 16:20 UTC, Simon McVittie
Details | Splinter Review
[2/9] tests: provide g_test_skip() emulation for older GLib (4.82 KB, patch)
2015-03-31 16:20 UTC, Simon McVittie
Details | Splinter Review
[3/9] test-shell, test-printf: produce TAP output like the other installable tests (6.16 KB, patch)
2015-03-31 16:20 UTC, Simon McVittie
Details | Splinter Review
[4/9] installed-tests: declare that the output is in TAP format (1.36 KB, patch)
2015-03-31 16:21 UTC, Simon McVittie
Details | Splinter Review
[5/9] installed-tests: don't set DBUS_TEST_HOME which is misleading (1.47 KB, patch)
2015-03-31 16:21 UTC, Simon McVittie
Details | Splinter Review
[6/9] Depend on Automake 1.13 so we can use the correct AM_TESTS_ENVIRONMENT (1.84 KB, patch)
2015-03-31 16:21 UTC, Simon McVittie
Details | Splinter Review
[7/9] Run most tests under the TAP driver, with a simple adaptor for non-TAP tests (3.94 KB, patch)
2015-03-31 16:22 UTC, Simon McVittie
Details | Splinter Review
[8/9] Run name-test tests under the TAP driver (7.35 KB, patch)
2015-03-31 16:23 UTC, Simon McVittie
Details | Splinter Review
[9/9] Move Autoconf/Automake droppings into /build-aux/ (1.95 KB, patch)
2015-03-31 16:24 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2015-03-31 16:17:51 UTC

    
Comment 1 Simon McVittie 2015-03-31 16:19:35 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.
Comment 2 Simon McVittie 2015-03-31 16:20:01 UTC
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.
Comment 3 Simon McVittie 2015-03-31 16:20:27 UTC
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.
Comment 4 Simon McVittie 2015-03-31 16:20:54 UTC
Created attachment 114772 [details] [review]
[3/9] test-shell, test-printf: produce TAP output like the  other installable tests
Comment 5 Simon McVittie 2015-03-31 16:21:15 UTC
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.
Comment 6 Simon McVittie 2015-03-31 16:21:29 UTC
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.
Comment 7 Simon McVittie 2015-03-31 16:21:46 UTC
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.
Comment 8 Simon McVittie 2015-03-31 16:22:54 UTC
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.
Comment 9 Simon McVittie 2015-03-31 16:23:47 UTC
Created attachment 114777 [details] [review]
[8/9] Run name-test tests under the TAP driver
Comment 10 Simon McVittie 2015-03-31 16:24:02 UTC
Created attachment 114778 [details] [review]
[9/9] Move Autoconf/Automake droppings into /build-aux/
Comment 11 Simon McVittie 2015-03-31 16:24:26 UTC
Cc pwithnall, I heard you like TAP.
Comment 12 Philip Withnall 2015-04-01 10:29:53 UTC
Comment on attachment 114770 [details] [review]
[1/9] tests: avoid noise on stdout when not --verbose

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

++
Comment 13 Philip Withnall 2015-04-01 10:30:00 UTC
Comment on attachment 114771 [details] [review]
[2/9] tests: provide g_test_skip() emulation for older GLib

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

++
Comment 14 Philip Withnall 2015-04-01 10:30:12 UTC
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 15 Philip Withnall 2015-04-01 10:30:17 UTC
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 16 Philip Withnall 2015-04-01 10:30:22 UTC
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 17 Philip Withnall 2015-04-01 10:30:27 UTC
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 18 Philip Withnall 2015-04-01 10:30:40 UTC
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 19 Philip Withnall 2015-04-01 10:30:50 UTC
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 20 Philip Withnall 2015-04-01 10:30:57 UTC
Comment on attachment 114778 [details] [review]
[9/9] Move Autoconf/Automake droppings into /build-aux/

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

\o/
Comment 21 Simon McVittie 2015-04-01 11:44:55 UTC
(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
Comment 22 Simon McVittie 2015-04-01 12:48:05 UTC
(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.
Comment 23 Simon McVittie 2015-04-01 12:49:18 UTC
(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.
Comment 24 Simon McVittie 2015-04-01 12:49:51 UTC
http://cgit.freedesktop.org/~smcv/dbus/log/?h=tests
Comment 25 Philip Withnall 2015-04-13 15:32:53 UTC
(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.
Comment 26 Simon McVittie 2015-04-16 12:11:23 UTC
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.