[Bug 41448] Installing unit tests

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Fri Oct 28 15:24:27 CEST 2011 

https://bugs.freedesktop.org/show_bug.cgi?id=41448

Alban Crequy <alban.crequy at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://cgit.collabora.com/g |http://cgit.collabora.com/g
                   |it/user/alban/telepathy-gab |it/user/alban/telepathy-gab
                   |ble.git/log/?h=tests4       |ble.git/log/?h=tests5

--- Comment #11 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-10-21 04:22:24 PDT ---
> +GABBLE_TESTS_PATH=${datarootdir}/telepathy-gabble-tests
...
> +if ENABLE_INSTALLED_TESTS
> +gabbletestsdir = $(GABBLE_TESTS_PATH)
> +gabbletests_PROGRAMS = $(tests_list)
> +gabbletests_DATA = gabble-C-tests.list

Machine-specific binaries in /usr/share/telepathy-gabble-tests? No thanks.

I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or
${libdir}/telepathy/gabble-tests or something), but you could split the
architecture-independent bits into /usr/share if you want to go that far.

If you rename GABBLE_TESTS_PATH to gabbletestsdir throughout, and change
"gabbletestsdir = $(GABBLE_TESTS_PATH)" to "gabbletestsdir = @gabbletestsdir@"
(or you might even be able to delete it entirely - automake might put that line
in automatically), you'll have one less redundant variable, => one less source
of confusion.

> tests: add --enable-install-tests

You renamed it, amend the commit message please.

> + test-debug.py \
> + test-fallback-socks5-proxy.py \
> + test-register.py \
> + test-location.py \
> + pubsub.py \
> + sidecar-own-caps.py \
> + sidecars.py \
> + mail-notification.py \
> + client-types.py \
> + gateways.py \
> + last-activity.py \
> + pep-support.py \
> + plugin-channel-managers.py \
> + power-save.py \
> + version.py \
> + dataforms.py \

If you're going to permute the order of things in a big list, please sort the
new positions alphabetically while you're changing it anyway. That could be a
separate, first, commit.

> -TWISTED_JINGLE_TESTS = \

You've removed the (documented!) ability to "make check TWISTED_JINGLE_TESTS="
when you know you haven't touched Jingle-specific code, so that it doesn't take
6 months to run the tests and then fail with an obscure race condition. (Or
perhaps we've fixed the Jingle tests enough by now that only the first reason
applies? Even so, it's a useful feature.)

> + $(AM_V_GEN)sed -e "s|[@]abs_top_tests_installdir[@]|$(GABBLE_TESTS_PATH)|g" \
> + -e "s|[@]python[@]|$(PYTHON)|g" \

abs_top_tests_installdir is a bit misleading: it looks like a standard autoconf
substitution, but it isn't. I'd use @GABBLE_TESTS_PATH@, or even
@gabbletestsdir at .

It's conventional to replace @Thing@ with $(Thing) (in any case combination),
so the placeholder for Python should be @PYTHON@ instead of @python at .

> + @mkdir -p tools

Not portable. Use $(MKDIR_P) instead (and make sure we have AC_PROG_MKDIR_P in
the configure.ac, but I think we do already).

> +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> + echo "This script is meant to be installed"

You could at least say "... installed at $script_fullname".

I'm not sure whether this use of readlink is portable away from Linux, but I'm
not sure that I care either.

> +export PYTHONPATH=@abs_top_tests_installdir@/twisted

Not portable to all POSIX shells, use

    PYTHONPATH=...
    export PYTHONPATH

(and the same in all #!/bin/sh scripts).

--- Comment #12 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-10-21 04:30:25 PDT ---
> # because test.la is not installed, libtool will want to compile it as static
> # despite -shared (a convenience library), unless we also use -rpath
> -test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)
> +test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(testplugindir)

This won't work in the not-installing-tests case, because testplugindir won't
be defined.

I'd just move "test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)" inside the
"not installing tests" branch of the conditional. If test.la is going to be
installed in $(testplugindir), it gets that as an rpath automatically, so you
don't need special workarounds to force it to be a shared library.

--- Comment #13 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2011-10-21 05:09:41 PDT ---
(In reply to comment #11)
> I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or
> ${libdir}/telepathy/gabble-tests or something), but you could split the
> architecture-independent bits into /usr/share if you want to go that far.

Note that under Red-Hat-style biarch and Debian-style multiarch, nothing that
encodes an architecture-specific path (like ${libdir}, which will typically be
something like /usr/lib64 on a biarch system or /usr/lib/x86_64-linux-gnu on a
multiarch system) can go in /usr/share either.

--- Comment #14 from Alban Crequy <alban.crequy at collabora.co.uk> 2011-10-21 09:07:52 PDT ---
Thanks for the reviews.

(In reply to comment #11)
> > +GABBLE_TESTS_PATH=${datarootdir}/telepathy-gabble-tests
> ...
> > +if ENABLE_INSTALLED_TESTS
> > +gabbletestsdir = $(GABBLE_TESTS_PATH)
> > +gabbletests_PROGRAMS = $(tests_list)
> > +gabbletests_DATA = gabble-C-tests.list
> 
> Machine-specific binaries in /usr/share/telepathy-gabble-tests? No thanks.
> 
> I'd personally just move all the tests to ${libdir}/telepathy-gabble-tests (or
> ${libdir}/telepathy/gabble-tests or something), but you could split the
> architecture-independent bits into /usr/share if you want to go that far.

Moved to ${libdir}/telepathy-gabble-tests.

> If you rename GABBLE_TESTS_PATH to gabbletestsdir throughout, and change
> "gabbletestsdir = $(GABBLE_TESTS_PATH)" to "gabbletestsdir = @gabbletestsdir@"
> (or you might even be able to delete it entirely - automake might put that line
> in automatically), you'll have one less redundant variable, => one less source
> of confusion.

Renamed to gabbletestsdir.

> > tests: add --enable-install-tests
> 
> You renamed it, amend the commit message please.

Done

> If you're going to permute the order of things in a big list, please sort the
> new positions alphabetically while you're changing it anyway. That could be a
> separate, first, commit.

Lists moved around in 2 separate commits.

> > -TWISTED_JINGLE_TESTS = \
> 
> You've removed the (documented!) ability to "make check TWISTED_JINGLE_TESTS="
> when you know you haven't touched Jingle-specific code, so that it doesn't take
> 6 months to run the tests and then fail with an obscure race condition. (Or
> perhaps we've fixed the Jingle tests enough by now that only the first reason
> applies? Even so, it's a useful feature.)

I reverted the 4 specialized lists.

> > + $(AM_V_GEN)sed -e "s|[@]abs_top_tests_installdir[@]|$(GABBLE_TESTS_PATH)|g" \
> > + -e "s|[@]python[@]|$(PYTHON)|g" \
> 
> abs_top_tests_installdir is a bit misleading: it looks like a standard autoconf
> substitution, but it isn't. I'd use @GABBLE_TESTS_PATH@, or even
> @gabbletestsdir at .

Done: it will be @gabbletestsdir at .

> It's conventional to replace @Thing@ with $(Thing) (in any case combination),
> so the placeholder for Python should be @PYTHON@ instead of @python at .

Done.

> > + @mkdir -p tools
> 
> Not portable. Use $(MKDIR_P) instead (and make sure we have AC_PROG_MKDIR_P in
> the configure.ac, but I think we do already).

Done. I added a first separate commit to add AC_PROG_MKDIR_P and to MKDIR_P-ify
the Makefiles.

> > +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> > + echo "This script is meant to be installed"
> 
> You could at least say "... installed at $script_fullname".

Done.

> I'm not sure whether this use of readlink is portable away from Linux, but I'm
> not sure that I care either.

Ok, I keep readlink then.

> > +export PYTHONPATH=@abs_top_tests_installdir@/twisted
> 
> Not portable to all POSIX shells, use
> 
>     PYTHONPATH=...
>     export PYTHONPATH

Done

> (and the same in all #!/bin/sh scripts).

Done in a separate commit.

(In reply to comment #12)
> > # because test.la is not installed, libtool will want to compile it as static
> > # despite -shared (a convenience library), unless we also use -rpath
> > -test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)
> > +test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(testplugindir)
> 
> This won't work in the not-installing-tests case, because testplugindir won't
> be defined.
> 
> I'd just move "test_la_LDFLAGS = $(AM_LDFLAGS) -rpath $(plugindir)" inside the
> "not installing tests" branch of the conditional. If test.la is going to be
> installed in $(testplugindir), it gets that as an rpath automatically, so you
> don't need special workarounds to force it to be a shared library.

Done. I keep a dummy test_la_LDFLAGS in the other branch of the if, otherwise
autoconf complains.

--- Comment #15 from Will Thompson <will.thompson at collabora.co.uk> 2011-10-28 06:24:27 PDT ---
It seems a bit unfortunate that a separate dbus-daemon and Gabble are spawned
for every individual installed test one runs … but oh well!

I assume Gabble with this branch passes `make distcheck`?

Here are some nitpicks:

+        Test files installation.....:  ${installed_tests}

“Install unit tests”

+gabble-C-tests.list:
+    echo $(tests_list) > $@

If you use this:

  $(AM_V_GEN)echo $(tests_list) > $@

then it will show up as

  GEN     gabble-C-tests.list

in the output of make V=0.

-  const gchar *directory_name = g_getenv ("GABBLE_PLUGIN_DIR");
+  const gchar *directories_name = g_getenv ("GABBLE_PLUGIN_DIR");

'directory_names' would be better English.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.

More information about the telepathy-bugs mailing list